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

fix: flaky test Navigate transactions should reject and remove all unapproved transactions #25312

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

seaona
Copy link
Contributor

@seaona seaona commented Jun 14, 2024

Description

This PR fixes the flaky test Navigate transactions should reject and remove all unapproved transactions.
It fails with the error TimeoutError: Waiting for element to be located By(xpath, //button[contains(text(), "Reject all")]).

The problem is that we click the Reject button, before the confirmation screen has fully loaded (specifically, the total amount for that tx is not there yet). This causes the click to don't have any effect and the subsequent popup with the Reject All never appears.
The fix is simply await for the tx to be fully loaded (notice how the total amount box is not loaded at the moment of the failure below).

Furthermore, it's been observed that the unapproved tx where trying to load simulations (see loading spinner). This shouldn't prevent to continue, but to further stabilize the test, the transactions have now disabled the simulations.

Open in GitHub Codespaces

Related issues

Fixes: #24661 and #24641

Manual testing steps

  1. Check ci
  2. Run test locally multiple times yarn test:e2e:single test/e2e/tests/transaction/navigate-transactions.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10

Screenshots/Recordings

Before

Notice how when the test failed, the total tx amount was yet not loaded (3.000,..) and we only see the gas displayed.

Furthermore: we see the tx simulation element loading (this is not the cause of this failure, but it has also disabled, to stabilize the screen).

image

After

Notice that the tx simulations loading element is not there.
Notice how the correct amount is await until proceeding (3.0000315)

Screenshot from 2024-06-14 11-33-56

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

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.

@DDDDDanica
Copy link
Contributor

LGTM ! A very legit fix indeed !

DDDDDanica
DDDDDanica previously approved these changes Jun 14, 2024
@seaona
Copy link
Contributor Author

seaona commented Jun 14, 2024

thank you @DDDDDanica ❤️ 🙇‍♀️

ℹ️ I've updated the PR to add the mitigation to the rest of the tests which have the same setup (preloaded transactions), to make sure it doesn't fail in the future in any other of the it blocks

@metamaskbot
Copy link
Collaborator

Builds ready [60e1300]
Page Load Metrics (121 ± 156 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint61997594
domContentLoaded8281042
load391534121324156
domInteractive8281042
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.67%. Comparing base (95b10da) to head (2e344e8).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #25312   +/-   ##
========================================
  Coverage    65.67%   65.67%           
========================================
  Files         1376     1376           
  Lines        54546    54546           
  Branches     14298    14298           
========================================
  Hits         35819    35819           
  Misses       18727    18727           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seaona seaona requested a review from DDDDDanica June 14, 2024 11:32
@metamaskbot
Copy link
Collaborator

Builds ready [2e344e8]
Page Load Metrics (53 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint731068694
domContentLoaded10161221
load41765394
domInteractive9161221
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@chloeYue chloeYue left a comment

Choose a reason for hiding this comment

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

LGTM

@seaona seaona merged commit 4aa2fd0 into develop Jun 14, 2024
74 checks passed
@seaona seaona deleted the flaky-tx-simulations-disabled branch June 14, 2024 13:25
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
flaky tests release-12.1.0 Issue or pull request that will be included in release 12.1.0 team-extension-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

flaky test: Navigate transactions should reject and remove all unapproved transactions
4 participants