test: Added e2e for switch network #27967
Merged
Conversation
This file contains 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
hjetpoluru
added
team-extension-platform
area-qa
|
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. |
Builds ready [f043170]
Page Load Metrics (1811 ± 101 ms)
Bundle size diffs
|
hjetpoluru
requested review from
seaona,
HowardBraham,
benjisclowder and
chloeYue
Builds ready [185abee]
Page Load Metrics (1799 ± 87 ms)
Bundle size diffs
|
chloeYue
reviewed
Oct 21, 2024
Builds ready [aa5f232]
Page Load Metrics (2158 ± 456 ms)
Bundle size diffs
|
Builds ready [2caa271]
Page Load Metrics (2005 ± 117 ms)
Bundle size diffs
|
seaona
reviewed
Oct 23, 2024
There was a problem hiding this comment.
Nice, the test looks good 🔥 Just added a couple of suggestions.
I would suggest some naming changes:
notification.tsseems quite a vague name -> this component represents the Network Confirmations modal for switching networks, so I would name something likenetwork-switch-modal-confirmationor similar, so it's easier to know what it refers to (see pic)
dialog/...I think the folder name is again quite vague -> here both components refer to the network picker and modal, so we could have some folder name more explicit
What do you think?
There was a problem hiding this comment.
Thanks @seaona for the detailed and valuable feedback.
Name Change
- I agree with you that
notification.tsis generic and a more accurate name as you suggested, will benetwork-switch-modal-confirmationand will make the change. - Since Dialog is the component, anything related to it should come under this folder. I can't think of any other options. Could you suggest some alternatives? It would be helpful.
seaona
reviewed
Oct 23, 2024
|
|
||
| async clickApproveButton(): Promise<void> { | ||
| console.log('Click Approve Button'); | ||
| await this.driver.clickElement(this.submitButton); |
There was a problem hiding this comment.
to avoid race conditions:
Suggested change
| await this.driver.clickElement(this.submitButton); | |
| await this.driver.clickElementAndWaitToDisappear( | |
| this.submitButton, | |
| ); |
There was a problem hiding this comment.
I actually tried using clickElementAndWaitToDisappear but it was unable to click properly. Since this is a dialog and I used clickElement instead. I will try again and check locally, and I will keep you updated.
There was a problem hiding this comment.
Ok I have updated clickElementAndWaitToDisappear and it worked locally and pushed up the changes.
seaona
reviewed
Oct 23, 2024
| await homePage.check_expectedBalanceIsDisplayed('25'); | ||
| await headerNavbar.check_currentSelectedNetwork('Localhost 8545'); | ||
|
|
||
| // Add Aribtrum network and perform the switch network functionality |
There was a problem hiding this comment.
Typo: Aribtrum should be Arbitrum
The test is called: Switch network - Ethereum Mainnet and Sepolia, however here we are adding Arbitrum network and switching to it too.
I would suggest either split this into 2 different tests and add more precise test naming, or change the test naming so it represents the complete test
There was a problem hiding this comment.
Good catch, I will change this..
There was a problem hiding this comment.
@seaona, I believe this test scenario is a simple one, so I have simplified the test name and provided comments. This should be sufficient. Please let me know if anything else needs to be updated
seaona
reviewed
Oct 23, 2024
|
|
||
| async clickCloseButton(): Promise<void> { | ||
| console.log('Click Close Button'); | ||
| await this.driver.clickElement(this.closeButton); |
There was a problem hiding this comment.
Suggested change
| await this.driver.clickElement(this.closeButton); | |
| await this.driver.clickElementAndWaitToDisappear( | |
| this.closeButton, | |
| ); |
There was a problem hiding this comment.
I have updated this change.
seaona
reviewed
Oct 23, 2024
|
|
||
| async clickAddButton(): Promise<void> { | ||
| console.log('Click Add Button'); | ||
| await this.driver.clickElement('[data-testid="test-add-button"]'); |
There was a problem hiding this comment.
Suggested change
| await this.driver.clickElement('[data-testid="test-add-button"]'); | |
| await this.driver.clickElementAndWaitToDisappear( | |
| '[data-testid="test-add-button"]', | |
| ); |
There was a problem hiding this comment.
I have updated this change.
Builds ready [70b9cd1]
Page Load Metrics (2076 ± 93 ms)
Bundle size diffs
|
seaona
reviewed
Oct 24, 2024
Co-authored-by: seaona <54408225+seaona@users.noreply.github.com>
Builds ready [c08dbca]
Page Load Metrics (2183 ± 97 ms)
Bundle size diffs
|
… switch-network-e2e
Builds ready [2318c32]
Page Load Metrics (2205 ± 176 ms)
Bundle size diffs
|
davidmurdoch
approved these changes
Oct 24, 2024
DDDDDanica
approved these changes
Oct 24, 2024
metamaskbot
added
the
release-12.7.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
This is e2e PR for switch network
Related issues
Fixes:
https://github.com/MetaMask/MetaMask-planning/issues/2906
Manual testing steps
Run the test locally or in codespaces using the below command:
yarn
yarn build:test:webpack
ENABLE_MV3=false yarn test:e2e:single test/e2e/tests/network/switch-network.spec.ts --browser=chrome
Pre-merge reviewer checklist