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

test: fix flaky snap signature test #29480

Merged
merged 10 commits into from
Jan 9, 2025
Merged

test: fix flaky snap signature test #29480

merged 10 commits into from
Jan 9, 2025

Conversation

pnarayanaswamy
Copy link
Contributor

@pnarayanaswamy pnarayanaswamy commented Jan 7, 2025

Description

Open in GitHub Codespaces

Related issues

Fixes: #29380

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

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.

Copy link
Contributor

github-actions bot commented Jan 7, 2025

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.

@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Jan 7, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [180a903]
Page Load Metrics (1618 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14961874161811254
domContentLoaded14861803159410450
load14961834161811153
domInteractive25176453718
backgroundConnect95925157
firstReactRender15102493215
getState55113147
initialActions00000
loadScripts1059135911939646
setupStore65919199
uiStartup167827561916261125

@metamaskbot
Copy link
Collaborator

Builds ready [065893f]
Page Load Metrics (1749 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15022075175016378
domContentLoaded14802030172416378
load15042077174916077
domInteractive23129432311
backgroundConnect986302010
firstReactRender1779442512
getState55615157
initialActions01000
loadScripts10481594129215675
setupStore66217199
uiStartup17082365199718589

@pnarayanaswamy pnarayanaswamy marked this pull request as ready for review January 8, 2025 13:34
@pnarayanaswamy pnarayanaswamy requested a review from a team as a code owner January 8, 2025 13:34
@@ -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

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

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.

@metamaskbot
Copy link
Collaborator

Builds ready [73d76e5]
Page Load Metrics (1738 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15222088173812259
domContentLoaded15071993170011656
load15222092173812460
domInteractive267437126
backgroundConnect1190372311
firstReactRender16107513517
getState495222713
initialActions00000
loadScripts1115144212528440
setupStore75313136
uiStartup175828132090275132

@metamaskbot
Copy link
Collaborator

Builds ready [21517af]
Page Load Metrics (1685 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1537191216829546
domContentLoaded1523189816559043
load1543191916859345
domInteractive219436199
backgroundConnect1095322411
firstReactRender16163524019
getState57119209
initialActions01000
loadScripts1107145412458742
setupStore684303115
uiStartup177526192087260125

@metamaskbot
Copy link
Collaborator

Builds ready [cc6dba9]
Page Load Metrics (1883 ± 143 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint41326811610535257
domContentLoaded153325921852297143
load155626511883298143
domInteractive22136493115
backgroundConnect86931189
firstReactRender1681412512
getState46619199
initialActions01000
loadScripts109919391373221106
setupStore65311115
uiStartup173030812132344165

@pnarayanaswamy pnarayanaswamy added this pull request to the merge queue Jan 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 8, 2025
@pnarayanaswamy pnarayanaswamy added this pull request to the merge queue Jan 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 8, 2025
@pnarayanaswamy pnarayanaswamy added this pull request to the merge queue Jan 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 8, 2025
@pnarayanaswamy pnarayanaswamy added this pull request to the merge queue Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@pnarayanaswamy pnarayanaswamy added this pull request to the merge queue Jan 9, 2025
Merged via the queue into main with commit ec75a8b Jan 9, 2025
77 checks passed
@pnarayanaswamy pnarayanaswamy deleted the fix-flaky-snaps-test branch January 9, 2025 08:46
@github-actions github-actions bot locked and limited conversation to collaborators Jan 9, 2025
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Jan 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix "Test Snap Signature Insights Redesigned confirmation..." flaky tests
4 participants