Test stability

[ebariaux]

• Added an injectable TimeSource abstraction, with production code using continuous monotonic time instead of Date.now.
• Propagated the time source through ESP provisioning timeout paths, including device search, Wi-Fi scan, and backend connection handling.
• Reworked ESP provisioning tests and mocks to be step-driven/deterministic instead of relying on sleeps or wall-clock timing.
• Updated CI test execution with explicit timeouts, iOS 18.6 simulator target, and XCResult artifact upload for debugging failed runs.

[Miggets7]

Review summary — "Test stability"

Reviewed against main with five focused passes (general code review, test coverage, silent-failure hunt, type-design analysis, comment-accuracy analysis). The PR's premise — making ESP provisioning tests deterministic via injectable time + step-driven mocks — is well executed. A few of the items below are pre-existing issues that the PR is the right vehicle to address since it's already touching the surrounding code.

Critical (worth blocking on)

1. BackendConnectionStatus.failed silently re-polled until timeout — ORLib/ConsoleProviders/ESPProvision/DeviceProvision.swift:68-78. The poll loop only exits on .connected. A definitive .failed from firmware is swallowed; the user sees a misleading 60-s timeoutError instead of the actual failure. The pre-existing // TODO: what about other status values ? (line 69) flagged this; the PR didn't fix it. Add an early exit on .failed with a distinct error code.
2. LoopDetector setter rebuilds the detector mid-scan, silently discarding state — DeviceRegistry.swift:73-85, WifiProvisioner.swift:39-51. After rebuild startTime == nil and LoopDetector.detectLoop() (LoopDetector.swift:46-48) returns true, so a setter call mid-scan instantly aborts with a fake timeoutError. Either reset() the new detector or refuse to mutate while running.
3. SystemTimeSource is entirely untested — production code uses it exclusively; tests use TestTimeSource. A bug in the mach_continuous_time × numer/denom math would only surface at runtime. Add a small Tests/SystemTimeSourceTest.swift (now_isMonotonic, now_advancesAfterRealSleep).
4. CallbackRecorder.wait / mock waitFor* helpers have no upper bound — Tests/ESPProvisionProviderTest.swift:41,92-107, plus the same pattern in ORESPDeviceMock.swift and ORESPProvisionManagerMock.swift. On a regression they hang until CI's 60-s allowance kills the test with the unhelpful "exceeded execution time" message. Wrap each wait in a deadline (withThrowingTaskGroup + Task.sleep) and Issue.record("waited for X, observed actions \(observed)") on timeout — the PR's stated goal is test stability.

Important

5. CallbackRecorder supports only one in-flight waiter — ESPProvisionProviderTest.swift:39-42, 92-107. A second concurrent wait() overwrites the first's continuation; CheckedContinuation will fatal-error on never-resume. Add precondition(self.continuation == nil, "concurrent waits not supported").
6. convenience init(apiURL:) silently drops timeSource — ESPProvisionProvider.swift:79-82. The point of the PR is injectable time, but the only public production init that takes a real parameter hard-codes SystemTimeSource(). There's no public init taking both apiURL and timeSource.
7. Dead/incorrect catch in provisionDevice — ESPProvisionProvider.swift:213-223. DeviceProvision.provision is async (not throws); the try/catch block is unreachable, and if reached it would emit under sendExitProvisioningError (wrong action) instead of sendProvisionDeviceError. Remove or fix.
8. pbxproj tab-indent churn — ORLib.xcodeproj/project.pbxproj (lines 71-86, 177-191, 351-358, 371-378, 556-565, 586-595). Hand-edited extra tabs next to existing 3-tab entries; next Xcode rewrite will produce a noisy reformat diff. Re-save via Xcode.
9. CI default == maximum execution allowance (60 s) — .github/workflows/ci_cd.yml:74-75. No test can opt into a longer allowance; a legitimately slow test gets the same message as a deadlocked one. Bump the maximum, or assert no production timeout exceeds half the cap.
10. Untested negative paths on the new mock surface — failNextDeviceScan, completeNextWifiScan(error:), completeNextSendDataRequest(error:) are defined but never invoked by any test. Notable production gap: DeviceRegistry.devicesScan swallows certain scan errors and re-iterates (the "be cautious" branch) — entirely uncovered.
11. Background Task { … } in ORESPDeviceMock.scanWifiList / completeNextWifiScan — Tests/ORESPDeviceMock.swift:328-336, 348-353. Detached tasks may invoke completionHandler after a test's defer { stopWifiScan() } runs. Currently safe because all tests await waitForWifiScanStarts first, but flaky-by-design under load — make completion awaitable.
12. BLE permission denial path untested — ESPProvisionProvider.enable calls a real BLEPermissionsChecker; the permission == false branch (.notReady error) is never exercised. Needs a small refactor to inject the checker.
13. Catch-all in DeviceProvision.provision — DeviceProvision.swift:92-94. Bare catch collapses every unexpected error type (CancellationError, URLError, …) into genericError. At minimum log the underlying type before mapping.

Suggestions

14. Introduce a nominal struct Instant { let seconds: TimeInterval } in TimeSource.swift. TimeInterval (= Double) leaks the abstraction — any conformer returning Date().timeIntervalSinceReferenceDate would compile and silently violate the monotonic contract. Free at runtime; makes LoopDetector.startTime: Instant? self-documenting. iOS-15 compatible (ContinuousClock is iOS 16+).
15. Collapse LoopDetector's (startTime: TimeInterval?, iterationCount: Int) into private enum State { case fresh; case running(start: Instant, iterations: Int) } so "both reset together" is structural rather than a convention.
16. Doc comments on CallbackRecorder.waitForMessages(matchingActions:) vs (matchingAction:count:) — one is strict-ordering-no-interleaving, the other is permissive. Easy to pick the wrong overload silently.
17. SystemTimeSource.now divides Double(numer)/Double(denom) per call — cache the ratio once.
18. ORESPDeviceMock still has delay: / mockResponses cycling (lines 405-408, 197-214) that no test exercises after the rewrite. Dead test code — remove or cover.
19. Pre-existing TODOs untouched by the rewrite: DeviceProvision.swift:69, 80, ORESPDeviceMock.swift:316, 321, ESPProvisionProviderTest.swift:558-561, 849-852. Resolve or narrow now that the surrounding tests have been rewritten.
20. LoopDetector.detectLoop() returning true for startTime == nil routes a programmer error through the user-facing timeout path. preconditionFailure would be louder and safer.

Strengths

• TimeSource injection is cleanly threaded through LoopDetector, DeviceRegistry, WifiProvisioner, DeviceProvision, ESPProvisionProvider with sensible defaults — no API change for non-test callers.
• mach_continuous_time (vs mach_absolute_time) is the correct primitive for "advances during sleep", and the comment at TimeSource.swift:34 justifies why — a good "why, not what" comment.
• CallbackRecorder correctly handles "messages already match before await" under the lock (no missed-signal bug for the single-waiter case) and resumes continuations outside the locked section.
• Step-driven mocks (completeNextDeviceScan, completeNextWifiScan, completeNextSendDataRequest) + waitFor…(atLeast:) are a clean pattern; Task.sleep is gone from the new tests, which is exactly the stated goal.
• CI hardening (timeout-minutes: 20, xcresult artifact upload, -test-timeouts-enabled YES) is appropriate; iOS 18.6 matches the macos-15 runner image.
• Choosing existential any TimeSource over a generic <Clock> is the right call here — witness-table cost is negligible vs BLE/Wi-Fi work, generic propagation would virally infect every storage site.

Recommended action

1. Fix Make unit tests report visible in GitHub #1, Feature/domain parsing fix #2, Entering domain without scheme prefix crashes the app #4 — these are user-visible reliability bugs the PR's own premise should preclude.
2. Add a SystemTimeSourceTest.swift (Fix license header #3) — small, isolates the only un-mocked production code in the new abstraction.
3. Tighten the test infra (Add ORLib package to Swift Package Index #5, Improve check for BLE permissions and only report back to webapp when… #11) and reuse the new mocks for negative paths (ESPProvisionProvider reports state too soon when enabled #10, ESPProviderProvision - exit provisioning reports back to webapp before action performed #12).
4. Re-save project.pbxproj via Xcode (Move ConfigManager to console-ios-app #8) and resolve init/catch confusion (Convert all unit tests to SwiftTesting #6, Check deprecations and warnings #7).

[MartinaeyNL]

So what exactly is the follow up request on this PR @Miggets7? Does @ebariaux know?
Or are you happy to approve it?

[Miggets7]

I would say to address the critical stuff or create follow up issues for this.
What do you think @ebariaux ?

[ebariaux]

Implemented 1, 2 and 4.
Do not want to implement 3, as making a test that relies on sleep / real clock risks being unstable and the logic to be tested is really small.

For all non critical issue, referenced this list from #7 that should perform a more general review/clean-up of the code.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
67.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ebariaux]

@Miggets7 Ready for another round of review, ran pipeline again 10 times without failures

[Miggets7]

Looks good to me!