fix: flaky test Navigation Signature - Different signature types initiates multiple signatures and rejects all
#27481
Conversation
seaona
added
team-extension-platform
flaky tests
area-qa
|
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. |
| @@ -98,7 +98,6 @@ describe('Navigation Signature - Different signature types', function (this: Sui | |||
| await unlockWallet(driver); | |||
| await openDapp(driver); | |||
| await queueSignatures(driver); | |||
| await driver.delay(regularDelayMs); | |||
There was a problem hiding this comment.
this makes the test fail when the delay is not enough
|
|
||
| await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); | ||
| await driver.clickElement('#signTypedDataV4'); | ||
| await driver.waitUntilXWindowHandles(3); | ||
| await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); | ||
| await driver.waitForSelector(By.xpath("//div[normalize-space(.)='1 of 3']")); |
There was a problem hiding this comment.
we wait for the navigation to be loaded. Since the inner value has spaces we need to normalize it
There was a problem hiding this comment.
@seaona, small nit could it possible to wait for any other stable element in the dialog to load?
There was a problem hiding this comment.
hi @hjetpoluru I've seen that waiting for the navigation to be loaded ensures that the signatures have been queued.
We could wait for the signature content, but that's not directly fixing the issue. What were you thinking on?
[Edit] I think with page objects, we could add a function for checking that the complete signature page is loaded if we want to, though I would leave this out of scope for this fix
There was a problem hiding this comment.
@seaona, I was thinking about waiting for the buttons to load. But after reading your reply, it looks like we need to wait for the navigation to be loaded to ensure the signatures are queued. Maybe we could add a data-testId to the page loader? Otherwise, I will approve this PR as it looks good.
There was a problem hiding this comment.
so in this case, we need to wait for the content of the element, to ensure that it has the expected value (1 of 2 or 1 of 3 etc.), looking just for the element itself with a data test id, could cause a race condition of the element being present, but not fulfilling the condition we need (the number of signatures queued). Let me know if this makes sense 🙇♀️
There was a problem hiding this comment.
Oh perfect @seaona thanks for your patience and explaining this to me.
There was a problem hiding this comment.
no problem! Thank you for your detailed review @hjetpoluru 🙇♀️
|
Builds ready [9922b38]
Page Load Metrics (1777 ± 111 ms)
Bundle size diffs
|
metamaskbot
added
the
release-12.6.0
Description
There is a delay for waiting the signatures to be queued, which sometimes is not enough. In this fix, we remove the delay and we add conditions to wait for (each new signature is added in the navigation), so the behaviour is deterministic.
Related issues
Fixes: #27480
Manual testing steps
yarn test:e2e:single test/e2e/tests/confirmations/navigation.spec.ts --browser=chrome --leave-running=trueScreenshots/Recordings
See how the last signature is not properly queued, so in the last screen we don't see the navigation. We should wait on each new signature to be added in the navigation queue, before adding a new one
queued-sign-flaky.mp4
Pre-merge author checklist
Pre-merge reviewer checklist