test(firmware): the WiFi updater's own edge cases had no tests of their own

[tylerkron]

What was wrong

WifiModuleUpdater is the ~900-line piece of Core that actually flashes the WiFi module — it finds Microchip's flash tool, builds its command line, answers its interactive prompts, decides whether a failed run is worth retrying, and turns whatever the tool printed into an explanation a caller can act on. None of that had a test of its own. Every test that touched it drove FirmwareUpdateService and watched from the outside.

That was fine for the happy path, which the facade suite covers well, but it left real behaviour unpinned. Nothing checked that a COM port or firmware path containing a space gets quoted before it becomes a command line. Nothing checked that "the tool couldn't open the port" and "the tool reached the device and the programming failed" produce different messages — the whole point of that code is telling those two apart. Nothing checked that the tool's stdin prompt still gets answered on a second attempt, even though the responder is deliberately one-shot and a spent one would leave the tool blocked forever. A regression in any of these would have shipped.

How it was fixed

30 direct unit tests for WifiModuleUpdater, covering the seams the facade cannot reach cleanly: flash-tool and port resolution, argument quoting, all five branches of the "why did this flash not report success" verdict, the retry policy's boundaries, the prompt handshake and its per-attempt freshness, the progress band mapping, and the status probe's behaviour when the (mutable) options object is changed after the service was constructed.

Deliberately not a re-run of the facade suite one level down. FirmwareUpdateServiceTests already covers the WiFi happy path, the cancellation points and the bridge-exit recovery, and duplicating those here would only mean two places to edit for every change. The file's doc comment says which half lives where.

The one thing a reviewer might push back on: this moves three test doubles (FakeStreamingDevice, FakeFirmwareDownloadService, FakeExternalProcessRunner) out of FirmwareUpdateServiceTests into a shared FirmwareUpdateTestDoubles.cs, rather than hand-copying ~145 lines of IStreamingDevice surface into a second file that would then have to be kept in step by hand. The move is verbatim — same names, same behaviour — so the facade test file loses 246 lines and gains none, and no existing test body changed. The only additions to the doubles are extra observation hooks (every request seen, and the stdin responses produced) that nothing existing reads.

Verification

• Mutation-checked, not just green. Nine mutations were applied to WifiModuleUpdater.cs one at a time and each was caught by the test written for it: dropping the stdout scan from the transient-failure classifier (2 tests), removing the attempt-count clamp, removing the argument quoting, removing the prompt responder's one-shot guard, reordering the image-build check behind the device-reached check, reporting raw tool percent instead of the mapped band, caching the chip-info retry budget instead of re-reading it, parsing the minimum version strictly instead of degrading to "no opinion", and waiting out the settle delay after a failed power-on send. Source restored after each.
• Full suite green on net9.0 and net10.0 (3165 passed / 2 skipped each, plus 86 Mcp), 0 warnings. Every pre-existing test passes unchanged — that is the equivalence check for the doubles move, not an afterthought.
• No bench run, because no production code changed — this PR touches only src/Daqifi.Core.Tests/.

Part of #464 — slice 1 of the four the issue lists. Deliberately not closes: the Pic32 collaborators, the discovery descriptor providers and SdCardOperations are still uncovered, so the issue should stay open until they land.

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add direct unit tests for WifiModuleUpdater edge cases

🧪 Tests 🕐 40+ Minutes

AI Description

• Add a focused WifiModuleUpdater unit test suite for port/tool resolution, quoting, retries, and
 prompts.
• Validate distinct failure classifications and progress mapping from flash tool output.
• Extract shared firmware update test doubles to avoid duplication across firmware test suites.

Diagram

graph TD
  T1["WifiModuleUpdaterTests.cs"] --> U(["WifiModuleUpdater"])
  U --> R["FakeExternalProcessRunner"]
  U --> D["FakeStreamingDevice / FakeLanChipInfoDevice"]
  U --> DL["FakeFirmwareDownloadService"]
  T2["FirmwareUpdateServiceTests.cs"] --> TD["FirmwareUpdateTestDoubles.cs"]
  T1 --> TD

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep doubles nested and duplicate per test file

• ➕ Preserves locality: fakes live next to the tests that use them
• ➕ Avoids introducing shared test infrastructure that can grow organically
• ➖ High duplication cost (notably IStreamingDevice surface) and drift risk across suites
• ➖ Harder to add new observation hooks consistently for retry/prompt assertions

2. Use a mocking framework for IStreamingDevice/IExternalProcessRunner

• ➕ Less handwritten fake code; tests can tailor behavior per test
• ➕ Can reduce file size by generating stubs dynamically
• ➖ Harder to model ordered interactions and capture prompt-response transcripts cleanly
• ➖ Mock-heavy tests tend to be more brittle/opaque for reviewers and future maintainers

3. Add coverage only via FirmwareUpdateService facade tests

• ➕ Exercises the system the way production calls it
• ➕ Avoids exposing collaborator seams in tests
• ➖ Many behaviors here are awkward or impossible to assert via the facade (quoting, retry classification branches, per-attempt stdin responder freshness)
• ➖ Facade tests would become slower and more coupled to integration behavior

Recommendation: Prefer the PR’s approach: add direct WifiModuleUpdater tests for collaborator-only seams, and share the existing fakes via a common test-doubles file. This keeps facade tests focused on end-to-end flow while pinning the nuanced classification/quoting/retry/prompt behaviors that would otherwise regress silently.

Files changed (3) +1133 / -246

Tests (3) +1133 / -246

FirmwareUpdateServiceTests.csRemove nested firmware update fakes now shared across suites +0/-246

Remove nested firmware update fakes now shared across suites

• Deletes the previously nested FakeStreamingDevice, FakeExternalProcessRunner, and FakeFirmwareDownloadService definitions. This is a test-infrastructure refactor to allow other firmware test files (notably WifiModuleUpdaterTests) to reuse identical doubles without duplication.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs

FirmwareUpdateTestDoubles.csIntroduce shared firmware update test doubles with extra observability +320/-0

Introduce shared firmware update test doubles with extra observability

• Adds shared implementations of FakeStreamingDevice, FakeFirmwareDownloadService, and FakeExternalProcessRunner, plus a FakeLanChipInfoDevice for chip-info probing paths. Enhances the process runner with request history and captured stdin prompt responses to validate per-attempt prompt handling and retry behavior.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateTestDoubles.cs

WifiModuleUpdaterTests.csAdd comprehensive unit tests for WifiModuleUpdater edge cases +813/-0

Add comprehensive unit tests for WifiModuleUpdater edge cases

• Introduces a dedicated test suite covering flash-tool/port resolution, argument quoting/escaping, platform-specific tool invocation behavior, failure-shape classification, retry policy boundaries (including misconfigured attempts), stdin prompt handshake behavior, progress-band mapping, and status probing against mutable options.

src/Daqifi.Core.Tests/Firmware/WifiModuleUpdaterTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

Qodo-clean, CI green — ready for review. (1 round on head 5516ba5: Bugs (0) / Rule violations (0) / Requirement gaps (0), 0 unresolved review threads; settle re-check at +4 min found the review comment byte-identical and still 0 threads. build SUCCESS, mergeStateStatus CLEAN. Not merging.)