test: new switchToWindowWithTitle w/ Extension communication #25362
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/[email protected], npm/[email protected] |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25362 +/- ##
===========================================
- Coverage 69.68% 69.67% -0.01%
===========================================
Files 1405 1405
Lines 49701 49705 +4
Branches 13738 13740 +2
===========================================
Hits 34630 34630
- Misses 15071 15075 +4 ☔ View full report in Codecov by Sentry. |
HowardBraham
force-pushed
the
e2e/switchToWindowWithTitle
branch
from
2ab973d to
4fae996
Compare
Builds ready [4fae996]
Page Load Metrics (201 ± 283 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
HowardBraham
force-pushed
the
e2e/switchToWindowWithTitle
branch
2 times, most recently
from
48dabcd to
fba217e
Compare
Builds ready [fba217e]
Page Load Metrics (141 ± 186 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
HowardBraham
added
needs-dev-review
HowardBraham
force-pushed
the
e2e/switchToWindowWithTitle
branch
from
fba217e to
a55651a
Compare
Builds ready [a55651a]
Page Load Metrics (45 ± 2 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Really nice catch 😍 this is precisely what happens in the vault decrypt test, where we had 2 windows and webdriver active window didn't match chrome's active window (this) |
7 tasks
seaona
reviewed
Jun 21, 2024
HowardBraham
force-pushed
the
e2e/switchToWindowWithTitle
branch
from
ef218a6 to
09533c6
Compare
Builds ready [09533c6]
Page Load Metrics (196 ± 221 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [8b9b1a4]
Page Load Metrics (171 ± 185 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
HowardBraham
dismissed
davidmurdoch’s stale review
Fixed everything and he is OOO
Builds ready [bef19fe]
Page Load Metrics (166 ± 202 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Introduced a new socket communication mechanism between Mocha/Selenium and the MetaMask extension's background script to improve E2E test stability.
app/scripts/background.js: AddedgetSocketBackgroundToMochafor socket communication in test environments.app/scripts/offscreen.js: Integrated socket initialization for test environments.test/e2e/background-socket/server-mocha-to-background.ts: New WebSocket server for Mocha/Selenium communication.test/e2e/background-socket/socket-background-to-mocha.ts: New WebSocket client for background script communication.test/e2e/helpers.js: Updated test functions to utilize the new socket communication mechanism.
22 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
Comment on lines
+27
to
+31
| if (this.ws) { | ||
| console.error( | ||
| 'ServerMochaToBackground got a second client connection, closing the first one', | ||
| ); | ||
| this.ws.close(); |
There was a problem hiding this comment.
Logic: Potential issue with handling multiple WebSocket connections. Consider adding logic to manage multiple connections more gracefully.
Comment on lines
+86
to
+88
| throw new Error( | ||
| `No window found by background script with ${message.property}: ${message.value}`, | ||
| ); |
There was a problem hiding this comment.
Logic: Throwing an error here might cause the entire test suite to fail. Consider handling this more gracefully or providing more context.
vthomas13
approved these changes
Jul 23, 2024
github-actions
bot
removed
the
needs-dev-review
metamaskbot
added
the
release-12.3.0
HowardBraham
added
contributor experience
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This creates a Socket between Mocha/Selenium and the Extension background script (service worker in MV3).
I had originally hoped that we could send a message to the Extension, and make the Extension switch windows. But it seems like Selenium's idea of "which tab/window are we looking at" is not the same as which one the browser has active. In fact, the "real user" can change tabs, and Selenium still works on the old tab. So an attempt to change tabs in the browser effectively does nothing.
My second attempt was to send back a list of open tabs and their names. This almost works, except the order of the tab lists is different between the browser list and the Selenium list. So we can't really act on that information to switch to a tab.
So the thing I went for is to ask the Extension to wait until there's a window open with the target title or url, and then inform Mocha/Selenium that it exists and to look for it.
These changes made a small number of tests more flaky, so I made some minor changes to them to work better with the new system.
Related issues
Manual testing steps
This is a special CircleCI workflow where I run every E2E test 4 times each. Some of these jobs took 40 minutes, with very few failures (and only on existing flaky tests). This PR makes things more stable.
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/87754/workflows/02b77156-ef91-45d3-9367-e8ebe2ee2f1f
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist