-
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: [POM] Migrate onboarding infura call privacy e2e tests #28079
base: develop
Are you sure you want to change the base?
Conversation
…metamask-extension into pom-migrate-onboarding
Removed test files that were migrated to TypeScript: - test/e2e/tests/privacy/basic-functionality.spec.js - test/e2e/tests/privacy/onboarding-privacy.spec.js
…k/metamask-extension into chloe-migrate-onboarding # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…k/metamask-extension into chloe-migrate-onboarding
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 [390d5be]
Page Load Metrics (2066 ± 98 ms)
Bundle size diffs
|
Builds ready [5622dd4]
Page Load Metrics (2100 ± 88 ms)
Bundle size diffs
|
Builds ready [9ffb554]
Page Load Metrics (2018 ± 106 ms)
Bundle size diffs
|
Builds ready [7d7e571]
Page Load Metrics (2094 ± 179 ms)
Bundle size diffs
|
export const importSRPOnboardingFlow = async (driver: Driver) => { | ||
console.log('start import srp onboarding flow '); | ||
export const createNewWalletOnboardingFlow = async (driver: Driver) => { | ||
console.log('start creat new wallet onboarding flow '); |
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.
minor nit
console.log('start creat new wallet onboarding flow '); | |
console.log("Starting the creation of a new wallet onboarding flow"); |
export const completeCreateNewWalletOnboardingFlow = async (driver: Driver) => { | ||
console.log('start to complete create new wallet onboarding flow '); | ||
export const importSRPOnboardingFlow = async (driver: Driver) => { | ||
console.log('start import srp onboarding flow '); |
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.
minor nit
console.log('start import srp onboarding flow '); | |
console.log("Starting the import of SRP onboarding flow"); |
async clickToggleButton(): Promise<void> { | ||
console.log('Click Toggle Button'); | ||
async toggleShowTestNetwork(): Promise<void> { | ||
console.log('Toggle show test network in select network dialog'); |
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.
thanks for correcting. Nice job
export const switchToNetworkFlow = async ( | ||
driver: Driver, | ||
networkName: string, | ||
toogleShowTestNetwork: 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.
minor nit - typo here
toogleShowTestNetwork: boolean = false, | |
toggleShowTestNetwork: boolean = false, |
* | ||
* @param driver | ||
* @param networkName - The name of the network to switch to. | ||
* @param toogleShowTestNetwork - A boolean indicating whether to toggle the display of test networks. Defaults to 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.
minor typo
* @param toogleShowTestNetwork - A boolean indicating whether to toggle the display of test networks. Defaults to false. | |
* @param toggleShowTestNetwork - A boolean indicating whether to toggle the display of test networks. Defaults to false. |
|
||
const selectNetworkDialog = new SelectNetwork(driver); | ||
await selectNetworkDialog.check_pageIsLoaded(); | ||
if (toogleShowTestNetwork) { |
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.
if (toogleShowTestNetwork) { | |
if (toggleShowTestNetwork) { |
@@ -16,4 +36,4 @@ class networkSwitchModalConfirmation { | |||
} | |||
} | |||
|
|||
export default networkSwitchModalConfirmation; | |||
export default NetworkSwitchModalConfirmation; |
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.
Thanks @chloeYue for correcting and adding the page check that I missed.
|
||
export const completeCreateNewWalletOnboardingFlow = async (driver: Driver) => { | ||
console.log('start to complete create new wallet onboarding flow '); |
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('start to complete create new wallet onboarding flow '); | |
console.log('Starting to complete the new wallet onboarding flow'); |
@chloeYue, Everything is looking good, just some minor typos. Logging has improved a lot which helps when fixing the flaky tests. Great effort! |
await this.driver.clickElement(this.lockMetaMaskButton); | ||
} | ||
|
||
async openAccountMenu(): Promise<void> { | ||
await this.driver.clickElement(this.accountMenuButton); | ||
} | ||
|
||
async openAccountOptionMenu(): Promise<void> { | ||
console.log('Open account options menu'); | ||
await this.driver.clickElement(this.accountOptionMenu); |
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.
accountOptionMenu
is slightly confusing as there is also accountMenuButton
.The main issue is that the data-testid that was defined is itself incorrectly named, as shown below. Correcting this would be a huge amount of work.
private readonly accountMenuButton = '[data-testid="account-menu-icon"]';
private readonly accountOptionMenu =
'[data-testid="account-options-menu-button"]';
I am thinking that adding toggleThreeDotMenu
, openThreeDotMenu
, or showThreeDotMenu
would be easier, but I leave it to you to decide as you have worked on this more.
|
||
// Validate the default network is Localhost 8545 | ||
await headerNavbar.check_currentSelectedNetwork('Localhost 8545'); | ||
await new HeaderNavbar(driver).check_currentSelectedNetwork( |
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.
Could you explain the reason for changing this line? It's currently used only once, but if someone needs it for reference and there are more calls, it could be easily missed. This brings up the question of whether clean code practices are needed. I will leave it to you to decide.
@@ -18,7 +18,7 @@ import SnapSimpleKeyringPage from '../../page-objects/pages/snap-simple-keyring- | |||
import TestDapp from '../../page-objects/pages/test-dapp'; | |||
|
|||
describe('Snap Account Signatures @no-mmi', function (this: Suite) { | |||
this.timeout(150000); // This test is very long, so we need an unusually high timeout | |||
this.timeout(200000); // This test is very long, so we need an unusually high timeout |
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.
Could you explain why this timeout is needed in general? I see that this is a multiple test scenario in a single test, and it being flaky irrespective of the timeout itself. Could we enhance this in another way, or should it be addressed in a separate ticket then incase then this change made not be needed is my thought.
Description
openAccountOptionMenu
method to reduce flakinessRelated issues
Fixes: #28080
Manual testing steps
Check code readability, make sure tests pass.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist