Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: flaky test Queued Confirmations Queued Requests Banner Alert Banner is shown on dApp 1, but not on dApp 2 after adding transaction on dApp 1, and one on dApp 2 (old confirmation flow) #30028

Merged
merged 3 commits into from
Jan 31, 2025

Conversation

seaona
Copy link
Contributor

@seaona seaona commented Jan 31, 2025

Description

The test Queued Confirmations Queued Requests Banner Alert Banner is shown on dApp 1, but not on dApp 2 after adding transaction on dApp 1, and one on dApp 2 (old confirmation flow) is flaky and fails with the following error.:

Screenshot from 2025-01-31 08-39-06

There is one fundamental problem which is that in the test we expect that switchChain will trigger a dialog, but that shouldn't be the case, as we already have permissions to that network (comes selected by default, see video below). Then the window management is messed up.

Roughly:

  • Whenever we connect to the dapp2, we don't wait for the dialog window to close
  • We switch to dapp 1
  • We trigger a switchChain request --> here we wait for the dialog to be there. The problem is that the switchChain request don't trigger a dialog (that's expected as that network already has permissions by default)
  • So now, depending on how fast the dialog from step 1 remains open, will make it to the next step, or not (if the dialog is close fast enough). That's why it doesn't fail all the time (but it should, given the incorrect expectation of waiting for a dialog after the request to switch chains, to an already approved chain)
  • Then the subsequent actions can work with an old instance of the dialog, but that is closed after some seconds (as it was the Connect dialog which we already accepted) making it fail

Open in GitHub Codespaces

Related issues

Fixes: #30012

Manual testing steps

  1. Check ci run Note: you'll see there's a spec failing but that's a different one that will be tackled in a separate PR, issue here
  2. Run test locally yarn test:e2e:single test/e2e/tests/confirmations/alerts/queued-confirmations.spec.ts --browser=chrome --leave-running=true

Screenshots/Recordings

See how the localhost 7777 is already selected by default. This willl make that any request to switch to that chain won't trigger any dialog (ExpecteD)

connect-permissions-default.mp4

See same behaviour in a prod build

prod-connect-default.mp4

Pre-merge author checklist

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.

@seaona seaona self-assigned this Jan 31, 2025
Copy link
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 flaky tests area-qa Relating to QA work (Quality Assurance) labels Jan 31, 2025
@metamaskbot metamaskbot added the team-qa QA team label Jan 31, 2025
@seaona seaona marked this pull request as ready for review January 31, 2025 07:44
@@ -392,7 +392,7 @@ async function connectToDappTwoAndSwitchBackToOne(
await driver.waitUntilXWindowHandles(4);
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);

await driver.clickElement({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

race condition here, we need to wait for the dialog to close, before proceeding to the dapp 1 window

Comment on lines -420 to -421
await driver.waitUntilXWindowHandles(4);
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);
Copy link
Contributor Author

@seaona seaona Jan 31, 2025

Choose a reason for hiding this comment

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

incorrect expectations, as the dialog should never open in this case, as that network already has permissions. We should just expect that the network has been switched.
The incorrect expectations together with the above race condition, made that this test passes sometimes, because we wait for 4 windows, and this is the case if the dialog from the step before is not closed fast enough (is possible because we didn't wait for the dialog to close in the first place), but that dialog instance doesn't exist anymore in the next steps

@seaona seaona requested a review from a team as a code owner January 31, 2025 07:52
await driver.waitForSelector({
css: '[id="chainId"]',
text: '0x539',
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we just wait for the test dapp to have switched to the new network

@metamaskbot
Copy link
Collaborator

Builds ready [3221328]
Page Load Metrics (1772 ± 79 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15822207177716378
domContentLoaded15602128174315173
load15862209177216579
domInteractive2599462411
backgroundConnect880342512
firstReactRender1673372211
getState44812105
initialActions00000
loadScripts10701620127913263
setupStore767242010
uiStartup17372473201418388
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@seaona seaona added this pull request to the merge queue Jan 31, 2025
Merged via the queue into main with commit 1f6f110 Jan 31, 2025
79 checks passed
@seaona seaona deleted the flaky-fix-queued-switch branch January 31, 2025 10:37
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2025
@metamaskbot metamaskbot added the release-12.13.0 Issue or pull request that will be included in release 12.13.0 label Jan 31, 2025
@seaona seaona restored the flaky-fix-queued-switch branch January 31, 2025 10:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-qa Relating to QA work (Quality Assurance) flaky tests release-12.13.0 Issue or pull request that will be included in release 12.13.0 team-qa QA team
Projects
None yet
5 participants