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: removing race condition for asserting inner values (PR-#1) #27606

Merged
merged 6 commits into from
Oct 7, 2024

Conversation

seaona
Copy link
Contributor

@seaona seaona commented Oct 3, 2024

Description

This PR fixes an anti-pattern in our e2e tests, where we assert that an element value is equal to a condition. This opens the door to a race condition where the element is already present, but not the value we want yet, making the assertion to fail.
We should find the element by its value, instead of asserting its inner value.

Open in GitHub Codespaces

Related issues

Fixes: #19870
Note: we will need several PRs to address all the occurrences, so we cannot close the issue yet, despite merging this first PR

Manual testing steps

  1. Check ci

Screenshots/Recordings

n/a

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.

@seaona seaona self-assigned this Oct 3, 2024
@seaona seaona requested a review from a team as a code owner October 3, 2024 17:34
Copy link
Contributor

github-actions bot commented Oct 3, 2024

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 changed the title fix: flaky test Terms of use accepts the updated terms of use @no-mmi test: enhance Terms of use accepts the updated terms of use @no-mmi test Oct 3, 2024
@seaona seaona changed the title test: enhance Terms of use accepts the updated terms of use @no-mmi test test: enhancement and removing extra steps in tests Oct 3, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [274211e]
Page Load Metrics (1891 ± 96 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16602378188519795
domContentLoaded16522240185418287
load16602393189119996
domInteractive26133462713
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@seaona seaona changed the title test: enhancement and removing extra steps in tests test: enhancement removing race condition for asserting inner values Oct 7, 2024
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);
await driver.delay(2000); // Delay needed due to a race condition
// To be fixed in https://github.com/MetaMask/metamask-extension/issues/25251
Copy link
Contributor Author

@seaona seaona Oct 7, 2024

Choose a reason for hiding this comment

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

this race condition can be mitigated by simply awaiting for the correct navigation values (ie 1 of 2, etc). So all delays have been removed now

Copy link

sonarcloud bot commented Oct 7, 2024

@seaona seaona added flaky tests area-qa Relating to QA work (Quality Assurance) labels Oct 7, 2024
@seaona seaona changed the title test: enhancement removing race condition for asserting inner values test: enhancement removing race condition for asserting inner values (PR-#1) Oct 7, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [888dce8]
Page Load Metrics (1964 ± 124 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29426271873449215
domContentLoaded159526221932265127
load166426271964259124
domInteractive17184644321
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 ! Thanks for the refacto !

@seaona seaona changed the title test: enhancement removing race condition for asserting inner values (PR-#1) test: removing race condition for asserting inner values (PR-#1) Oct 7, 2024
@seaona seaona added this pull request to the merge queue Oct 7, 2024
Merged via the queue into develop with commit b7b3bdd Oct 7, 2024
102 checks passed
@seaona seaona deleted the settings-terms-use branch October 7, 2024 12:47
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 7, 2024
@hjetpoluru
Copy link
Contributor

@seaona , This is interesting learning. It's very easy to miss out on and slightly difficult to stick with the pattern. Thanks for fixing them.

@gauthierpetetin gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-qa Relating to QA work (Quality Assurance) flaky tests release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-extension-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Refactor] Replace getText() with findElement using css
6 participants