-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: migrate signature redesign tests to page object model #28538
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 [b7dc11a]
Page Load Metrics (1981 ± 105 ms)
Bundle size diffs
|
Builds ready [ff7afb9]
Page Load Metrics (1930 ± 233 ms)
Bundle size diffs
|
); | ||
|
||
// await driver.waitUntilXWindowHandles(2); | ||
// await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); |
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.
Remove commented out code?
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.
done
text: '0x581c3...45947', | ||
}; | ||
|
||
private nftTitle = { text: 'Withdrawal request' }; |
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.
[suggestion/non-blocker]: rather than private vars here, we could keep them outside of the class as a map structure. This would provide a quick lookup and instantiating a single object
e.g.
const SELECTOR = {
nft: { text: 'Withdrawal request' },
nftContractPetName: {
css: '.name__value',
text: '0x581c3...45947',
}
}
export default class PermitConfirmation extends Confirmation {
// ..
.. await this.driver.findElement(SELECTOR.nft);
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.
That's a good idea. But maybe best to do it out of the scope of this PR as there are already many pages using the current pattern for now. I'd also like to run it by the extension team.
Builds ready [81b6437]
Page Load Metrics (1705 ± 74 ms)
Bundle size diffs
|
Builds ready [3070f53]
Page Load Metrics (1625 ± 46 ms)
Bundle size diffs
|
Description
Migrates the existing signature redesign e2e tests to use page object model.
Adds the relevant pages and updates the tests using the newly created pages.
Related issues
Fixes: #28540
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist