fix: "Phishing Detection Via Iframe should redirect users ..." flaky test#26779
Merged
hjetpoluru merged 7 commits intodevelopfrom Aug 30, 2024
Merged
fix: "Phishing Detection Via Iframe should redirect users ..." flaky test#26779hjetpoluru merged 7 commits intodevelopfrom
hjetpoluru merged 7 commits intodevelopfrom
Conversation
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. |
Collaborator
Builds ready [46a4892]
Page Load Metrics (1581 ± 63 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26779 +/- ##
========================================
Coverage 70.13% 70.13%
========================================
Files 1417 1417
Lines 49444 49444
Branches 13835 13835
========================================
Hits 34676 34676
Misses 14768 14768 ☔ View full report in Codecov by Sentry. |
hjetpoluru
commented
Aug 30, 2024
| text: 'continue to the site.', | ||
| }); | ||
| const header = await driver.findElement('h1'); | ||
| assert.equal(await header.getText(), 'E2E Test Dapp'); |
Contributor
Author
There was a problem hiding this comment.
Removed this code to prevent race conditions.
hjetpoluru
commented
Aug 30, 2024
| const header = await driver.findElement('h1'); | ||
| assert.equal(await header.getText(), 'E2E Test Dapp'); | ||
|
|
||
| await driver.delay(2000); |
Contributor
Author
There was a problem hiding this comment.
This is delay is needed to loading of dapp.
Collaborator
Builds ready [6e37386]
Page Load Metrics (1830 ± 49 ms)
Bundle size diffs
|
Contributor
|
I think the safest and fastest-executing way to approach this is and then no I have tested it myself, and it seems to work great. EDIT: Oh right, also we should be using the constant |
Collaborator
Builds ready [0ef1973]
Page Load Metrics (1706 ± 65 ms)
Bundle size diffs
|
Contributor
Author
|
thanks @HowardBraham, The approach you suggested works and is effective. I implemented all the points you mentioned, and the CI passes(yay yay!!). Please do approve at your convenience. |
… fix-flaky-phishing-detection-test
|
Collaborator
Builds ready [3d2d3e8]
Page Load Metrics (1688 ± 82 ms)
Bundle size diffs
|
HowardBraham
approved these changes
Aug 30, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix for flaky test "Phishing Detection Via Iframe should redirect users ..."
I am unable to reproduce the issue on my mac machine. However, based on the logs and screenshot, it appears that the Dapp was taking time to load after clicking the "Continue to the site" button on the phishing page.
I added a delay to allow the Dapp sufficient time to load completely before proceeding with further actions.switchToWindowWithTitlemethod with a conditional wait provided by Selenium. This approach works effectively and provides logs in case of failure when the title is not loaded.Related issues
Fixes:
#26505
Manual testing steps
Run the below commands locally or in codespaces:
yarn
ENABLE_MV3=false yarn start:test
ENABLE_MV3=false yarn test:e2e:single test/e2e/tests/phishing-controller/phishing-detection.spec.js --browser=firefox
The test was executed x5 times against all the builds. Below is the link for the Firefox browser as reported in the flaky test failure:
https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/98030/workflows/344ccffd-8671-46a9-83ab-5dce7a0b9cdd/jobs/3649076
Pre-merge author checklist
Pre-merge reviewer checklist