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 for Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls #24809

Merged
merged 1 commit into from
May 28, 2024

Conversation

seaona
Copy link
Contributor

@seaona seaona commented May 28, 2024

Description

This PR fixes the flaky test Request Queuing for Multiple Dapps and Txs on different networks. should switch to the dapps network automatically when handling sendTransaction calls.

The test fails at the point of trying to find the unapproved transaction, in the transaction list. The problem is that in the test, we are doing a browser refresh in order for the transaction to show up, but there is a race condition that, if you do a refresh once the unapproved tx is already present, you are redirected to the confirmation screen. This causes that the transaction cannot be found in the activity list and the test fails.

The solution is to change the approach for a more robust one, and instead of doing a browser refresh, we simply go explicitly to the activity tab and wait until the transaction appears.

Screenshot from 2024-05-28 11-45-58

Circle ci Screenshot: it's looking for the unapproved-transaction but it cannot find it because after reloading MetaMask, the screen is redirected to the unapproved transaction.

image

Open in GitHub Codespaces

Related issues

Fixes: #24603

Manual testing steps

  1. Check ci jobs
  2. Run test multiple times yarn test:e2e:single test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js --browser=firefox --leave-running --retryUntilFailure --retries=10ng/multiple-networks-dapps-txs

Screenshots/Recordings

Behavior when you reload the browser with the unapproved transaction already present --> see how you are redirected to the confirmation page

reload-unapproved.mp4

Solution: no browser refresh but instead, going to the activity tab and wait until tx appears (see last seconds on the video)

unappproved-tx-list-fix.mp4

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.

@seaona seaona self-assigned this May 28, 2024
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.

@metamaskbot
Copy link
Collaborator

Builds ready [5a4b70e]
Page Load Metrics (797 ± 532 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint66172983014
domContentLoaded97314147
load5526747971107532
domInteractive97314147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@seaona seaona marked this pull request as ready for review May 28, 2024 09:52
@seaona seaona requested a review from a team as a code owner May 28, 2024 09:52
// Second Switch Network
await driver.switchToWindowWithTitle(
WINDOW_TITLES.ExtensionInFullScreenView,
);
Copy link
Contributor Author

@seaona seaona May 28, 2024

Choose a reason for hiding this comment

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

  • we don't reload the browser anymore, and instead go to the activity tab explicitly
  • we already switched to the Extension window (see line146 above), so we can remove this step

@seaona seaona added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 28, 2024
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! Thanks!

@seaona seaona merged commit 4987677 into develop May 28, 2024
105 of 107 checks passed
@seaona seaona deleted the flaky-fix-multiple-networks branch May 28, 2024 10:04
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 28, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 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
Development

Successfully merging this pull request may close these issues.

Flaky Test: Multiple tests failing with error "No window with title: MetaMask Dialog"
4 participants