test(firmware): guard the canceled-settle case against a persisted network config

[tylerkron]

Rescoped

This PR originally reconciled the WiFi prep/recovery expectations that #444 and #445 left crossed, which had main red. #456 landed that same reconciliation first, so main is green and that part of this PR became redundant — and conflicting.

Rebased onto main and narrowed to the one thing #456 did not carry.

What's left

UpdateWifiModuleAsync_WhenCanceledDuringTransparentModeExitSettle_LeavesLanRestoreUnsent pins the entire recovery sequence with a single Assert.Equal. That already implies LAN:ENAbled 1 and LAN:SAVE never go out — but only implies it.

That expected list is the assertion in this file most likely to be rewritten: every change to the recovery sequence edits it, and whoever edits it is thinking about the commands they're adding, not the two that must never appear. The invariant — a canceled flash must never persist a network configuration — is load-bearing enough to say out loud:

Assert.DoesNotContain("SYSTem:COMMunicate:LAN:ENAbled 1", device.SentCommands);
Assert.DoesNotContain("SYSTem:COMMunicate:LAN:SAVE", device.SentCommands);

Now a cancel that starts persisting config fails on a line that says exactly that, rather than looking like one more expected-list update to wave through.

Notes

• Assertions only; no production change.
• The two command literals are the ones ScpiMessageProducer.EnableNetworkLan / SaveNetworkLan emit, and match how the success-path tests in this same file spell them — so the guards are not vacuous.
• UpdateWifiModuleAsync tests green on net9.0 (24 passed).

If you'd rather not carry the extra guard, closing this is reasonable — #456 already delivered the fix that made main green.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix WiFi firmware update tests for prep power-on and recovery bridge-exit

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Update WiFi update tests to include the prep power-on SCPI command.
• Reconcile cancel/failure-path command sequences with the bridge-exit recovery behavior.
• Add explicit assertions preventing LAN configuration persistence on cancel/failure.

Diagram

graph TD
  T["FirmwareUpdateServiceTests.cs"] --> S["FirmwareUpdateService"] --> U["WifiModuleUpdater"] --> D(["Streaming device (SCPI)"])
  U --> H[["Bridge-exit recovery"]] --> D
  U --> X["Flash tool runner"]
  subgraph Legend
    direction LR
    _f["File"] ~~~ _m["Module/Service"] ~~~ _d(["Device"]) ~~~ _h[["Helper"]]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Dedupe transparent-mode exit on recovery path

• ➕ Avoids sending SetTransparentMode 0 twice when cancel lands mid-exit settle
• ➕ May reduce noisy command traces and simplify exact-sequence tests
• ➖ Requires production-code change and careful reasoning about partial failures/mode ambiguity
• ➖ Risk of reintroducing stranded-bridge scenarios if device state detection is unreliable

2. Assert subsequences/invariants instead of full exact sequences

• ➕ More resilient to harmless command reordering or extra no-op commands
• ➕ Focuses tests on behavioral guarantees (e.g., never persist LAN config on cancel)
• ➖ Weaker regression coverage for ordering-sensitive pacing/settle behavior
• ➖ Can hide unintended extra commands that matter on constrained transports

3. Refactor test helper to build expected command lists from options

• ➕ Reduces duplicated command arrays across tests
• ➕ Makes future prep/recovery changes less likely to break multiple tests
• ➖ More indirection in tests can reduce readability
• ➖ Still needs explicit coverage for ordering-sensitive scenarios (settle placement)

Recommendation: Keep the PR’s approach: update exact-sequence expectations to match the merged prep/recovery reality, and add explicit negative assertions for the real invariant (no LAN:ENAbled / LAN:SAVE persistence on cancel/failure). Consider a follow-up discussion/PR only if the duplicate SetTransparentMode 0 is undesirable in production traces; otherwise it’s a benign no-op and the tests now document why it occurs.

Files changed (1) +28 / -2

Tests (1) +28 / -2

FirmwareUpdateServiceTests.csReconcile WiFi update SCPI command expectations after prep/recovery changes +28/-2

Reconcile WiFi update SCPI command expectations after prep/recovery changes

• Updates multiple WiFi firmware update tests to include the prep power-on command (SYSTem:POWer:STATe 1) before LAN:FWUpdate. Adjusts the cancel-during-transparent-exit-settle test to reflect the failure-path bridge-exit helper sending a second SetTransparentMode 0 plus LAN:APPLY, and adds explicit assertions that LAN persistence commands are never sent.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.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

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[tylerkron]

Ready for review — not merging.

Verified on the exact head b711eca: Qodo reports Bugs (0) / Rule violations (0) / Requirement gaps (0) with 0 unresolved review threads, CI build SUCCESS, MERGEABLE, and origin/main (7965b26) confirmed an ancestor.

Full suite green net9 + net10 — 2703 passed / 2 skipped each, plus 23 Mcp, 0 warnings. Test-only diff (28 insertions, 2 deletions, one file); no production code touched.

Flagging priority again since it is easy to miss in a queue: main is red right now, and #443's failing build is inherited from it rather than caused by anything in #443. Once this lands, #443 should go green on a rerun without needing a change of its own.