-
Notifications
You must be signed in to change notification settings - Fork 5k
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
fix: flaky test ERC721 NFTs testdapp interaction
#25854
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. |
Quality Gate passedIssues Measures |
await driver.waitForSelector({ | ||
css: '.confirm-page-container-summary__action__name', | ||
text: 'Deposit', | ||
}); | ||
await driver.clickElement({ text: 'Confirm', tag: 'button' }); | ||
await driver.waitUntilXWindowHandles(2); | ||
await driver.switchToWindow(extension); |
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.
changing this for switchToWindowWithTitle
which has more advance logic with retrials to mitigate flakiness, so we can remove all the window management adhoc steps from the spec file:
const windowHandles = await driver.getAllWindowHandles();
const [extension] = windowHandles;
@@ -38,19 +38,16 @@ describe('ERC721 NFTs testdapp interaction', function () { | |||
|
|||
// Notification | |||
await driver.waitUntilXWindowHandles(3); | |||
const windowHandles = await driver.getAllWindowHandles(); | |||
const [extension] = windowHandles; |
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.
Having the extension window as a variable and keep referencing it steps later can cause the context to be invalidated. This is not recommended pattern and instead we should get the windows at the moment we want to switch to each of them
Builds ready [a0fa95f]
Page Load Metrics (189 ± 235 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25854 +/- ##
========================================
Coverage 69.77% 69.77%
========================================
Files 1398 1398
Lines 49165 49165
Branches 13574 13574
========================================
Hits 34304 34304
Misses 14861 14861 ☔ View full report in Codecov by Sentry. |
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 ! Thanks for the quick fix !
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
Description
This PR fixes the flakiness of this specs due to how the window handling is managed in the spec file. When the test fails, we are in an invalidated context making the test flaky, when the current window is not correctly selected.
StaleElementReferenceError: stale element reference: stale element not found in the current frame
ci failure example: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/91382/workflows/70a1e3eb-3ff0-4e77-866a-fa2c4ed1d6ab/jobs/3407807/tests
Related issues
Fixes: #25855
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist