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 test Send ETH from inside MetaMask finds the transaction #24639

Merged
merged 1 commit into from
May 21, 2024

Conversation

seaona
Copy link
Contributor

@seaona seaona commented May 20, 2024

Description

The problem with this flaky test is that in some occurrences we are proceeding to the last Confirmation screen without the balance being loaded (race condition). This can be observed at second 4 from this video. This test fails every time this condition is met (very often in Mutlichain build, possibly due to shorter flow of UI?)
The fix is simply waiting for the balance to be loaded before proceeding with the actions.

However, I think we should also revise the application behaviour, as it does not seem correct that:

  • whenever we proceed to the last screen without the balance being loaded, the gas is set to 0 (specifically the gas limit)
  • then, no matter how long we wait, the gas cannot be recovered from this and we are stucked in the confirmation screen (Confirm is disabled) --> this happens despite the balance being then loaded and also getting numerous successful responses from the gas API

I'll open separate tickets for that

Example of failure: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/81147/workflows/ec5f9950-23fa-4f6f-b248-9f894813e75d/jobs/2874889/tests

e2e-insufficient-balance.mp4

Open in GitHub Codespaces

Related issues

Fixes: #24574

Manual testing steps

  1. Check ci jobs
  2. Run test multiple times locally yarn test:e2e:single test/e2e/tests/transaction/send-eth.spec.js --browser=chrome --leave-running=true --retry-until-failure --retries=15

Screenshots/Recordings

In the test failing artifacts we can see how we cannot proceed with the transaction as the Confirm button is disabled. However the problem is that the gas is set to 0 and never recovered, but the real root cause comes from proceeding with the previous screen without the balance loaded.

image

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

@seaona seaona self-assigned this May 20, 2024
@seaona seaona added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) flaky tests team-extension-platform labels May 20, 2024
@seaona seaona marked this pull request as ready for review May 20, 2024 15:08
@seaona seaona requested a review from a team as a code owner May 20, 2024 15:08
@metamaskbot
Copy link
Collaborator

Builds ready [086ce3b]
Page Load Metrics (972 ± 533 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint65151952512
domContentLoaded95916126
load5325319721110533
domInteractive95916126
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 cb7cc64 into develop May 21, 2024
122 of 124 checks passed
@seaona seaona deleted the flaky-send-multisig-multichain branch May 21, 2024 09:35
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 21, 2024
@metamaskbot metamaskbot added release-11.16.6 Issue or pull request that will be included in release 11.16.6 and removed release-11.18.0 labels Jun 4, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.16.6 on PR. Adding release label release-11.16.6 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.6.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
flaky tests release-11.16.6 Issue or pull request that will be included in release 11.16.6 team-extension-platform
Projects
Archived in project
4 participants