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.

Sign up for GitHub

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

test: fix flaky snap signature test #29480

Merged
merged 10 commits into from
Jan 9, 2025
+13 −3
Merged

test: fix flaky snap signature test #29480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
69bafed
test: fix flaky snap signature test
pnarayanaswamy Jan 7, 2025
f1ac41e
chore: increase delay
pnarayanaswamy Jan 8, 2025
39d85fa
chore: change selector for arrow
pnarayanaswamy Jan 8, 2025
180a903
Merge branch 'main' into fix-flaky-snaps-test
pnarayanaswamy Jan 8, 2025
8bfeb48
chore: reduce delay
pnarayanaswamy Jan 8, 2025
065893f
Merge branch 'fix-flaky-snaps-test' of github.com:MetaMask/metamask-e…
pnarayanaswamy Jan 8, 2025
0150013
chore: remove ff job
pnarayanaswamy Jan 8, 2025
73d76e5
Merge branch 'main' into fix-flaky-snaps-test
pnarayanaswamy Jan 8, 2025
21517af
Merge branch 'fix-flaky-snaps-test' of github.com:MetaMask/metamask-e…
pnarayanaswamy Jan 8, 2025
cc6dba9
Merge branch 'main' into fix-flaky-snaps-test
pnarayanaswamy Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/e2e/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ async function clickSignOnSignatureConfirmation({
if (snapSigInsights) {
// there is no condition we can wait for to know the snap is ready,
// so we have to add a small delay as the last alternative to avoid flakiness.
await driver.delay(regularDelayMs);
await driver.delay(largeDelayMs);
}
await driver.waitForSelector(
{ text: 'Sign', tag: 'button' },
Expand Down
14 changes: 12 additions & 2 deletions test/e2e/snaps/test-snap-siginsights.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,13 @@ describe('Test Snap Signature Insights', function () {
// switch back to MetaMask window and switch to tx insights pane
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);

await driver.waitForSelector(
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the below clickElementSafe also wait for the selector, or does it take so long we need to wait twice as long and we can't change the timeout?

Copy link
Contributor Author

@pnarayanaswamy pnarayanaswamy Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The clickElementSafe waits for elementsLocated, while the selector is waiting for it to be visible both serve different purposes. When an element exists in the dom it does not necessarily mean it is visible/interactable on the page hence the extra check. Also the wait is an intelligent one, so as soon as the condition is met it moves on, so we don't really add any extra time to the test if the element is present. The timeout is just the max time it waits before failing.

Copy link
Member

@matthewwalsh0 matthewwalsh0 Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the confusion, I'm aware the timeout isn't fixed, I was asking if there was a big delay in the test meaning we needed to wait twice to generate a sufficient delay.

Out of scope for this PR, but if one method is silently checking visibility, and the other just whether it exists, perhaps we could benefit from a requireVisible property on clickElementSafe to minimise duplication like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'll make that change in another PR.

'[data-testid="signature-request-scroll-button"]',
);
// click down arrow
await driver.clickElementSafe('.fa-arrow-down');
await driver.clickElementSafe(
'[data-testid="signature-request-scroll-button"]',
);

// wait for and click sign
await clickSignOnSignatureConfirmation({
Expand Down Expand Up @@ -223,7 +228,12 @@ describe('Test Snap Signature Insights', function () {
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);

// click down arrow
await driver.clickElementSafe('.fa-arrow-down');
await driver.waitForSelector(
'[data-testid="signature-request-scroll-button"]',
);
await driver.clickElementSafe(
'[data-testid="signature-request-scroll-button"]',
);

// wait for and click sign
await clickSignOnSignatureConfirmation({
Expand Down
Loading