Skip to content

feat: add QR sync SRP e2e inject path and SmokeAccounts specs#33238

Draft
grvgoel81 wants to merge 7 commits into
mainfrom
test/TO-810-qr-sync-single-srp-e2e
Draft

feat: add QR sync SRP e2e inject path and SmokeAccounts specs#33238
grvgoel81 wants to merge 7 commits into
mainfrom
test/TO-810-qr-sync-single-srp-e2e

Conversation

@grvgoel81

@grvgoel81 grvgoel81 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Description

Description

Adds Appium smoke coverage for mobile ↔ extension QR account sync (SRP) for new-user and existing-user flows.

Real MWP pairing is impractical in CI, so this PR adds a HAS_TEST_OVERRIDES-gated inject path:

  • Appium specs inject via the E2E command queue (applyQrSyncSyncReady) while the Add Device screen is mounted (more reliable than warm-session deep links). A deep-link bridge (e2e://qr-sync/apply-sync-ready?... / metamask://e2e/qr-sync/...) remains as a fallback.
  • Specs live under tests/smoke-appium/seedless/qr-sync-srp.spec.ts (SmokeSeedlessOnboarding) and cover new-user onboarding and existing-user “link extension” paths with addDeviceSyncEnabled mocked on.
  • Flow helpers wait with iOS-safe wallet-home readiness (waitForWalletHomePlaywright) after inject / onboarding.
  • Adds the testIDs / page objects needed for those screens. Existing-user assertions match current product behavior (account import lands back on wallet home).

Changelog

CHANGELOG entry: null

Related issues

Refs: https://consensyssoftware.atlassian.net/browse/TO-810, https://consensyssoftware.atlassian.net/browse/TO-881

Manual testing steps

Feature: QR sync SRP — mobile ↔ extension e2e tests

 Scenario: new user imports SRP from extension sync and lands on wallet home
    Given the app is on onboarding with addDeviceSyncEnabled mocked on
    And HAS_TEST_OVERRIDES is enabled in the e2e build
    When the user chooses Have an existing wallet and Import from extension
    And the test injects a sync-ready SRP payload via the e2e QR sync deep link
    And the user completes password creation and MetaMetrics opt-in
    Then the wallet home is visible
    And the account list shows one account

  Scenario: existing user links extension and imports one additional SRP
    Given the user is logged into an existing wallet with addDeviceSyncEnabled mocked on
    When the user opens Add wallet and chooses Import from extension
    And the test injects a sync-ready SRP payload via the e2e QR sync deep link
    Then the account list shows the additional imported SRP account

Screenshots/Recordings

Before

After

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Medium Risk
E2E bridge accepts plaintext mnemonics via deep links but is gated to test builds; it still touches wallet import state and must not ship enabled in production.

Overview
Adds an E2E-only path to exercise mobile↔extension QR account sync without MWP pairing, plus Detox/Appium smoke tests for new and existing users.

When HAS_TEST_OVERRIDES is on, QrSyncController.applyTestSyncReadyPayload seeds awaiting_password / pending SRP state as if sync-ready arrived; e2eBridgeQrSync listens for metamask://e2e/qr-sync/apply-sync-ready (and e2e://qr-sync/...) and is wired from qr-sync-controller-init. The shared E2E deep-link helper now maps per-scheme prefixes (including new QR_SYNC), not only perps.

UI test hooks: testIDs on Add Device to Wallet and the import-from-extension link. Tests: qr-sync.flow, page objects, and qr-sync-srp.spec (onboarding inject → password → wallet; logged-in add-wallet → second SRP).

Reviewed by Cursor Bugbot for commit 0fc0cf5. Configure here.

Add HAS_TEST_OVERRIDES sync-ready injection, page objects/flows, and new/existing user Detox smoke coverage for mobile↔extension QR account sync.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown
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.

@metamask-ci metamask-ci Bot added the team-onboarding Onboarding team label Jul 14, 2026
@metamask-ci

metamask-ci Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Description section is empty. Describe what changed and why.
  • Screenshots/Recordings section is empty. Add an image/video for user-facing changes, logs/console output for non-user-facing changes, or write N/A if no evidence is applicable.
  • Pre-merge author checklist has unchecked items (e.g. "I've tested on Android"). Every box must be consciously checked — see docs/readme/ready-for-review.md.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0fc0cf5. Configure here.

Comment thread app/core/QrSync/e2eBridgeQrSync.ts
@codecov-commenter

codecov-commenter commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.65217% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.41%. Comparing base (c4cb337) to head (ff12977).
⚠️ Report is 57 commits behind head on main.

Files with missing lines Patch % Lines
app/core/QrSync/e2eBridgeQrSync.ts 96.96% 1 Missing and 1 partial ⚠️
app/components/Views/AddDeviceToWallet/index.tsx 0.00% 1 Missing ⚠️
...ents/Views/ImportFromSecretRecoveryPhrase/index.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #33238      +/-   ##
==========================================
+ Coverage   84.37%   84.41%   +0.04%     
==========================================
  Files        6127     6136       +9     
  Lines      163530   163894     +364     
  Branches    39887    40012     +125     
==========================================
+ Hits       137974   138351     +377     
- Misses      16050    16051       +1     
+ Partials     9506     9492      -14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added size-XL and removed size-L labels Jul 14, 2026
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/components/Views/AddDeviceToWallet/index.test.tsx 0/122 0/167 0/374
app/components/Views/ImportFromSecretRecoveryPhrase/index.test.tsx 0/122 0/167 0/374
app/core/Engine/controllers/qr-sync-controller-init.test.ts 0/122 0/167 0/374
app/core/QrSync/e2eBridgeQrSync.test.ts 0/122 0/167 0/374
app/util/test/e2eCommandPolling.test.ts 0/122 0/167 0/374

AI-detected flaky patterns

app/components/Views/AddDeviceToWallet/index.test.tsx

  • J7 — Non-deterministic data: Date.now() (medium)
    • Date.now() is called live at test-execution time to compute the OTP deadline. If the component under test evaluates whether the OTP has expired by comparing deadline to the current time, a slow CI machine could cause the deadline to appear expired before the assertion runs, making the test intermittently fail. Pin the system clock with jest.useFakeTimers() / jest.setSystemTime() to make the value deterministic. The same pattern appears in the second DISPLAYING_OTP test at line ~242.
    • Suggested fix in app/components/Views/AddDeviceToWallet/index.test.tsx:228:
      -      renderComponent({
      -        phase: QrSyncPhases.DISPLAYING_OTP,
      -        otp: { otp: '123456', deadline: Date.now() + 30_000 },
      -      });
      +      jest.useFakeTimers();
      +      jest.setSystemTime(new Date('2024-01-01T12:00:00.000Z'));
      +
      +      renderComponent({
      +        phase: QrSyncPhases.DISPLAYING_OTP,
      +        otp: { otp: '123456', deadline: Date.now() + 30_000 },
      +      });
      +
      +      // ... assertions ...
      +
      +      jest.useRealTimers();

app/components/Views/ImportFromSecretRecoveryPhrase/index.test.tsx

  • J10 — jest.spyOn without restoreAllMocks() (medium)
    • Throughout the file, many tests create spies with jest.spyOn (e.g., jest.spyOn(Authentication, 'componentAuthenticationType'), jest.spyOn(Authentication, 'newWalletAndRestore'), jest.spyOn(Authentication, 'getType'), jest.spyOn(StorageWrapper, 'getItem'), jest.spyOn(Alert, 'alert')) but the outer afterEach only calls jest.clearAllMocks(). clearAllMocks resets call counts but does NOT restore the original implementations replaced by spyOn. This means a spy installed in one test (e.g., mockRejectedValueOnce) can leak its replaced implementation into subsequent tests, causing order-dependent failures. The onQrCodePress nested describe correctly calls navigationSpy.mockRestore() in its own afterEach, but the outer describe needs jest.restoreAllMocks() to cover all other spies.
    • Suggested fix in app/components/Views/ImportFromSecretRecoveryPhrase/index.test.tsx:1:
      -  afterEach(() => {
      -    jest.clearAllMocks();
      -  });
      -  beforeEach(() => {
      -    jest.clearAllMocks();
      -    // ...
      -  });
      +  afterEach(() => {
      +    jest.clearAllMocks();
      +    jest.restoreAllMocks();
      +  });

app/core/Engine/controllers/qr-sync-controller-init.test.ts

  • J10 — jest.spyOn without restoreAllMocks() (medium)
    • A jest.spyOn on initRequestMock.controllerMessenger.registerActionHandler is created inside the test body with no corresponding mockRestore() call and no jest.restoreAllMocks() in afterEach. Although jest.clearAllMocks() is called in beforeEach, that only clears call counts — it does NOT restore the original implementation. If the spy replaces the real method, subsequent tests that rely on the real registerActionHandler behavior will see the spy instead. Add jest.restoreAllMocks() to afterEach to guarantee cleanup.
    • Suggested fix in app/core/Engine/controllers/qr-sync-controller-init.test.ts:107:
      -  it('registers provisioning mutation action handlers on the controller messenger', () => {
      -    const registerSpy = jest.spyOn(
      -      initRequestMock.controllerMessenger,
      -      'registerActionHandler',
      -    );
      -
      -    qrSyncControllerInit(initRequestMock);
      -
      -    expect(registerSpy).toHaveBeenCalledWith(
      -      'QrSyncController:importRemainingSecrets',
      -      expect.any(Function),
      -    );
      -    // ...
      -  });
      +  afterEach(() => {
      +    jest.restoreAllMocks();
      +  });
      +
      +  it('registers provisioning mutation action handlers on the controller messenger', () => {
      +    const registerSpy = jest.spyOn(
      +      initRequestMock.controllerMessenger,
      +      'registerActionHandler',
      +    );
      +
      +    qrSyncControllerInit(initRequestMock);
      +
      +    expect(registerSpy).toHaveBeenCalledWith(
      +      'QrSyncController:importRemainingSecrets',
      +      expect.any(Function),
      +    );
      +    // ...
      +  });
  • J7 — Non-deterministic data: Date.now() (medium)
    • Date.now() is called at test-execution time to compute the deadline field. If the production code under test compares this deadline against the current time (e.g., to check expiry), the result can differ depending on how long the test takes to run, making the assertion non-deterministic under load. Pin the clock with jest.useFakeTimers() / jest.setSystemTime() so the value is stable across all runs.
    • Suggested fix in app/core/Engine/controllers/qr-sync-controller-init.test.ts:80:
      -    const persistedState: QrSyncControllerState = {
      -      ...defaultQrSyncControllerState,
      -      phase: QrSyncPhases.DISPLAYING_OTP,
      -      otp: { otp: '123456', deadline: Date.now() + 30_000 },
      -    };
      +    jest.useFakeTimers();
      +    jest.setSystemTime(new Date('2024-01-01T12:00:00.000Z'));
      +
      +    const persistedState: QrSyncControllerState = {
      +      ...defaultQrSyncControllerState,
      +      phase: QrSyncPhases.DISPLAYING_OTP,
      +      otp: { otp: '123456', deadline: Date.now() + 30_000 },
      +    };
      +
      +    // ... rest of test ...
      +
      +    jest.useRealTimers();

app/core/QrSync/e2eBridgeQrSync.test.ts

  • J10 — jest.spyOn without restoreAllMocks() (medium)
    • jest.spyOn(Linking, 'getInitialURL') is called inside a test body with no mockRestore() call and no jest.restoreAllMocks() in afterEach. The beforeEach only calls mockClear() on individual mocks, not restoreAllMocks(). If this spy leaks into the next test, Linking.getInitialURL will still return the mocked resolved value instead of the mockGetInitialURL mock set up at module level, causing the 'swallows getInitialURL rejection' test to behave incorrectly. Add jest.restoreAllMocks() to afterEach.
    • Suggested fix in app/core/QrSync/e2eBridgeQrSync.test.ts:248:
      -  it('applies the initial Linking url when present', async () => {
      -    const applyTestSyncReadyPayload = jest.fn();
      -    jest
      -      .spyOn(Linking, 'getInitialURL')
      -      .mockResolvedValue(
      -        'metamask://e2e/qr-sync/apply-sync-ready?mnemonic=initial',
      -      );
      -
      -    registerE2EQrSyncDeepLinkHandler(() =>
      -      buildController(applyTestSyncReadyPayload),
      -    );
      -
      -    await Promise.resolve();
      -    await Promise.resolve();
      -
      -    expect(applyTestSyncReadyPayload).toHaveBeenCalledWith({
      -      mnemonic: 'initial',
      -      isPrimary: true,
      -      walletName: undefined,
      -      accountName: undefined,
      -    });
      -  });
      +  afterEach(() => {
      +    jest.restoreAllMocks();
      +  });
      +
      +  beforeEach(() => {
      +    mockUrlListeners.length = 0;
      +    mockAddEventListener.mockClear();
      +    mockGetInitialURL.mockReset().mockResolvedValue(null);
      +    jest.mocked(Logger.log).mockClear();
      +    jest.mocked(Logger.error).mockClear();
      +    __resetE2EQrSyncBridgeForTests();
      +  });
  • J6 — Arbitrary sleep used as synchronization barrier (high)
    • Two consecutive await Promise.resolve() calls are used as a microtask-flush barrier to let the getInitialURL promise chain settle before asserting. This is a fragile synchronization technique: if the production code adds another async hop (e.g., an extra await), the flush count becomes insufficient and the assertion races the async work, causing intermittent failures. The same pattern appears in the 'swallows getInitialURL rejection' test. Replace with waitFor(() => expect(...)) so the assertion retries until the condition is met.
    • Suggested fix in app/core/QrSync/e2eBridgeQrSync.test.ts:261:
      -    await Promise.resolve();
      -    await Promise.resolve();
      -
      -    expect(applyTestSyncReadyPayload).toHaveBeenCalledWith({
      -      mnemonic: 'initial',
      -      isPrimary: true,
      -      walletName: undefined,
      -      accountName: undefined,
      -    });
      +    registerE2EQrSyncDeepLinkHandler(() =>
      +      buildController(applyTestSyncReadyPayload),
      +    );
      +
      +    await waitFor(() => {
      +      expect(applyTestSyncReadyPayload).toHaveBeenCalledWith({
      +        mnemonic: 'initial',
      +        isPrimary: true,
      +        walletName: undefined,
      +        accountName: undefined,
      +      });
      +    });

app/util/test/e2eCommandPolling.test.ts

  • J6 — Arbitrary sleep used as synchronization barrier (high)
    • The flushMicrotasks helper loops await Promise.resolve() a fixed number of times (default 10) to drain the microtask queue after jest.advanceTimersByTime(). This is a fragile synchronization barrier: if the production code under test adds an extra async hop, the fixed count of 10 may not be enough to fully drain the queue, causing assertions to run before async work completes. Under CI load this is especially unreliable. Since fake timers are already in use, the correct approach is to use jest.runAllTimersAsync() inside act() to advance both timers and microtasks atomically, or to restructure so that waitFor is used after restoring real timers.
    • Suggested fix in app/util/test/e2eCommandPolling.test.ts:44:
      -async function flushMicrotasks(count = 10) {
      -  for (let i = 0; i < count; i++) {
      -    await Promise.resolve();
      -  }
      -}
      +// Replace flushMicrotasks() calls with jest.runAllTimersAsync() inside act():
      +// Before:
      +//   jest.advanceTimersByTime(0);
      +//   await flushMicrotasks();
      +// After:
      +//   await act(async () => { await jest.runAllTimersAsync(); });
      +
      +// Remove the flushMicrotasks helper entirely and update each call site, e.g.:
      +it('dispatches export-state command to handleExportStateCommand', async () => {
      +  mockHasTestOverrides = true;
      +  const { startE2ECommandPolling } = loadModule();
      +
      +  mockedAxios.get.mockResolvedValueOnce(probeResponse).mockResolvedValueOnce({
      +    status: 200,
      +    data: {
      +      queue: [{ type: 'export-state', args: {} }],
      +    },
      +  } as AxiosResponse);
      +
      +  await startE2ECommandPolling();
      +
      +  await act(async () => { await jest.runAllTimersAsync(); });
      +
      +  expect(handleExportStateCommand).toHaveBeenCalled();
      +});
  • J8 — jest.useFakeTimers() combined with waitFor (high)
    • Fake timers are activated globally in beforeEach for the entire suite. The flushMicrotasks helper (which uses await Promise.resolve() in a loop) is used as a substitute for waitFor, but if any future test in this file were to use waitFor, it would hang indefinitely because waitFor internally polls via real setTimeout which is frozen under fake timers. Even without explicit waitFor calls today, the combination of jest.useFakeTimers() + microtask-flush barriers is a known fragile pattern (J8/J6 overlap). The safest fix is to use jest.runAllTimersAsync() inside act() to advance both timers and microtasks together, eliminating the need for the manual flush loop.
    • Suggested fix in app/util/test/e2eCommandPolling.test.ts:63:
      -  beforeEach(() => {
      -    jest.useFakeTimers();
      -    jest.clearAllMocks();
      -    mockHasTestOverrides = false;
      -  });
      -
      -  afterEach(() => {
      -    jest.useRealTimers();
      -  });
      +  beforeEach(() => {
      +    jest.useFakeTimers();
      +    jest.clearAllMocks();
      +    mockHasTestOverrides = false;
      +  });
      +
      +  afterEach(() => {
      +    jest.useRealTimers();
      +  });
      +
      +  // Replace all:
      +  //   jest.advanceTimersByTime(N);
      +  //   await flushMicrotasks();
      +  // with:
      +  //   await act(async () => { await jest.runAllTimersAsync(); });
      +  // This advances timers AND drains microtasks atomically.

This check is informational only and does not block merging.

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeAccounts, SmokeSeedlessOnboarding, SmokeWalletPlatform
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: medium
  • AI Confidence: 90%
click to see 🤖 AI reasoning details

E2E Test Selection:

The PR introduces QR sync SRP import functionality for E2E testing, with the following key changes:

  1. SmokeAccounts (direct): tests/smoke/accounts/qr-sync-srp.spec.ts is a new smoke spec explicitly tagged with SmokeAccounts. It tests QR sync SRP import for both new users (onboarding path) and existing users (add wallet path). The spec exercises multi-SRP architecture — importing additional SRPs via QR sync from extension.

  2. SmokeSeedlessOnboarding (direct): tests/smoke-appium/seedless/qr-sync-srp.spec.ts is a new Appium smoke spec explicitly tagged with SmokeSeedlessOnboarding. It tests the same QR sync SRP flows via Appium/Playwright framework.

  3. SmokeWalletPlatform (per tag description): The SmokeAccounts tag description explicitly states "When changes touch multi-SRP architecture, account list, SRP export, or sync flows, also select SmokeWalletPlatform." This PR directly touches multi-SRP architecture (importing additional SRPs via QR sync).

App code changes analyzed:

  • QrSyncController.ts: Added applyTestSyncReadyPayload() E2E-only method (gated by HAS_TEST_OVERRIDES)
  • e2eBridgeQrSync.ts: New E2E bridge for QR sync deep link handling (E2E-only, gated by HAS_TEST_OVERRIDES)
  • qr-sync-controller-init.ts: Registers E2E deep link handler when HAS_TEST_OVERRIDES=true
  • AddDeviceToWallet/index.tsx: Added testIDs to UI elements (non-functional change)
  • ImportFromSecretRecoveryPhrase/index.js: Refactored "import from extension" link to use TouchableOpacity with testID — this is a structural change to the onboarding import screen that could affect existing import flows

Test infrastructure changes:

  • DeepLink.ts: Fixed a bug where ALL E2E schemes were mapped to metamask://e2e/perps/. Now correctly maps each scheme. The PERPS mapping is preserved (metamask://e2e/perps/), so existing PERPS tests are unaffected.
  • Constants.ts: Added QR_SYNC to E2EDeeplinkSchemes enum
  • types.ts: Added applyQrSyncSyncReady to E2ECommandTypes enum
  • New page objects: AddDeviceToWalletView.ts, AddWalletView.ts
  • Updated page object: ImportWalletView.ts (added importFromExtensionLink)

No other smoke tags are impacted — the changes are scoped to QR sync/SRP import flows, onboarding, and account management. No confirmations, swaps, browser, snaps, or network changes are involved.

Performance Test Selection:
The changes are focused on E2E test infrastructure for QR sync SRP import (new E2E-only methods gated by HAS_TEST_OVERRIDES, new test flows, page objects, and deep link handling). The app code changes are either E2E-only (gated by HAS_TEST_OVERRIDES) or minor UI additions (testIDs). There are no changes to performance-sensitive flows like app launch, login, onboarding rendering, asset loading, or swap execution. No performance spec files were modified.

View GitHub Actions results

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants