-
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: [Snaps E2E] Unified methods and clean up snaps e2e tests #27684
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. |
Before there's any question about the added delays, all snaps tests failed test-e2e-firefox until the delays were reinstated. In an attempt to remain in method parity between all tests, documented delays have been added for all scrollTo actions. |
Builds ready [3bc665b]
Page Load Metrics (1746 ± 42 ms)
Bundle size diffs
|
const snapButton1 = await driver.findElement('#connectbip32'); | ||
await driver.scrollToElement(snapButton1); | ||
|
||
// added delay for firefox (deflake) | ||
await driver.delay(1000); |
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.
Wondering if it's possible to conditionally add the delay for just firefox?
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.
Yes it is, and now is.
Added a function in driver.js
called delayFirefox
which only delays if firefox is the current browser.
await driver.delay(1000); | ||
|
||
// wait for and click connect | ||
await driver.waitForSelector('#connecterrors'); |
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.
I see you're doing this in some other tests so I'll just comment once. Why do we need to have both the waitForSelector
call here with an increase in the delay?
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.
Well, because at first, it didn't hurt. In the case now, because those delays are "firefox specific" we need them for chrome instances.
970771e
to
94f1e6b
Compare
Builds ready [94f1e6b]
Page Load Metrics (1792 ± 74 ms)
Bundle size diffs
|
Builds ready [9b116f3]
Page Load Metrics (1796 ± 81 ms)
Bundle size diffs
|
Delay: another round of changes i'm working on has to be finished before reviewing this code again. |
9b116f3
to
475660b
Compare
Quality Gate passedIssues Measures |
fixed merge error
Builds ready [dba0b52]
Page Load Metrics (2053 ± 155 ms)
Bundle size diffs
|
Builds ready [52b9b24]
Page Load Metrics (2117 ± 83 ms)
Bundle size diffs
|
Description
Because of several factors, over time, the E2E tests had diverged in their use of different methods to do the same thing. This PR represents work to unify these methods throughout the entire test base. It also was is an attempt to, as well as possible, document each individual step in the tests.
Some delays were removed in lieu of waitForSelector when appropriate.
Other delays were standardized. For example, Firefox requires delays to allow
scrollToElement
to complete, hence the new Firefox Specific Delay™ has been added todriver.js
asdelayFirefox(time)
because these delays are not reuired in Chrome instances.Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist