test: remove unnecessary delays and wait for condition with `waitForUrlContaning` #30265

seaona · 2025-02-12T14:05:40Z

Description

Just a small enhancement to remove some unnecessary delays in bridge specs.
This PR adds a new function in the driver waitForUrlContaning, which just checks for a substring, instead of the exact match with the waitForUrl. We can now use this and remove the hardcoded delays

Related issues

Fixes:

Manual testing steps

Check ci

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2025-02-12T14:05:52Z

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.

metamaskbot · 2025-02-12T16:03:58Z

Builds ready [08a9ed8]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1750 ± 92 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	322	2135	1683	364	175
		domContentLoaded	1494	2068	1716	175	84
		load	1503	2152	1750	191	92
		domInteractive	27	121	48	28	13
		backgroundConnect	10	137	34	30	14
		firstReactRender	16	77	33	21	10
		getState	4	57	20	19	9
		initialActions	0	1	0	0	0
		loadScripts	1059	1528	1255	143	69
		setupStore	7	62	17	17	8
		uiStartup	1689	2843	2036	261	125

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

davidmurdoch · 2025-02-12T15:56:18Z

test/e2e/tests/bridge/bridge-test-utils.ts

-    assert.ok((await this.driver.getCurrentUrl()).includes('asset'));
+    await this.driver.waitForUrlContaining({
+      url: 'asset',
+      timeout: this.driver.timeout,


is this.driver.timeout the same as the default timeout? if so, should we just remove timeout: this.driver.timeout, from these calls?

it is the same, indeed, but lint is failing if I don't add it explicitly :/

ah, you need to define the type as optional in the JSDoc for the function . See the "Optional parameter" section at https://jsdoc.app/tags-type

aww got it 😍 thank you so much, learning a lot from your reviews 🙏

davidmurdoch · 2025-02-12T15:57:00Z

test/e2e/tests/bridge/bridge-test-utils.ts

-        : expectedHandleCount + 1,
-    );
-    assert.match(await this.driver.getCurrentUrl(), /.+cross-chain\/swaps/u);
+    await this.driver.waitUntilXWindowHandles(expectedHandleCount);


Just curious why this is needed. seems like waitForUrlContaining should just wait.

I agree, this is not needed. I left the validation because this was explicitly validated in the original spec, but happy to remove it! We can completely remove the expectedHandleCount in the func singature too then 🙏

test/e2e/webdriver/driver.js

Co-authored-by: David Murdoch <[email protected]>

seaona · 2025-02-12T16:43:36Z

.circleci/config.yml

@@ -147,7 +146,6 @@ workflows:
            - prep-build-test
            - get-changed-files-with-git-diff
      - test-e2e-firefox:
-          <<: *main_master_rc_only


just for testing ci, will be removed once finished

all green https://app.circleci.com/pipelines/github/MetaMask/metamask-extension?branch=e2e-wait-url-containing

metamaskbot · 2025-02-12T18:01:39Z

Builds ready [b02539b]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1764 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	273	2153	1695	358	172
		domContentLoaded	1513	2100	1734	140	67
		load	1521	2187	1764	154	74
		domInteractive	26	78	43	17	8
		backgroundConnect	8	77	30	23	11
		firstReactRender	16	97	31	24	11
		getState	4	83	26	25	12
		initialActions	0	0	0	0	0
		loadScripts	1106	1550	1273	119	57
		setupStore	8	65	24	22	11
		uiStartup	1739	2576	2050	217	104

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

waitforurlcontaning

5ce544c

seaona requested a review from a team as a code owner February 12, 2025 14:05

metamaskbot added the team-qa QA team label Feb 12, 2025

seaona changed the title ~~test: remove unnecessary delays with waitforurlcontaning~~ test: remove unnecessary delays with waitForUrlContaning Feb 12, 2025

seaona self-assigned this Feb 12, 2025

seaona added flaky tests area-qa Relating to QA work (Quality Assurance) labels Feb 12, 2025

fix

08a9ed8

seaona changed the title ~~test: remove unnecessary delays with waitForUrlContaning~~ test: remove unnecessary delays and wait for condition instead with waitForUrlContaning Feb 12, 2025

davidmurdoch requested changes Feb 12, 2025

View reviewed changes

seaona and others added 3 commits February 12, 2025 17:40

David's comment: int to number

8e039b2

Co-authored-by: David Murdoch <[email protected]>

David's comment: return

4599be8

Co-authored-by: David Murdoch <[email protected]>

address comments

b10cdcb

seaona requested a review from a team as a code owner February 12, 2025 16:41

seaona commented Feb 12, 2025

View reviewed changes

lint

cb847c2

danjm previously approved these changes Feb 12, 2025

View reviewed changes

optional jsdoc and remove unnecessary value

78462ae

seaona dismissed danjm’s stale review via 78462ae February 12, 2025 16:56

add ci filters back

b02539b

seaona changed the title ~~test: remove unnecessary delays and wait for condition instead with waitForUrlContaning~~ test: remove unnecessary delays and wait for condition with waitForUrlContaning Feb 12, 2025

micaelae approved these changes Feb 12, 2025

View reviewed changes

davidmurdoch approved these changes Feb 12, 2025

View reviewed changes

seaona added this pull request to the merge queue Feb 12, 2025

Merged via the queue into main with commit 297e327 Feb 12, 2025
76 checks passed

seaona deleted the e2e-wait-url-containing branch February 12, 2025 18:52

github-actions bot locked and limited conversation to collaborators Feb 12, 2025

metamaskbot added the release-12.13.0 Issue or pull request that will be included in release 12.13.0 label Feb 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: remove unnecessary delays and wait for condition with `waitForUrlContaning` #30265

test: remove unnecessary delays and wait for condition with `waitForUrlContaning` #30265

seaona commented Feb 12, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025

metamaskbot commented Feb 12, 2025

davidmurdoch Feb 12, 2025

seaona Feb 12, 2025

davidmurdoch Feb 12, 2025

seaona Feb 12, 2025

davidmurdoch Feb 12, 2025

seaona Feb 12, 2025

seaona Feb 12, 2025

seaona Feb 12, 2025

metamaskbot commented Feb 12, 2025

test: remove unnecessary delays and wait for condition with waitForUrlContaning #30265

test: remove unnecessary delays and wait for condition with waitForUrlContaning #30265

Conversation

seaona commented Feb 12, 2025 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Feb 12, 2025

metamaskbot commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Feb 12, 2025

test: remove unnecessary delays and wait for condition with `waitForUrlContaning` #30265

test: remove unnecessary delays and wait for condition with `waitForUrlContaning` #30265

seaona commented Feb 12, 2025 •

edited

Loading