-
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 Request-queue UI changes handles three confirmations on three confirmations concurrently
#25675
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. |
Quality Gate passedIssues Measures |
// Differs between confirmation and signature | ||
'.confirm-page-container-summary__origin bdi, .request-signature__origin .chip__label', | ||
); | ||
const originText = await originElement.getText(); |
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.
this is a bad pattern that can cause flakiness: if we get the element without waiting for the exact text contents we want, it can be a race condition when the element is rendered but the value is not there yet, making all the assertions fail
Builds ready [33d806b]
Page Load Metrics (286 ± 249 ms)
Bundle size diffs
|
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.
LGTM ! Thanks !
Description
The queue UI test fails has a couple of flaky points.
Sometimes we see the console errors for network unresponsive -- when this happens the test fails.
When we don't see the console errors for network unresponsive the test passes. The problem is that in the tests we were ignoring the PollingBlock errors, but more errors appear when the network is down, making the test fail when they appear in the console.
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/90602/workflows/e41e1e65-d445-4b60-9602-89a261cc5654/jobs/3362082/parallel-runs/4?filterBy=ALL
There are more sources of flakiness, that can also cause this test to fail. Those are described here and fixed in this same PR:
Performing a subsequent action when the popup is closed, causes the
NoSuchWindowError: no such window: target window already closed
error. We need to switch back to an existing window, before proceeding with test actionsTriggering a tx/signature with an unresponsive network (when we kill ganache) takes a significant amount of time to open the popup. Now we wait for the expected number of windows before switching to the popup and we add a big timeout for this case (it's a max value and won't delay the test if it's not needed, like in FF). There's an issue open to fix this on the app level in this ticket.
Asserting values from elements found by their css/testid can introduce flakiness as the elements could be rendered without having the expected text yet loaded. Now the approach for these validations has been fixed by looking for the exact selector which includes the text
A lavamoat error appeared once, but this seem unrelated to this specific test and just saw it once so it's left outside the scope of this PR
error caught in retry(): JavascriptError: javascript error: LavaMoat - property "name" of globalThis is inaccessible under scuttling mode. To learn more visit https://github.com/LavaMoat/LavaMoat/pull/360.
Failure 2:
Logs:
Failure 3:
Logs from error 2:
failing-test-window-ui.mp4
Related issues
Fixes: #25674
Manual testing steps
Screenshots/Recordings
Pre-merge author checklist
Pre-merge reviewer checklist