Skip to content

Patrol Framework added for E2E test setup#542

Open
dewabisma wants to merge 8 commits into
mainfrom
beast/e2e-test-setup
Open

Patrol Framework added for E2E test setup#542
dewabisma wants to merge 8 commits into
mainfrom
beast/e2e-test-setup

Conversation

@dewabisma

@dewabisma dewabisma commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Added patrol test setup
  • Added runner ui test target to ios
  • Added create wallet e2e test
  • Added import wallet e2e test

Note

Medium Risk
Touches wallet onboarding UI and startup path plus iOS signing/build phases; E2E reset uses full SettingsService.clearAll() in-process, which is test-only but must not leak into production behavior.

Overview
Adds Patrol integration tests for the mobile wallet onboarding flows (create wallet and import wallet → home), with iOS native runner wiring and a device-friendly test script.

App / test harness: Startup is extracted into bootstrap() / buildApp() so E2E can reuse production init and pump the real tree; tests reset wallet storage via SettingsService.clearAll() when Patrol can’t clear app data on iOS. Shared E2EKeys and widget keys on welcome, import, account-ready, and home screens drive stable selectors.

iOS: New RunnerUITests target (Patrol macro), CocoaPods nesting, scheme/test plan updates, manual signing for the UI test bundle, and a build phase that embeds Swift Testing support libraries on physical devices (Xcode 26.4+ / Patrol workaround). patrol_ios_device.sh builds with Patrol then runs xcodebuild with a longer destination timeout; test mnemonics come from --dart-define / gitignored .env.test.

Small prod fix: Welcome screen push registration is best-effort so Firebase-not-ready doesn’t block navigation after create wallet.

Reviewed by Cursor Bugbot for commit c618ef2. Configure here.

@dewabisma dewabisma requested a review from n13 June 29, 2026 17:30

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 using default effort and found 1 potential issue.

Fix All in Cursor

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

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit c618ef2. Configure here.

Comment thread mobile-app/lib/v2/screens/welcome/welcome_screen.dart Outdated
@n13 n13 changed the title E2E test setup Patrol Framework added for E2E test setup Jun 30, 2026
@n13

n13 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Ok I think we can try this out but only on minimal code paths that are essential for the app to function

  • Create wallet
  • Import wallet
  • Send flow
  • Show secret phrase

That's all for now, I think.

Then we can also add that to CI so we can always be sure these things keep working

@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: Patrol E2E test setup

Verdict: Approve with minor follow-ups — the approach is solid and the production-facing changes are minimal and safe. Nothing here is a hard blocker, but a few housekeeping items below are worth fixing before/just after merge.

What's good

  • Clean extraction of startup into bootstrap() / buildApp() so tests reuse the real production init path instead of forking a second startup sequence. The _initialized guard makes it safe to run several tests in one process.
  • Single source of truth for selectors: E2EKeys (framework-free strings in lib/) is referenced by both the production widgets and patrol_test/support/selectors.dart, so test selectors and the app can't drift. Good DRY choice.
  • Test secrets handled correctly: TestEnv reads from --dart-define (compile-time), .env.test is gitignored, and nothing is bundled into the release app.
  • The iOS device workarounds (-destination-timeout, the "Embed Swift Testing Frameworks (device)" phase for the Xcode 26.4+/patrol bug) are well documented in-place — that context is genuinely useful.
  • The KeyedSubtree wrapper on the import button is the right call since _buttonKey is already a GlobalKey used for Scrollable.ensureVisible.

Issues to address

  1. patrol_test/test_bundle.dart is committed but is generated. Its own header says DO NOT MODIFY BY HAND AND DO NOT COMMIT TO VERSION CONTROL — patrol_cli regenerates it on every patrol build/patrol test. Committing it will cause needless churn/merge conflicts. Add it to .gitignore and drop it from the PR.

  2. Release-build silent failure in _registerForRemoteNotifications. The catch logs via quantusDebugPrint, which is a no-op in release (if (kDebugMode)). So in production the Firebase/registration failure is still swallowed with zero observability — which is exactly what the Bugbot learned-rule (and our "all failures must produce error logging" principle) is flagging. Since this is a non-critical path the best-effort try/catch is the right shape, but route the error to TelemetryService so we can actually see it in release.

  3. Android is configured but has no harness. pubspec.yaml declares patrol.android.package_name: com.quantus.wallet, but there's no android/.../androidTest instrumentation runner / MainActivityTest. So Android E2E can't run yet. Fine if iOS-only is intentional, but the config implies Android support that doesn't exist — either add the harness or note it as TODO so nobody assumes it works.

  4. AppState.reset() calls SettingsService().clearAll(), which wipes ALL SharedPreferences + ALL secure storage (same method as logout). That's correct for E2E, but it's a high-blast-radius reset: running these tests on a device/simulator that holds a real wallet will irrecoverably destroy its keys. Worth a loud note in the script/README so this only ever runs on throwaway devices, and so we never wire it anywhere near a non-test code path.

Notes / scope

  • iOS signing uses manual signing with a hardcoded PROVISIONING_PROFILE_SPECIFIER = "Local e2e test runner". That works for local device runs but every machine/CI needs that exact named profile installed — keep in mind for the CI goal below.
  • Minor: Selectors and E2EKeys keep two parallel identifier lists in sync by hand. Acceptable given the strings-vs-Key split, just flagging.
  • Minor: TestEnv._require's switch (key) is effectively single-branch today (driven by the String.fromEnvironment const-arg limitation) — fine, just slightly over-built for one value.

vs. the agreed scope

Per my earlier comment, the essential paths were: create wallet, import wallet, send flow, show secret phrase — plus wiring this into CI. This PR delivers create and import; send flow and show secret phrase are still outstanding, and there's no CI workflow yet. Happy to land this as the foundation and track those as follow-ups.

@n13

n13 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Ok lets fix this:
patrol_test/test_bundle.dart is committed but is generated

Then we can check it in

Android tests missing

The reset all is curious - I guess it's ok for any testing device to be reset. These tests are not meant to be run on your personal iPhone?!

@dewabisma

Copy link
Copy Markdown
Collaborator Author

Ok lets fix this: patrol_test/test_bundle.dart is committed but is generated

Then we can check it in

Android tests missing

The reset all is curious - I guess it's ok for any testing device to be reset. These tests are not meant to be run on your personal iPhone?!

Yes this e2e tests are not mean to be run on personal iPhone ideally. Because in test we want to condition the phone to be in state of purity to not have flaky result.

Android tests is missing because I want to have minimal working setup before adding more. Can add it later and test using the google pixel phone I have here.

Also, ideally we can put it in CI and use cloud service for real device instead.

@dewabisma

Copy link
Copy Markdown
Collaborator Author

Actually found out we checked google key for ios notification in git, I will rotate this key so rendering it unusable.

@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.

Re-review @ 1ad9bdf — Verdict: Approve with one should-fix

Re-reviewed in a clean worktree off the latest head. The housekeeping items from the first pass are resolved and the foundation is solid. One should-fix and a few follow-ups below.

Resolved since the last review ✅

  • patrol_test/test_bundle.dart is no longer tracked and is now gitignored.
  • GoogleService-Info.plist and quantus_sdk/rust/Cargo.lock removed from the tree; .env.test gitignored.
  • Startup extraction (bootstrap() / buildApp()) and the shared E2EKeys selectors are clean and DRY — production widgets and patrol_test/support/selectors.dart reference the same strings, so they can't drift.

Should-fix before merge

1. Silent failure in release — welcome_screen.dart:91. _registerForRemoteNotifications catches and logs via quantusDebugPrint, which is compiled out in release (if (kDebugMode) in shared/utils/print.dart). So in production the registration failure is swallowed with zero observability — exactly the "no silent failures / all failures must produce error logging" rule. daf0be9d added the log, but it's still a debug-only no-op. Route it to the existing TelemetryService().sendError(...) (already used for this purpose in import_wallet_screen._discoverAccounts) and capture the stack trace (catch (e, st)).

Follow-ups (not blockers)

2. Same hazard left unfixed in the import flow (DRY). import_wallet_screen._import() still calls ref.read(firebaseMessagingServiceProvider).registerDeviceIfPossible() (lines 119–123) unguarded, inside the same try that gates navigation to Home. FirebaseMessagingService's field initializer touches FirebaseMessaging.instance synchronously (firebase_messaging_service.dart:23), so the first provider read before Firebase is initialized throws synchronously → caught by _import's catch → error surfaced and navigation skipped. That's the identical bug the welcome screen just fixed; the import path is only less likely to hit it (more time elapses before the tap). Rather than duplicate the best-effort wrapper, extract one shared helper and call it from both onboarding entry points.

3. Android harness still missing. pubspec.yaml declares patrol.android.package_name, but there's no android/app/src/androidTest runner, so Android E2E can't run yet. Fine if iOS-first is intentional — just track it so nobody assumes it works.

4. Scope. Per the agreed essential paths: create ✅ and import ✅ are here; send flow, show-secret-phrase, and the CI wiring are still outstanding. Good to land this as the foundation and track those as follow-ups.

On the leaked Firebase API key — root cause (for the record)

The key in GoogleService-Info.plist was not introduced by this PR. It landed in c041e417 ("quic mining", #377, 2026-02-19), before the ignore rule existed. 2be1da87 ("Design V3", #381) then added **/ios/Runner/GoogleService-Info.plist to .gitignore — but git keeps tracking files already committed, so the ignore rule masked an already-tracked secret and it stopped appearing in diffs. This PR is the first to git rm --cached it (it shows up here only as a deletion).

Why review never flagged it:

  • Diff-based review (Bugbot + humans) only sees a PR's delta. The secret was never added in any reviewed delta after the ignore rule existed; here it appears only as a deletion, which reads as a fix.
  • secret_scanning and secret_scanning_push_protection are both disabled on this repo, and there's no gitleaks/trufflehog CI — so nothing scanned the existing tree or blocked the original push.

Remediation: rotate + restrict the key in the Firebase/GCP console (removing it from HEAD does not purge git history — it's still reachable in origin/main), enable GitHub push protection + secret scanning, and ideally add a gitleaks CI step. (A Firebase iOS API key is a client identifier rather than a true secret per Google's guidance, so severity is low — but an unrestricted key can still be abused, so rotating is the right call.)

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