fix(25350): fix flakey token importing e2e test #26351
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. |
| @@ -42,6 +42,7 @@ describe('Token List', function () { | |||
| await driver.clickElement( | |||
| '[data-testid="import-tokens-modal-import-button"]', | |||
| ); | |||
| await driver.findElement({ text: 'Token imported', tag: 'h6' }); | |||
There was a problem hiding this comment.
wait until token modal is closed and token is properly imported before asserting
| @@ -155,40 +156,30 @@ describe('Token List', function () { | |||
| async ({ driver }: { driver: Driver }) => { | |||
| await unlockWallet(driver); | |||
| await importToken(driver); | |||
| await driver.delay(500); | |||
There was a problem hiding this comment.
removed with better await here
| await driver.findElement( | ||
| `[data-testid="token-increase-decrease-percentage"]`, | ||
| ) | ||
| ).getText(); |
There was a problem hiding this comment.
There is no timeout for this assertion, we need to give the element sometime to render and fetch.
Builds ready [3bdd4b5]
Page Load Metrics (384 ± 363 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26351 +/- ##
========================================
Coverage 70.10% 70.10%
========================================
Files 1430 1430
Lines 50144 50147 +3
Branches 13870 13872 +2
========================================
+ Hits 35149 35152 +3
Misses 14995 14995 ☔ View full report in Codecov by Sentry. |
| @@ -155,40 +156,30 @@ describe('Token List', function () { | |||
| async ({ driver }: { driver: Driver }) => { | |||
| await unlockWallet(driver); | |||
| await importToken(driver); | |||
| await driver.delay(500); | |||
|
|
|||
| // Verify native token increase | |||
| const testIdNative = `token-increase-decrease-percentage-${zeroAddress()}`; | |||
|
|
|||
| // Verify native token increase | |||
| const testId = `token-increase-decrease-percentage-${tokenAddress}`; | |||
|
|
|||
There was a problem hiding this comment.
We have a recommended pattern for when we need to assert data that might be updated asynchronously. It is best if we have explicit assertions (separate from the findElement call) and it is necessary to also include expected text in the find element call. See the docs for the recommended pattern: https://github.com/MetaMask/contributor-docs/blob/main/docs/e2e/extension-e2e-guidelines.md#guidelines-3
Could you update it to follow that sort of pattern and maintain the use of explicit assert calls?
There was a problem hiding this comment.
Thanks for linking the pattern, it is indeed needed for text assertion, changed here 8d5843a
|
Updated after 8d5843a |
|
Builds ready [8d5843a]
Page Load Metrics (225 ± 222 ms)
Bundle size diffs
|
metamaskbot
added
the
release-12.4.0
gauthierpetetin
added
release-12.3.0
Description
Captured in https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/95033/workflows/faea9f93-9600-4207-bb3c-2dc17eec663b/jobs/3536505/tests.
This test is flaky from not waiting for token price to be fetched and rendered properly. The simple fix would be
findVisibleElementand the built intimeoutto wait for element to be rendered properlyAttached to this PR is a report here of running it 20 times after the fix and results in no flakiness.
Related issues
Fixes: #26350
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist