-
Notifications
You must be signed in to change notification settings - Fork 5k
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: [POM] Migrate connections e2e tests to TS and Page Object Model #29609
base: main
Are you sure you want to change the base?
Conversation
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 [d8189fc]
Page Load Metrics (1777 ± 73 ms)
|
Builds ready [8dc3e3c]
Page Load Metrics (1540 ± 48 ms)
Bundle size diffs
|
Builds ready [fa94554]
Page Load Metrics (1797 ± 61 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [be2da98]
Page Load Metrics (1807 ± 63 ms)
Bundle size diffs
|
LGTM ! |
console.log('Disconnect all permissions for accounts and networks'); | ||
await this.driver.clickElement(this.disconnectButton); | ||
await this.driver.waitForSelector(this.disconnectModalTitle); | ||
await this.driver.clickElementAndWaitToDisappear( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more flakiness mitigation, nice 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Just added a small nit, let me know if it makes sense!
@@ -612,13 +612,16 @@ class TestDapp { | |||
* @param options - Options for connecting account to test dapp. | |||
* @param [options.connectAccountButtonEnabled] - Indicates if the connect account button should be enabled. | |||
* @param options.publicAddress - The public address to connect to test dapp. | |||
* @param [options.networkId] - The network to connect to, defaults to 0x539. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*/ | ||
async connectAccount({ | ||
connectAccountButtonEnabled = true, | ||
publicAddress, | ||
networkId = '0x539', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
networkId = '0x539', | |
chainId = '0x539', |
}: { | ||
connectAccountButtonEnabled?: boolean; | ||
publicAddress?: string; | ||
networkId?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
networkId?: string; | |
chainId?: string; |
await this.driver.waitForSelector(this.localhostNetworkMessage); | ||
await this.driver.waitForSelector({ | ||
css: '#chainId', | ||
text: networkId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text: networkId, | |
text: chainId, |
Description
Follow PR #29384 and continue migrating the remaining connection e2e tests to TypeScript and the Page Object Model.
Specs migrated:
Related issues
Fixes:
Manual testing steps
Check code readability, make sure tests pass.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist