-
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: removing race condition for asserting inner values (PR-#1) #27606
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. |
Terms of use accepts the updated terms of use @no-mmi
Terms of use accepts the updated terms of use @no-mmi
test
Terms of use accepts the updated terms of use @no-mmi
test
Builds ready [274211e]
Page Load Metrics (1891 ± 96 ms)
Bundle size diffs
|
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); | ||
await driver.delay(2000); // Delay needed due to a race condition | ||
// To be fixed in https://github.com/MetaMask/metamask-extension/issues/25251 |
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.
this race condition can be mitigated by simply awaiting for the correct navigation values (ie 1 of 2
, etc). So all delays have been removed now
Quality Gate passedIssues Measures |
Builds ready [888dce8]
Page Load Metrics (1964 ± 124 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 ! Thanks for the refacto !
@seaona , This is interesting learning. It's very easy to miss out on and slightly difficult to stick with the pattern. Thanks for fixing them. |
Description
This PR fixes an anti-pattern in our e2e tests, where we assert that an element value is equal to a condition. This opens the door to a race condition where the element is already present, but not the value we want yet, making the assertion to fail.
We should find the element by its value, instead of asserting its inner value.
Related issues
Fixes: #19870
Note: we will need several PRs to address all the occurrences, so we cannot close the issue yet, despite merging this first PR
Manual testing steps
Screenshots/Recordings
n/a
Pre-merge author checklist
Pre-merge reviewer checklist