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: Assert accounts are removed with snap removal in existing test #24442
test: Assert accounts are removed with snap removal in existing test #24442
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #24442 +/- ##
========================================
Coverage 67.37% 67.37%
========================================
Files 1289 1289
Lines 50220 50220
Branches 13005 13005
========================================
Hits 33835 33835
Misses 16385 16385 ☔ View full report in Codecov by Sentry. |
Builds ready [638b002]
Page Load Metrics (359 ± 419 ms)
Bundle size diffs
|
Could we include more assertions based on the specific account that was added? Otherwise, it looks good. |
Thank you, @hjetpoluru! Yes, we could. Are you suggesting to add assertions to confirm that the created account address is what we expect? Bear in mind the snap simple keyring (SSK) does create a unique private key on any new account creation. In these tests we are not expecting a specific account address, rather just some public address derived from the private key that was created. Verifying the address that was added in the accounts list would certainly be beneficial if we were testing the import of a known private key through SSK, but here we are a creating new random private key. |
Yes, I totally agree with you that the address is dynamically created, and it's validated in the creation of the account in another test. |
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.
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!
Builds ready [40bcb35]
Page Load Metrics (1181 ± 660 ms)
Bundle size diffs
|
Builds ready [4bfd166]
Page Load Metrics (1916 ± 609 ms)
Bundle size diffs
|
Merge this PR as Mike's OOO. |
Description
Updating existing test to confirm that accounts are removed when the snap is removed
Related issues
Fixes: https://github.com/MetaMask/accounts-planning/issues/439
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist