-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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 hardware wallet e2e tests to follow Page Object Model #28768
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 [9182760]
Page Load Metrics (1702 ± 69 ms)
Bundle size diffs
|
Builds ready [2eb0cfd]
Page Load Metrics (1911 ± 78 ms)
Bundle size diffs
|
*/ | ||
async signTypedDataV4() { | ||
async signTypedDataV4(confirmationRedesign: boolean = false) { |
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.
Keep the confirmationRedesign
as a parameter as a temporary solution, will do the refacto once the redesign toggle is removed, as planned in MetaMask-planning issue #3030.
Builds ready [524a8fb]
Page Load Metrics (2034 ± 106 ms)
Bundle size diffs
|
Builds ready [89e2f7a]
Page Load Metrics (1625 ± 36 ms)
Bundle size diffs
|
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.
Nice job @chloeYue, I notice minot observation otherwise everything is looking good.
Co-authored-by: Harika <[email protected]>
Builds ready [bc3c4e1]
Page Load Metrics (1862 ± 92 ms)
Bundle size diffs
|
test/e2e/page-objects/pages/hardware-wallet/connect-hardware-wallet-page.ts
Show resolved
Hide resolved
test/e2e/page-objects/pages/hardware-wallet/connect-hardware-wallet-page.ts
Show resolved
Hide resolved
test/e2e/page-objects/pages/hardware-wallet/connect-hardware-wallet-page.ts
Show resolved
Hide resolved
test/e2e/page-objects/pages/hardware-wallet/select-trezor-account-page.ts
Show resolved
Hide resolved
…allet-page.ts Co-authored-by: Derek Brans <[email protected]>
…allet-page.ts Co-authored-by: Derek Brans <[email protected]>
…sk/metamask-extension into hardware-wallet-migration
Builds ready [d31ac54]
Page Load Metrics (1931 ± 63 ms)
Bundle size diffs
|
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.
Nice work, Chloe!
const testDappPage = new TestDappPage(driver); | ||
await testDappPage.openTestDappPage(); | ||
await testDappPage.check_pageIsLoaded(); | ||
await testDappPage.signTypedDataV4(true); |
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.
I think passing an object with the flag name, would make it clearer what this boolean is actually indicating. However given that handling redesign/no-redesign is temporary I think it's fine as it is
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.
yeah it's a very temporary solution. Let's wait for the removal of the toggle as planned in this ticket, and then we can refactor the redesign together with other parts of the testing code to keep them coherent.
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.
LGTM!!
Description
Related issues
Fixes: #28808
Manual testing steps
Check code readability, make sure tests pass.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist