fix: flaky test `ERC1155 NFTs testdapp interaction should batch transfers ERC1155 token` #27897

seaona · 2024-10-16T10:05:30Z

Description

Same problem as #27889.

We are looking for transactions by its text in the activity tab, but the transaction element updates its state, from pending to confirm, meaning that it can become stale when we do the assertion after.

await driver.waitForSelector({
          css: '[data-testid="activity-list-item-action"]',
          text: 'Deposit',
});
assert.equal(await transactionItem.isDisplayed(), true);

Related issues

Fixes: #27896

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-16T10:05:44Z

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-16T10:07:58Z

test/e2e/tests/tokens/nft/erc1155-interaction.spec.js

-          await transactionItem.isDisplayed(),
-          true,
-          `transaction item should be displayed in activity tab`,
-        );


this is an anti-pattern, that causes the race condition, when we do the assert if the element is updated (from pending to confirmed)

seaona · 2024-10-16T10:08:10Z

test/e2e/tests/tokens/nft/erc1155-interaction.spec.js

-          await transactionItem.isDisplayed(),
-          true,
-          `transaction item should be displayed in activity tab`,
-        );


anti-pattern

seaona · 2024-10-16T10:08:22Z

test/e2e/tests/tokens/nft/erc1155-interaction.spec.js

-        const displayedDescription = await driver.findElement(
-          '.confirm-approve-content__description',
-        );
-        assert.equal(await displayedDescription.getText(), expectedDescription);


all these above are anti-patterns

Do you think we could write a regular expression (or a piece of code in general) to detect this anti-pattern? Because then we could write a lint rule for it.

whoa I think that is worth investigating, as this anti-pattern is re-introduced over and over in our tests unfortunately :( Despite having this documented as an anti-pattern. I can create a ticker for this.
Would it be something like: forbid anything in the form of assert.equal(await something.getText(), something)

Hmm you're thinking of a much simpler test than I was thinking of. I was thinking of considering the findElement part too. I think no matter what we write, there's going to be false positives and false negatives. There are times when assert.equal(await something.getText(), something) is actually the correct solution. But I guess we can force an eslint-disable-next-line in those cases.

ℹ️ as info, we have this ticket which reflects the work of removing this anti-pattern, and we've been adding some fixes around that:

#19870

In parallel, adding this lint rule (if it's possible) could make that no more new tests are using this pattern anymore

I was thinking of considering the findElement part too. I think no matter what we write, there's going to be false positives and false negatives. There are times when assert.equal(await something.getText(), something) is actually the correct solution

oh that is a really good point! Yeah maybe the rule needs to be a bit more elaborated indeed, otherwise we'll be force to disable lint in legit occasions, as you mention 🤔

seaona · 2024-10-16T10:08:31Z

test/e2e/tests/tokens/nft/erc1155-interaction.spec.js

-        const displayedDescription = await driver.findElement(
-          '.confirm-approve-content__description',
-        );
-        assert.equal(await displayedDescription.getText(), expectedDescription);


anti-patterns

seaona · 2024-10-16T10:08:40Z

test/e2e/tests/tokens/nft/erc1155-interaction.spec.js

          css: '.transaction-list__completed-transactions',
          text: 'Approve Token with no spend limit',
        });
-        assert.equal(await revokeApprovalItem.isDisplayed(), true);


anti-pattern

sonarqubecloud · 2024-10-16T10:16:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-10-16T10:44:20Z

Builds ready [7a40481]

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 (1969 ± 131 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1684	2594	1960	259	125
		domContentLoaded	1670	2435	1913	219	105
		load	1680	2665	1969	274	131
		domInteractive	15	189	57	39	19
		backgroundConnect	10	315	55	71	34
		firstReactRender	54	167	98	27	13
		getState	5	157	26	36	17
		initialActions	0	1	0	0	0
		loadScripts	1214	1919	1436	182	87
		setupStore	11	149	41	35	17
		uiStartup	1871	3570	2240	417	200

Bundle size diffs

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

hjetpoluru

LGTM

remove assert race conditions with stale elems

7a40481

seaona added team-extension-platform Extension Platform team flaky tests area-qa Relating to QA work (Quality Assurance) labels Oct 16, 2024

seaona self-assigned this Oct 16, 2024

seaona requested a review from a team as a code owner October 16, 2024 10:05

seaona commented Oct 16, 2024

View reviewed changes

seaona mentioned this pull request Oct 16, 2024

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

Open

HowardBraham approved these changes Oct 16, 2024

View reviewed changes

seaona enabled auto-merge October 16, 2024 10:54

hjetpoluru self-requested a review October 16, 2024 12:08

hjetpoluru approved these changes Oct 16, 2024

View reviewed changes

seaona added this pull request to the merge queue Oct 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2024

seaona added this pull request to the merge queue Oct 16, 2024

Merged via the queue into develop with commit 71de55b Oct 16, 2024
94 checks passed

seaona deleted the erc1155-flaky-assert branch October 16, 2024 13:22

github-actions bot locked and limited conversation to collaborators Oct 16, 2024

metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 16, 2024

gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: flaky test `ERC1155 NFTs testdapp interaction should batch transfers ERC1155 token` #27897

fix: flaky test `ERC1155 NFTs testdapp interaction should batch transfers ERC1155 token` #27897

seaona commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024

seaona Oct 16, 2024

seaona Oct 16, 2024

seaona Oct 16, 2024

HowardBraham Oct 16, 2024

seaona Oct 16, 2024

HowardBraham Oct 16, 2024

seaona Oct 16, 2024

seaona Oct 16, 2024 •

edited

Loading

seaona Oct 16, 2024

seaona Oct 16, 2024

sonarqubecloud bot commented Oct 16, 2024

metamaskbot commented Oct 16, 2024

hjetpoluru left a comment

fix: flaky test ERC1155 NFTs testdapp interaction should batch transfers ERC1155 token #27897

fix: flaky test ERC1155 NFTs testdapp interaction should batch transfers ERC1155 token #27897

Conversation

seaona commented Oct 16, 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 16, 2024

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

seaona Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 16, 2024

Quality Gate passed

metamaskbot commented Oct 16, 2024

hjetpoluru left a comment

Choose a reason for hiding this comment

fix: flaky test `ERC1155 NFTs testdapp interaction should batch transfers ERC1155 token` #27897

fix: flaky test `ERC1155 NFTs testdapp interaction should batch transfers ERC1155 token` #27897

seaona commented Oct 16, 2024 •

edited

Loading

seaona Oct 16, 2024 •

edited

Loading