-
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: NOTIFY-1256 - Extending E2E tests for Account Sync #28067
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. |
await accountListPage.check_numberOfAvailableAccounts( | ||
accountsSyncMockResponse.length, | ||
); | ||
await accountListPage.check_accountIsNotDisplayedInAccountList( |
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.
just out of curiosity, why do we have one method for "isNotDisplayed" and one method for "isDisplayed"? can not be implemented in a single method where it handles the exception in case it is not displayed and returns 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.
This is a good point actually. I just used the method as it was implemented. Also, from a lot of the other tests I have seen, it looks like the check_
methods are quite explicit.
@chloeYue might have some insight?
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.
Yes we try to make check_
methods explicit and easy to use, so we can call the check_
methods directly, and we don't need to assert in test body like assert.equal(accountListPage.check_accountIsDisplayedInAccountList(xxx), false);
…m:MetaMask/metamask-extension into NOTIFY-1256-e-2-e-tests-for-accounts-sync
Approved |
Builds ready [b22c776]
Page Load Metrics (2039 ± 130 ms)
Bundle size diffs
|
Builds ready [49e678d]
Page Load Metrics (1935 ± 69 ms)
Bundle size diffs
|
await driver.clickElement( | ||
'[data-testid="multichain-account-menu-popover-add-account"]', | ||
await accountListPage.check_accountDisplayedInAccountList( | ||
'My First Synced Account', |
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'd love to ditch these magic strings and use something like
import { SDK } from '@metamask/profile-sync-controller';
const decryptedAccountNames = await Promise.all(
accountsSyncMockResponse.map(async (response) => {
const decryptedAccountName = await SDK.Encryption.decryptString(
response.Data,
NOTIFICATIONS_TEAM_STORAGE_KEY,
);
return JSON.parse(decryptedAccountName).n;
}),
);
...
for (const accountName of decryptedAccountNames) {
await accountListPage.check_accountDisplayedInAccountList(
accountName,
);
}
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 would apply to all other occurences of magic strings in this PR. Wdyt?
I just did this for mobile and it works great. Something to be wary of, we'll be adding some decryption workload to our CI by doing that. I'm just not sure how much. Let's discuss!
Description
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist