test: add driver.clickElementAndWaitForWindowToClose helper method#26449
test: add driver.clickElementAndWaitForWindowToClose helper method#26449davidmurdoch merged 19 commits intodevelopfrom
driver.clickElementAndWaitForWindowToClose helper method#26449Conversation
|
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. |
driver.withWindowUntilClose helper method
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26449 +/- ##
===========================================
+ Coverage 70.12% 70.17% +0.05%
===========================================
Files 1428 1424 -4
Lines 50096 49857 -239
Branches 13896 13852 -44
===========================================
- Hits 35127 34985 -142
+ Misses 14969 14872 -97 ☔ View full report in Codecov by Sentry. |
Builds ready [00b649a]
Page Load Metrics (226 ± 231 ms)
Bundle size diffs
|
f75e65b to
97b54fe
Compare
1) switchToNotificationWindow 2) driver.switchToWindowWithTitle with a second parameter (that second param is now just ignored)
97b54fe to
d803179
Compare
| await driver.clickElement('#connectjsx'); | ||
|
|
||
| // switch to metamask extension and click connect | ||
| await switchToNotificationWindow(driver, 2); |
There was a problem hiding this comment.
switchToNotificationWindow is deprecated, so they've been removed as a bit of a drive-by refactor.
| @@ -1,14 +1,13 @@ | |||
| import { strict as assert } from 'assert'; | |||
| import { MockedEndpoint } from 'mockttp'; | |||
There was a problem hiding this comment.
just fixing types in this file.
| @@ -1,21 +1,12 @@ | |||
| import { Suite } from 'mocha'; | |||
| import { withFixtures, logInWithBalanceValidation } from '../../helpers'; | |||
There was a problem hiding this comment.
just fixing types in this file.
| @@ -1,20 +1,12 @@ | |||
| import { Suite } from 'mocha'; | |||
| import { withFixtures, logInWithBalanceValidation } from '../../helpers'; | |||
There was a problem hiding this comment.
just fixing types in this file.
|
|
||
| const convertETHToHexGwei = (eth) => convertToHexValue(eth * 10 ** 18); | ||
|
|
||
| /** |
There was a problem hiding this comment.
it was helpful to have proper types when refactoring to use the new withWindowUntilClose method, so I added them here (and fixed several usages of withFixtures).
driver.withWindowUntilClose helper methoddriver.clickElementAndWaitForWindowToClose helper method
Builds ready [d803179]
Page Load Metrics (62 ± 9 ms)
Bundle size diffs
|
|
Builds ready [5613a52]
Page Load Metrics (265 ± 270 ms)
Bundle size diffs
|
| async clickElementAndWaitForWindowToClose(rawLocator, retries = 3) { | ||
| const handle = await this.driver.getWindowHandle(); | ||
| await this.clickElement(rawLocator, retries); | ||
| await this.waitForWindowToClose(handle); | ||
| } | ||
|
|
||
| /** | ||
| * Waits for the specified window handle to close before returning. | ||
| * | ||
| * @param {string} handle - The handle of the window or tab we'll wait for. | ||
| * @param {number} [timeout] - The amount of time in milliseconds to wait | ||
| * before timing out. Defaults to `this.timeout`. | ||
| * @throws {Error} throws an error if the window handle doesn't close within | ||
| * the timeout. | ||
| */ | ||
| async waitForWindowToClose(handle, timeout = this.timeout) { | ||
| const start = Date.now(); | ||
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { | ||
| const handles = await this.getAllWindowHandles(); | ||
| if (!handles.includes(handle)) { | ||
| return; | ||
| } | ||
|
|
||
| const timeElapsed = Date.now() - start; | ||
| if (timeElapsed > timeout) { | ||
| throw new Error( | ||
| `waitForWindowToClose timed out waiting for window handle '${handle}' to close.`, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
How do you feel about adding a title? It would allow for slightly easier-to-read error messages.
| async clickElementAndWaitForWindowToClose(rawLocator, retries = 3) { | |
| const handle = await this.driver.getWindowHandle(); | |
| await this.clickElement(rawLocator, retries); | |
| await this.waitForWindowToClose(handle); | |
| } | |
| /** | |
| * Waits for the specified window handle to close before returning. | |
| * | |
| * @param {string} handle - The handle of the window or tab we'll wait for. | |
| * @param {number} [timeout] - The amount of time in milliseconds to wait | |
| * before timing out. Defaults to `this.timeout`. | |
| * @throws {Error} throws an error if the window handle doesn't close within | |
| * the timeout. | |
| */ | |
| async waitForWindowToClose(handle, timeout = this.timeout) { | |
| const start = Date.now(); | |
| // eslint-disable-next-line no-constant-condition | |
| while (true) { | |
| const handles = await this.getAllWindowHandles(); | |
| if (!handles.includes(handle)) { | |
| return; | |
| } | |
| const timeElapsed = Date.now() - start; | |
| if (timeElapsed > timeout) { | |
| throw new Error( | |
| `waitForWindowToClose timed out waiting for window handle '${handle}' to close.`, | |
| ); | |
| } | |
| } | |
| } | |
| async clickElementAndWaitForWindowToClose(rawLocator, retries = 3) { | |
| const handle = await this.driver.getWindowHandle(); | |
| const title = await this.driver.getTitle(); | |
| await this.clickElement(rawLocator, retries); | |
| await this.waitForWindowToClose(handle, title); | |
| } | |
| /** | |
| * Waits for the specified window handle to close before returning. | |
| * | |
| * @param {string} handle - The handle of the window or tab we'll wait for. | |
| * @param {number} [timeout] - The amount of time in milliseconds to wait | |
| * before timing out. Defaults to `this.timeout`. | |
| * @throws {Error} throws an error if the window handle doesn't close within | |
| * the timeout. | |
| */ | |
| async waitForWindowToClose(handle, title, timeout = this.timeout) { | |
| const start = Date.now(); | |
| // eslint-disable-next-line no-constant-condition | |
| while (true) { | |
| const handles = await this.getAllWindowHandles(); | |
| if (!handles.includes(handle)) { | |
| return; | |
| } | |
| const timeElapsed = Date.now() - start; | |
| if (timeElapsed > timeout) { | |
| throw new Error( | |
| `waitForWindowToClose timed out waiting for window handle '${handle}' with title '${title}' to close.`, | |
| ); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
I went down this route to add the title to the error message, but I found it likely to be flaky. The way it's prototyped above, where title isn't guaranteed to be the actual title of the window due to race conditions between multiple async calls to query the windows (this async nature of these calls seem to be the cause of another race condition in our tests), seems especially prone to flakiness.
That said, all of these async window interactions are potentially flaky since the windows can be changing while we are interacting with them. I'm in favor of removing as much potential flakiness as possible from the hot path.
If the async query for the current window title were to be made in the error path of waitForWindowToClose it might be slightly better... but a title fetched by getTitle isn't guaranteed to match that of the referenced window handle (since the windowHandle could finally close during the call to getTitle, in which case it would return a different title).
All this just makes me want to switch to a better browser testing framework, if there is one, and/or extend socket-background.to-mocha.ts to automatically sync all tab state (chrome.tabs.query) with the server, and implement event-driven interactions (e.g., windowHandle.on("close", ...)) and maybe even some sort of interaction lock mechanism.
There are lots of tests that click a button in our popup window (AKA "MetaMask Dialog") that Eventually close the popup, but they don't wait for the popup to close before continuing on with the rest of the test.
This can cause some race conditions due to the way our test infrastructure finds windows (by title). The problem is that we look for windows of the name "MetaMask Dialog", and in some cases we find 2, we start referencing the first one, then it closes (as it should), and when we try to reference it again Selenium throws (because the window no longer exists).
So this PR introduces a new method "driver.clickElementAndWaitForWindowToClose" that will click the element (just as before), but then wait until the window closes before returning.
Related issues
Fixes:
#26369
#26353
#26387