Repository navigation
test: isolate hosted camera permission - #1117
Conversation
Restore shared camera state after hosted navigation fixtures so unit tests do not leave a system permission alert for subsequent UI launches. Refs #1110
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Forces CameraManager.shared.hasPermission = true for the hosted Send fixtures in SendQuickpayLnurlRemountTests and SendSheetLifecycleTests, so Scanner's onAppear no longer calls requestPermissionIfNeeded() and leaves a system camera alert for later UI tests, and restores the original value in a teardown block. Reviewed 1891f12, light tier (12 changed lines, test-only).
What I checked, and 4 candidates I ruled out
Read in full: Bitkit/Managers/CameraManager.swift, the Scanner view in Bitkit/Components/Scanner.swift, both makeHost fixtures.
Not built here: iOS does not build on Linux, so this review is reasoned from the code; the unit test job was still pending when I read CI.
Ruled out
- A prompt from another path:
AVCaptureDevice.requestAccessis only called fromrequestPermissionIfNeeded(), and the only automatic caller isScanner.onAppear, which returns early whenhasPermissionis true.requestPermission()runs only from the permission-request button, which is not rendered oncehasPermissionis true. - The forced value overwritten mid-test:
refreshPermission()is called only from the privateinit, so nothing re-reads the system status after the fixture sets the flag. - A real capture session started: with
hasPermissiontrue on the simulatorScannerrendersColor.blackunder#if targetEnvironment(simulator), notScannerCamera. - Teardown actor mismatch: both test classes are
@MainActorand the teardown block is annotated@MainActor, matchingCameraManager's isolation; no other test inBitkitTestsreadsCameraManager.hasPermission.
Merge confidence: 4/5, no findings, but the unit test job had not finished and the change could not be run here.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for 1891f12:
Check 1: app and test-target build ✅ I built the app and test targets successfully with Xcode 27.0 for the arm64 iOS Simulator.
build-for-testing compiled both changed fixtures with the unchanged Package.resolved. This was a compilation check; it did not execute tests.
Check 2: hosted fixture isolation ✅ Code inspection found the permission override and restoration correctly scoped to the hosted tests.
Both fixtures capture the shared value before hosting, set it before Scanner.onAppear, and restore it on the main actor after the tests remove their windows. Repeated hosts unwind in reverse teardown order. The simulator scanner uses its non-camera branch. Existing test methods, assertions and production code are unchanged.
Check 3: formatting and patch integrity ✅ SwiftFormat 0.64.0 lint and `git diff --check` passed.
Lint covered both changed Swift files using the repository configuration.
Runtime evidence: I relied on @ben-kaufman's controlled comparison at this exact head. It reports the base leaking a camera alert, the candidate completing both hosted suites and the subsequent launch without a prompt, and a passing canonical suite with its existing prerequisite skips. I did not repeat those unchanged cases or run a manual device journey; the QA notes list manual tests and journeys as N/A.
The separate bitkit-ios#1110 Investigate repeated XCTest app-launch background-assertion timeout remains unexplained; the reported base launch passed despite the alert. This approval covers fixture isolation. It does not establish that timeout's cause or resolution.
Coverage: full e4bf44a..1891f12, both changed files and the affected scanner, permission and teardown callers. No new runtime case was driven in this review. The other current-head review also reports no findings.
Android twin: none declared; this changes only iOS hosted XCTest fixtures.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
|
Merged, as it was approved and green. |
|
@ben-kaufman Merged, as it was approved and green. |
Refs #1110
This PR prevents hosted send-navigation fixtures from leaving a system camera-permission alert behind for subsequent UI tests.
Description
Out of Scope
Design
N/A — no UI changes.
Preview
N/A — no user-visible change.
QA Notes
Setup
The controlled comparison used separate newly created, empty iPhone 17 simulators on iOS 26.5 with Xcode 26.1/SDK 26.1 and unchanged
Package.resolved. Camera authorization began undecided; no permission response or privacy reset was issued. Build/test through XcodeBuildMCP with serial testing. SelectSendQuickpayLnurlRemountTestsandSendSheetLifecycleTests, thenBitkitUITests/testExample; inspect the simulator and TCC logs between stages. No funded wallet is required.Journeys
N/A — no user-visible behaviour change.
Manual Tests
N/A
Automated Checks
SendQuickpayLnurlRemountTests.swift— isolate the camera permission state used by six hosted remount cases.SendSheetLifecycleTests.swift— isolate the same state used by fifteen hosted lifecycle cases.e4bf44a2: base leaves a real camera alert after the triggering test and the subsequent passing launch; candidate passes all 21 cases and launch with no prompt request or leaked alert. TCC records only preflight queries for the candidate and no persisted camera authorization.Evidence
Validated
1891f128(the unchanged reviewed diff) against mastere4bf44a2: 2,871 unit tests passed; 43 UI executions finished with 39 passes, four existing prerequisite skips and zero failures, using the six CI integration exclusions. SwiftFormat 0.64: 0/843 files require formatting;git diff --checkpassed. Complete diagnostics were inspected, including four SDK/Pubky constructor QoS stacks and retained simulator/fault-fixture messages; 99 emitting/context sources were compared with the base.Both base and candidate 19-target builds passed. Their focused 21-case suites passed; all 39 runtime diagnostic records match the same five normalized groups/counts, including appearance-transition and socket notices. Production app dylib and UI test executable hashes match across the two builds. Independent review of the frozen two-file diff found no actionable issue.
Gaps
Local Xcode/SDK/runtime differ from failing CI's 16.4/18.5/26.2. This validates fixture isolation, not the daemon assertion-delay cause or a fully repaired #1054 run. The first build failed before compilation because cached VSS metadata pointed to an obsolete path; a private cache copy with corrected artifact paths, identical VSS files and verified pinned revisions built successfully. Runtime diagnostics are retained; no warning-free claim. The later canonical USDT scanner test requested camera normally and XCTest’s default interruption handler allowed it; the no-prompt/undecided-state result above is specifically the controlled fixture comparison. No financial authorization/payment, latency or hardware validation.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)