-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: [POM] Migrate autodetect and import nft e2e tests to use Page Object Model #28383
base: develop
Are you sure you want to change the base?
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [60d54eb]
Page Load Metrics (1885 ± 136 ms)
Bundle size diffs
|
Builds ready [b5ba5fb]
Page Load Metrics (2109 ± 141 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!! nice job 🙌
async ({ driver }) => { | ||
await loginWithBalanceValidation(driver); | ||
|
||
// navigate to security & privacy settings and toggle on NFT autodetection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chloeYue, this is a suggestion: could we include comments in the test console.log?
I feel that when analyzing any flaky test, it would be helpful especially when actions and pages are repeated like visiting the settings twice or if the test has lot of switching windows.
Including the test console.log would be beneficial. You don't have to modify this PR, but if you think it would help please do include them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hjetpoluru , yes, I totally agree that we currently have very poor logging and need to add more console.log
statements for page interactions. However, I try to put these logs in the page class functions instead of the test bodies. This way, when people call these functions, the logs are automatically added. Please see the current log on this branch for autodetect NFT:
@chloeYue, general question: how do you determine which flaky tests you want to migrate? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and neat.
Hi @hjetpoluru, thanks for your comment, that's a good question. Generally, I choose flaky tests that cover important functionalities and have a lower likelihood of being fixed by other teams. For example, confirmation team has people actively working on e2e tests and creating new tests following Page Object Models, so I leave confirmation related tests to them. For assets-related, accounts-related, and wallet UX related tests, I try to prioritize them because these teams don't have a dedicated QA engineer and need more support. Additionally, the functionalities for these teams are foundational for many other scenarios. For instance, many other scenarios require switching accounts and sending transactions. By fixing flaky tests for accounts and assets, I create Page Object Model classes with stable functions that are ready to use, which benefits other feature teams and other functionalities as well. Another priority example is onboarding. Onboarding tests were flaky and belong to extension platform team, so I also prioritize onboarding tests. |
Description
test/e2e/tests/tokens/nft/auto-detect-nft.spec.js
to TS and Page Object Model, to reduce flakiness.test/e2e/tests/tokens/nft/import-nft.spec.js
to TS and Page Object Model, to reduce flakiness.helper.js
Related issues
Fixes: #28388
Manual testing steps
Check code readability, make sure tests pass.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist