fix: flaky anti-pattern `getText` + assert #28041

seaona · 2024-10-23T13:02:04Z

Description

Continuing the work of removing the e2e anti-pattern of finding the element and then asserting its text.
There are more occurrences, but this work is split in several PRs, for an easy review and a faster ci.
Once all occurrences have been fixed, we'll be able to merge @HowardBraham 's PR for adding a lint rule which prevents introducing it again.

Related issues

Fixes: (but doesn't yet closes) #19870

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 · 2024-10-23T13:02:16Z

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.

seaona · 2024-10-23T13:05:54Z

test/e2e/tests/network/add-custom-network.spec.js

+  await driver.findElement({
+    xpath:
+      "//div[@data-testid='useSafeChainsListValidation']//label[contains(@class, 'toggle-button') and contains(@class, 'toggle-button--on')]",
+  });


we need to use an xpath here since there's a long list of toggles, without a way of differentiating to which feature belong to

metamaskbot · 2024-10-23T13:38:28Z

Builds ready [4cfcca0]

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

Page Load Metrics (2080 ± 140 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1755	2818	2087	284	136
		domContentLoaded	1737	2788	2036	282	135
		load	1747	2826	2080	292	140
		domInteractive	17	185	60	45	22
		backgroundConnect	10	123	39	33	16
		firstReactRender	49	139	91	24	12
		getState	5	61	22	19	9
		initialActions	0	1	0	0	0
		loadScripts	1249	2213	1529	257	124
		setupStore	13	92	32	23	11
		uiStartup	1944	3045	2294	301	145

Bundle size diffs

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

hjetpoluru

I think we could update this one in incremental-security.spec.js.

   const outerSegment = await driver.findElement(
          '.qr-code__address-segments',
        );
        const publicAddress = await outerSegment.getText();

hjetpoluru · 2024-10-23T16:13:44Z

test/e2e/tests/dapp-interactions/signin-with-ethereum.spec.js

@@ -13,7 +12,7 @@ const FixtureBuilder = require('../../fixture-builder');
 describe('Sign in with ethereum', function () {
  it('user should be able to confirm sign in with ethereum', async function () {
    const expectedSigninMessageTitle =
-      'This site is requesting to sign in with Account 1';
+      'This site is requesting to sign in with';


Good catch!!

hjetpoluru · 2024-10-23T16:18:42Z

test/e2e/tests/hardware-wallets/trezor-sign.spec.ts

-        );
+        await driver.waitForSelector({
+          css: '#signTypedDataV4VerifyResult',
+          text: KNOWN_PUBLIC_KEY_ADDRESSES[0].address.toLocaleLowerCase(),


minor nit: could we have this in variable and then pass the variable. Its ok if you dont make the change too.
let address = KNOWN_PUBLIC_KEY_ADDRESSES[0].address.toLocaleLowerCase()

sure I've updated it!

I don't think this change is needed here, it's clear enough the origin implementation.
But since you already made the change, we can keep it.

@chloeYue, yeah you're right. After the change was made, I also feel it's not required. I had noted this point..
My initial thought was to read the value, convert it to lowercase and then pass it.

@seaona, thanks for making the change. Since you've made the adjustment we can skip it for now.

hjetpoluru · 2024-10-23T16:48:34Z

test/e2e/tests/network/add-custom-network.spec.js

+          await driver.waitForSelector({
+            tag: 'dd',
+            text: currencySYMBOL,
+          });


Could you add a more specific data-testid for this element or make be some other approach?
The reason I mention this is - I changed the text for currencySYMBOL from ETH to ET, the test would not fail leading to false negatives.
I also checked for any other unique identifiers but there aren't any so we need to add one in the code.

Update on my previous comment - I think changing the data-testid would work.

nice catch! I've added test ids in the app level 🙏

hjetpoluru

I think we could update this one in incremental-security.spec.js.

   const outerSegment = await driver.findElement(
          '.qr-code__address-segments',
        );
        const publicAddress = await outerSegment.getText();

hjetpoluru · 2024-10-23T17:13:06Z

test/e2e/tests/account/incremental-security.spec.js

@@ -109,12 +109,10 @@ describe('Incremental Security', function () {
        await driver.switchToWindow(extension);

        // should have the correct amount of eth
-        let currencyDisplay = await driver.waitForSelector({
+        await driver.waitForSelector({


I think we need to come up with a different approach because this does not fail if the text is changed for waitForSelector, and it does fail with the previous assertion

hey @hjetpoluru 👋 I've seen the test fail if the text is changed from 1to another value 🤔 Do you see something different?

fail-selector.mp4

Hey @seaona, thanks for addressing the other issue. I tried changing the value to 0, and it did not fail. I will try again later today but if you can check earlier that would be great.

thank you @hjetpoluru let me check it and will report back 🙏

hjetpoluru

To my surprise this did not fail if the text was changed here

const backupReminder = await driver.findElements({
          xpath:
            "//div[contains(@class, 'home-notification__text') and contains(text(), 'Back up your Secret Recovery Phrase to keep your wallet and funds secure')]",
        });
        assert.equal(backupReminder.length, 1);

metamaskbot · 2024-10-24T08:16:11Z

Builds ready [5c99c5c]

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

Page Load Metrics (1962 ± 108 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1708	2642	1972	223	107
		domContentLoaded	1697	2622	1928	224	108
		load	1706	2640	1962	225	108
		domInteractive	17	109	51	20	10
		backgroundConnect	10	89	35	23	11
		firstReactRender	49	217	92	46	22
		getState	5	61	27	22	11
		initialActions	0	0	0	0	0
		loadScripts	1228	2133	1433	206	99
		setupStore	11	76	31	23	11
		uiStartup	1948	2843	2191	231	111

Bundle size diffs

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

getText assert anti-pattern

4cfcca0

github-actions bot added the team-extension-platform label Oct 23, 2024

seaona self-assigned this Oct 23, 2024

seaona commented Oct 23, 2024

View reviewed changes

seaona marked this pull request as ready for review October 23, 2024 13:20

seaona requested a review from a team as a code owner October 23, 2024 13:20

seaona added flaky tests area-qa Relating to QA work (Quality Assurance) labels Oct 23, 2024

seaona mentioned this pull request Oct 23, 2024

[Refactor] Replace getText() with findElement using css #19870

Open

hjetpoluru self-requested a review October 23, 2024 16:05

hjetpoluru reviewed Oct 23, 2024

View reviewed changes

HowardBraham mentioned this pull request Oct 23, 2024

feat: allow custom ESLint rules and add get-text-anti-pattern #27591

Draft

7 tasks

seaona added 3 commits October 24, 2024 09:32

address feedback

4189899

waitforselector

a3bacfa

waitforselector

5c99c5c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: flaky anti-pattern `getText` + assert #28041

fix: flaky anti-pattern `getText` + assert #28041

seaona commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024

seaona Oct 23, 2024

hjetpoluru Oct 23, 2024

metamaskbot commented Oct 23, 2024

hjetpoluru left a comment

hjetpoluru Oct 23, 2024

hjetpoluru Oct 23, 2024

seaona Oct 24, 2024

chloeYue Oct 24, 2024

hjetpoluru Oct 24, 2024

hjetpoluru Oct 23, 2024

hjetpoluru Oct 23, 2024 •

edited

Loading

seaona Oct 24, 2024

hjetpoluru left a comment

hjetpoluru Oct 23, 2024

seaona Oct 24, 2024

hjetpoluru Oct 24, 2024

seaona Oct 24, 2024

hjetpoluru left a comment

metamaskbot commented Oct 24, 2024

fix: flaky anti-pattern getText + assert #28041

Are you sure you want to change the base?

fix: flaky anti-pattern getText + assert #28041

Conversation

seaona commented Oct 23, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Oct 23, 2024

hjetpoluru left a 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

hjetpoluru Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjetpoluru left a 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

hjetpoluru left a comment

Choose a reason for hiding this comment

metamaskbot commented Oct 24, 2024

fix: flaky anti-pattern `getText` + assert #28041

fix: flaky anti-pattern `getText` + assert #28041

seaona commented Oct 23, 2024 •

edited

Loading

hjetpoluru Oct 23, 2024 •

edited

Loading