-
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: Migrating snaps test to POM #30312
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 [e8b5b7a]
Page Load Metrics (1658 ± 85 ms)
Bundle size diffs
|
Builds ready [837a61f]
Page Load Metrics (1831 ± 190 ms)
Bundle size diffs
|
Builds ready [0456648]
Page Load Metrics (1854 ± 104 ms)
Bundle size diffs
|
Builds ready [044aa8d]
Page Load Metrics (1662 ± 85 ms)
Bundle size diffs
|
Builds ready [03a6ca3]
Page Load Metrics (1727 ± 85 ms)
Bundle size diffs
|
Builds ready [ca1bbbc]
Page Load Metrics (1494 ± 33 ms)
|
Builds ready [be21a99]
Page Load Metrics (3132 ± 575 ms)
Bundle size diffs
|
Builds ready [fdd607e]
Page Load Metrics (5696 ± 3578 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.
This seems much better. I mostly left some nits.
const inputLocator: { [key: string]: string } = { | ||
entropyMessageInput: '#entropyMessage', | ||
messageBip44Input: '#bip44Message', | ||
messageEd25519Bip32Input: '#bip32Message-ed25519Bip32', | ||
messageEd25519Input: '#bip32Message-ed25519', | ||
messageSecp256k1Input: '#bip32Message-secp256k1', | ||
wasmInput: '#wasmInput', | ||
}; |
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.
Nit:
const inputLocator: { [key: string]: string } = { | |
entropyMessageInput: '#entropyMessage', | |
messageBip44Input: '#bip44Message', | |
messageEd25519Bip32Input: '#bip32Message-ed25519Bip32', | |
messageEd25519Input: '#bip32Message-ed25519', | |
messageSecp256k1Input: '#bip32Message-secp256k1', | |
wasmInput: '#wasmInput', | |
}; | |
const inputLocator = { | |
entropyMessageInput: '#entropyMessage', | |
messageBip44Input: '#bip44Message', | |
messageEd25519Bip32Input: '#bip32Message-ed25519Bip32', | |
messageEd25519Input: '#bip32Message-ed25519', | |
messageSecp256k1Input: '#bip32Message-secp256k1', | |
wasmInput: '#wasmInput', | |
} satisfies Record<string, string>; |
This helps with intellisense.
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.
ok sure I will try changes and execute them locally.
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.
console.log('Wait and click get public key button'); | ||
await this.driver.waitForSelector(this.publicKeyBip44Button); | ||
await this.driver.clickElement(this.publicKeyBip44Button); | ||
async fillMessageTestSnapsPage(inputElement: string, message: 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.
The TestSnapsPage
in this and other methods feels redundant. When you're calling this, it would typically look like
testSnaps.fillMessageTestSnapsPage(...);
I think omitting the TestSnapsPage
is clear enough:
testSnaps.fillMessage(...);
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.
ok agreed I will make the changes
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.
This makes totally sense and addressed in the commit dc915ef
Also, I updated the other ones too.
async selectEntropySource(id: string, name: string) { | ||
console.log('Select entropy source'); | ||
const selector = await this.driver.findElement(`#${id}-entropy-selector`); | ||
async scrollAndSelectEntropySource(dropDownName: string, name: 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.
I think it's fine in this case to expect callers to provide the actual locator, instead of using the switch statement.
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.
ok I will make the changes
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 agreed now it looks much clear after the update in the commit dc915ef
…tension into snaps-migration-pom
Builds ready [3a28043]
Page Load Metrics (2858 ± 675 ms)
Bundle size diffs
|
Description
PR is migration of the snaps test to POM
Related issues
Fixes:
#29894
Manual testing steps
Chrome browser execution commands below
yarn build:test:flask
yarn test:e2e:single test/e2e/snaps/test-snap-clientstatus.spec.ts --browser=chrome
yarn test:e2e:single test/e2e/snaps/test-snap-txinsights.spec.ts --browser=chrome
yarn test:e2e:single test/e2e/snaps/test-snap-installed.spec.ts --browser=chrome
yarn test:e2e:single test/e2e/snaps/test-snap-lifecycle.spec.ts --browser=chrome
yarn test:e2e:single test/e2e/snaps/test-snap-namelookup.spec.ts --browser=chrome
Firefox browser execution commands below
yarn build:test:flask:mv2
ENABLE_MV3=false yarn test:e2e:single test/e2e/snaps/test-snap-clientstatus.spec.ts --browser=firefox
ENABLE_MV3=false yarn test:e2e:single test/e2e/snaps/test-snap-txinsights.spec.ts --browser=firefox
ENABLE_MV3=false yarn test:e2e:single test/e2e/snaps/test-snap-installed.spec.ts --browser=firefox
ENABLE_MV3=false yarn test:e2e:single test/e2e/snaps/test-snap-lifecycle.spec.ts --browser=firefox
ENABLE_MV3=false yarn test:e2e:single test/e2e/snaps/test-snap-namelookup.spec.ts --browser=firefox
Pre-merge author checklist
Pre-merge reviewer checklist