feat: Add E2E test for storage operations failure recovery#39317
Merged
itsyoboieltr merged 12 commits intomainfrom Jan 22, 2026
Merged
feat: Add E2E test for storage operations failure recovery#39317itsyoboieltr merged 12 commits intomainfrom
itsyoboieltr merged 12 commits intomainfrom
Conversation
Contributor
|
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. |
gauthierpetetin
commented
Jan 16, 2026
|
|
||
| return accountAddress; | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
These are all moved to helpers.ts file
gauthierpetetin
commented
Jan 16, 2026
| getConfig(this.test?.title), | ||
| async ({ driver }: { driver: Driver }) => { | ||
| const initialFirstAddress = await onboardThenCorruptVault( | ||
| const initialFirstAddress = await onboardThenTriggerCorruption( |
Contributor
Author
There was a problem hiding this comment.
onboardThenCorruptVault has just been renamed into onboardThenTriggerCorruption to have a more generic name that's more compatible with new E2E test scenarios we're adding in this PR (we're not specifically corrupting the vault, we're corrupting/breaking the database).
Contributor
Builds ready [d098277]
UI Startup Metrics (1286 ± 113 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Contributor
Builds ready [801f437]
UI Startup Metrics (1255 ± 99 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Contributor
Builds ready [d256427]
UI Startup Metrics (1272 ± 104 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
itsyoboieltr
previously approved these changes
Jan 20, 2026
seaona
reviewed
Jan 20, 2026
| await homePage.checkPageIsLoaded(); | ||
|
|
||
| // The toast should appear because writes are failing | ||
| await driver.waitForSelector('[data-testid="storage-error-toast"]'); |
Member
There was a problem hiding this comment.
we should avoid using selectors directly in a spec file. Rather, we should create a method in the corresponding page object class, and then call it from here
Contributor
Author
There was a problem hiding this comment.
thanks for the feedback, it's now updated in this commit: 76903ac
seaona
reviewed
Jan 20, 2026
| // Click the toast action button to navigate to reveal SRP page | ||
| // The button text is inside a nested span, so we target the span's text | ||
| await driver.clickElement({ | ||
| text: 'Back up Secret Recovery Phrase', |
seaona
reviewed
Jan 20, 2026
| }); | ||
|
|
||
| // Verify we navigated to the reveal SRP page (password prompt) | ||
| await driver.waitForSelector('[data-testid="input-password"]'); |
Move vault recovery-specific selectors and methods from helpers.ts to a new VaultRecoveryPage class that extends CriticalErrorPage, following the page object pattern with separation of concerns by feature. Changes: - Create VaultRecoveryPage with recovery button methods - Revert CriticalErrorPage to base error page functionality - Change CriticalErrorPage fields from private to protected for inheritance - Update helpers.ts to use VaultRecoveryPage
Contributor
✨ Files requiring CODEOWNER review ✨🧪 @MetaMask/qa (4 files, +249 -10)
|
Contributor
Builds ready [0205518]
UI Startup Metrics (1287 ± 101 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
itsyoboieltr
previously approved these changes
Jan 21, 2026
seaona
reviewed
Jan 21, 2026
Member
seaona
left a comment
There was a problem hiding this comment.
hi there 👋 thanks for the changes! Looking good! I added a couple of more comments in regards to stick withthe page object standard practices using flows🙏
Let me know if this makes sense or have any questions!
| password: WALLET_PASSWORD, | ||
| skipSRPBackup: true, | ||
| }); | ||
| } |
Member
There was a problem hiding this comment.
instead of having a helper function which just calls a flow, can we call the flow directly in the spec file?
In our spec files we should try to use page objects methods or flows (under flow folder). Adding extra layers on top can make it harded to debug, when we are investigating flakiness etc.
The general POM guidelines for performing test actions are eitherto work with:
- page objects
- flows
So we could remove this wrapper. Wdyt?
Contributor
Author
There was a problem hiding this comment.
Makes total sense, it's updated in this commit: 4a60f05
| }); | ||
|
|
||
| // now that we are re-onboarded, get the first account's address | ||
| return await getFirstAddress(driver); |
Member
There was a problem hiding this comment.
same here, could we omit this extra layer onboardAfterRecovery and call the inside flows in the spec file?
Contributor
Author
There was a problem hiding this comment.
Same here, updated in this commit: 4a60f05
| */ | ||
| async function waitForVaultRestorePage(driver: Driver): Promise<void> { | ||
| const vaultRecoveryPage = new VaultRecoveryPage(driver); | ||
| await vaultRecoveryPage.waitForPageAfterExtensionReload(); |
Contributor
Author
There was a problem hiding this comment.
Same here, updated in this commit: 4a60f05
| * background page for MV2 or offscreen page for MV3. | ||
| * @returns The initial first account's address. | ||
| */ | ||
| export async function onboardThenTriggerCorruption( |
Member
There was a problem hiding this comment.
could we place this under the page-objects/flow folder? to follow our POM guidelines -> if there are actions which need multiple pages, and it's re-used in multiple places, we can create a flow for that
Contributor
Author
There was a problem hiding this comment.
Sure, it's now moved to the flow folder in this commit: e560fb0
| * @param options.driver - The WebDriver instance. | ||
| * @param options.confirm - Whether to confirm the action or not. | ||
| */ | ||
| export async function clickRecover({ |
Member
There was a problem hiding this comment.
this abstraction can also be removed and instead, call the actions from the spec file
Contributor
Author
There was a problem hiding this comment.
Same here, updated in this commit: 4a60f05
| * @param driver - The WebDriver instance. | ||
| * @param headerNavbar | ||
| */ | ||
| export async function getFirstAddress( |
Member
There was a problem hiding this comment.
it seems like we need a flow here as well
Contributor
Author
There was a problem hiding this comment.
Same here, moved to the flow folder in this commit: e560fb0
Remove wrapper functions that just call flows/page objects directly, following POM guidelines to reduce indirection and improve debuggability. Removed wrappers: - onboard() - now call completeCreateNewWalletOnboardingFlow directly - clickRecover() - now call vaultRecoveryPage.clickRecoveryButton directly - onboardAfterRecovery() - now call flow + getFirstAddress directly - waitForVaultRestorePage() - inlined in onboardThenTriggerCorruption Spec files now import and use flows/page objects directly.
Move multi-page orchestration functions to page-objects/flows following POM guidelines for reusable flows that span multiple pages. Changes: - Create vault-corruption.flow.ts with getFirstAddress and onboardThenTriggerCorruptionFlow - Reduce helpers.ts to only contain getConfig test configuration - Update spec files to import from the new flow file
Contributor
Builds ready [69d22bb]
UI Startup Metrics (1304 ± 117 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
| * | ||
| * @throws Error if simulating storage failure for testing | ||
| */ | ||
| #maybeSimulateSetFailure(): void { |
Contributor
There was a problem hiding this comment.
Why is the set failure in its own function but not the get failure? This is not a request for change in either direction, just curious.
Contributor
Author
There was a problem hiding this comment.
The get failure is only called in one method (get).
The set failure is called from two different methods (set and persist), so extracting logic to a separate function (maybeSimulateSetFailure) avoids code duplication.
itsyoboieltr
approved these changes
Jan 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR is a follow-up PR for #39010
It adds E2E tests
Changelog
CHANGELOG entry: null
Related issues
Fixes: #10091
Manual testing steps
Screenshots/Recordings
NA
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Adds targeted E2E tests to validate recovery behavior when
browser.storage.localoperations fail, plus supporting test hooks and small UI/test utilities.storage-operations-failure.spec.tsverifies vault recovery on simulatedget()failure and storage error toast on simulatedset()failure; refactorsvault-corruption.spec.tsto reuse shared flowstesting.simulateStorageGetFailureandtesting.simulateStorageSetFailureinmanifestFlags.ts#getFromLocalStore,#setInLocalStore,#setKeyValuesInLocalStorewith optional failure simulation; integrates intoget,set, andpersistVaultRecoveryPage, sharedvault-corruption.flow.ts, and helpers; updatesCriticalErrorPagevisibility for reusedata-testid="storage-error-toast"to the storage error toast and page object methods to assert and act on itWritten by Cursor Bugbot for commit 69d22bb. This will update automatically on new commits. Configure here.