Skip to content

feat: finish adding recovery phrase e2e test#548

Merged
dewabisma merged 20 commits into
mainfrom
beast/e2e-show-secret-phrase
Jul 2, 2026
Merged

feat: finish adding recovery phrase e2e test#548
dewabisma merged 20 commits into
mainfrom
beast/e2e-show-secret-phrase

Conversation

@dewabisma

@dewabisma dewabisma commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add test flow for viewing recovery phrase.


Note

Low Risk
Changes are limited to E2E keys, test helpers, and a new Patrol test; production recovery-phrase and auth behavior is unchanged aside from test-only widget keys.

Overview
Adds a Patrol E2E test that creates a wallet, opens Settings → Wallet → Recovery phrase, passes the caution screen, and asserts the phrase can be tapped to reveal.

To drive that flow reliably, the PR extends E2EKeys and mirrors them in Selectors, then attaches Keys on the home settings button, settings hub (wallet row), wallet settings screen, recovery phrase confirmation/screen scaffolds, the caution Continue button (via new optional continueButtonKey on SettingsCautionScaffold), and the recovery phrase reveal tap target / revealed hint in RecoveryPhraseBody.

WalletFlows.createFromWelcome is extracted so create_wallet_test and the new view_recovery_phrase_test share the same onboarding steps.

Reviewed by Cursor Bugbot for commit 9c2017c. Configure here.

@dewabisma dewabisma requested a review from n13 July 1, 2026 12:16

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: finish adding recovery phrase e2e test

Verdict: Approve with comments — clean, low-risk, test-only PR. All 10 selectors used by the new test are correctly wired to real widget keys (each attached exactly once), the reveal/continue/caution logic is sound, flutter analyze is clean, and Bugbot passed. Reviewed in an isolated worktree off the head commit (incremental diff over #547 only).

Should-fix (non-blocking)

Dropped assertion during extractionmobile-app/patrol_test/support/wallet_flows.dart:13-22. WalletFlows.createFromWelcome omits the original expect($('Account 1'), findsOneWidget) that create_wallet_test asserted between waiting for and tapping the done button — minor lost coverage. Re-add it after the accountReadyDoneButton wait (the file already imports flutter_test); ideally key the account-name widget to avoid the locale-fragile text finder.

Nits

  • recovery_phrase_confirmation_screen.dart:51 / recovery_phrase_screen.dart:47 wrap the child in KeyedSubtree(key:). Correct and no remount/state-loss risk (stable const key), but you could pass key: straight to the component to match how sibling screens key ScaffoldBase(key:) directly (settings_screen.dart:39, wallet_settings_screen.dart:61). Purely stylistic.
  • The test assumes no biometrics enrolled on the device: Continue → LocalAuthService().authenticate() returns true early only when biometrics are unavailable; with enrolled biometrics a native prompt would hang the test (it never calls $.native…). Pre-existing prod behavior, not introduced here — worth documenting the "no enrolled biometrics" assumption in the E2E setup.

Nice

  • E2EKeys (String consts in lib/) shared by prod widgets and Selectors (Key consts in test) prevents selector/app drift; new keys follow the existing naming/ordering.
  • Good DRY: createFromWelcome shared by two tests, mirrors the existing importFromWelcome.
  • Minimal, behavior-neutral production edits (key attachments + one optional null-safe continueButtonKey, correctly plumbed through _SettingsCautionBottom, no collision with the walletReset caution flow).
  • The reveal key is attached only to the revealed hint, so expect(recoveryPhraseRevealed, findsOneWidget) is satisfied only after the tap — precise, no false positives.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — recovery phrase E2E

Reviewed the incremental (stacked-on-#547) diff in an isolated worktree with flutter analyze (clean). Low-risk and test-only: inert Keys plus one optional continueButtonKey param, with no behavioral changes for existing callers. I verified all 10 selectors used by view_recovery_phrase_test map to real attached keys (each attached exactly once), and the reveal/continue/caution flow is correct — the reveal key is attached only to the revealed hint, so the tap->assert is unambiguous.

Findings

  • [Low] The extraction into WalletFlows.createFromWelcome silently dropped the original expect($('Account 1'), findsOneWidget) assertion that create_wallet_test used to make. Minor lost coverage (the flow still asserts landing on homeScreen). Recommend restoring it — ideally via a Key on the account-name widget to avoid the locale-fragile text finder.
  • [Low] The test assumes no enrolled biometrics on the device: Continue -> LocalAuthService().authenticate() returns true early only when biometrics are unavailable; with biometrics enrolled a native prompt appears and the test hangs (it never drives $.native...). Pre-existing production behavior — worth documenting the "no enrolled biometrics" assumption in the E2E setup.
  • [Nit] The KeyedSubtree(key:) wrapping in recovery_phrase_confirmation_screen / recovery_phrase_screen is correct and carries no remount/state-loss risk, but you could pass key: straight to the component widgets (as the sibling screens do with ScaffoldBase(key:)) and drop the wrapper. Purely stylistic.

Positives

  • Clean anti-drift indirection: E2EKeys (strings in lib/) are shared by both the widgets and Selectors (keys in test), so they can't diverge.
  • Good DRY: createFromWelcome is shared by two tests and mirrors the existing importFromWelcome.
  • continueButtonKey is plumbed correctly with no collisions; reveal toggle logic is sound.

Verdict

Approve with comments. Clean, analyze-clean, correctly-wired test-only PR. Only the dropped Account 1 assertion is worth restoring before/at merge.

@dewabisma dewabisma changed the base branch from beast/e2e-regular-send-flow to main July 2, 2026 15:09
@dewabisma dewabisma merged commit 3cadcb4 into main Jul 2, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants