Skip to content

fix: flaky test flaky Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls#24741

Merged
seaona merged 3 commits intodevelopfrom
flaky-fix-request-queuing
May 24, 2024
Merged

fix: flaky test flaky Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls#24741
seaona merged 3 commits intodevelopfrom
flaky-fix-request-queuing

Conversation

@seaona
Copy link
Copy Markdown
Member

@seaona seaona commented May 23, 2024

Description

This PR fixes the flaky test Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls .

The problem is that sometimes when we call the const currentWindowHandles = await driver.getAllWindowHandles there is still the previous popup open, meaning that we get the total count of 4 windows.

Right after that, we trigger a send await driver.clickElement('#sendButton'); and now we wait for the windowHandles to be currentWindowHandles + 1 meaning we wait until we have 5 windows. However this will never happen as we should wait for 4 windows instead, but the previous windowCount was considering the previous popup, resulting in 1 more window in total.

  const currentWindowHandles = await driver.getAllWindowHandles();
  await driver.clickElement('#sendButton');
  const newWindowHandles = await driver.waitUntilXWindowHandles(
    process.env.ENABLE_MV3
      ? currentWindowHandles.length
      : currentWindowHandles.length + 1,

The proposed fix is to hardcode the expect number of windows directly, as we know how many there should be and we do this in the majority of the tests. There could be alternative approaches,

  • wait for a condition: though in this case the condition of having switched networks is already met in the wallet, so there doesn't seem to be another condition to wait for 🤔
  • add a delay

Related issues

Fixes: the third item of this list #24603

Manual testing steps

  1. Run the test several times yarn test:e2e:single test/e2e/tests/request-queuing/ui.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10
  2. Check ci jobs

Screenshots/Recordings

See how at one point, we get current windows as 4 (we expected 3) because the popup was still not closed and the next time we trigger a tx, we would expect 4+1, which will never happen.

flaky-window-handle.mp4

Pre-merge author checklist

  • I’ve followed MetaMask 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
Copy link
Copy Markdown
Contributor

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 seaona added team-extension-platform Extension Platform team flaky tests needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels May 23, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [54cf65f]
Page Load Metrics (822 ± 555 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint723061035124
domContentLoaded95617136
load6028938221155555
domInteractive95517136
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

// Windows: MetaMask, TestDapp1, TestDapp2
const expectedWindowHandles = 3;
await driver.waitUntilXWindowHandles(expectedWindowHandles);
const currentWindowHandles = await driver.getAllWindowHandles();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the flakiness came from this line: we were calling getAllWindowHandles, expecting to get 3 windows, however sometimes the popup from the previous step was not closed, meaning we could get 4 instead of 3. This made the test fail in the subsequent step where we wait for newWindowHandles which should be 3+1 (or in the case of flakyness 4+1 where this condition is never met)

const newWindowHandles = await driver.waitUntilXWindowHandles(
process.env.ENABLE_MV3
? currentWindowHandles.length
: currentWindowHandles.length + 1,
);

@seaona seaona marked this pull request as ready for review May 24, 2024 07:11
@seaona seaona requested a review from a team as a code owner May 24, 2024 07:11
Copy link
Copy Markdown
Contributor

@chloeYue chloeYue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [ddcf79d]
Page Load Metrics (283 ± 312 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6113882189
domContentLoaded9401273
load502233283649312
domInteractive9401273
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@seaona seaona merged commit 3053e0e into develop May 24, 2024
@seaona seaona deleted the flaky-fix-request-queuing branch May 24, 2024 09:25
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 24, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2024
@metamaskbot metamaskbot added release-11.16.6 Issue or pull request that will be included in release 11.16.6 and removed release-11.18.0 labels Jun 4, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Missing release label release-11.16.6 on PR. Adding release label release-11.16.6 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.6.

@seaona seaona self-assigned this Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

flaky tests release-11.16.6 Issue or pull request that will be included in release 11.16.6 team-extension-platform Extension Platform team

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants