-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Conversation
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. |
@@ -392,7 +392,7 @@ async function connectToDappTwoAndSwitchBackToOne( | |||
await driver.waitUntilXWindowHandles(4); | |||
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); | |||
|
|||
await driver.clickElement({ |
There was a problem hiding this comment.
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
await driver.waitUntilXWindowHandles(4); | ||
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); |
There was a problem hiding this comment.
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
await driver.waitForSelector({ | ||
css: '[id="chainId"]', | ||
text: '0x539', | ||
}); |
There was a problem hiding this comment.
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
Builds ready [3221328]
Page Load Metrics (1772 ± 79 ms)
Bundle size diffs
|
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.: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:
Related issues
Fixes: #30012
Manual testing steps
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