test(firmware): reconcile the WiFi update SCPI sequence expectations (part of #269)

[tylerkron]

Problem

Five FirmwareUpdateServiceTests fail on a clean checkout of main (7965b26), on both net9.0 and net10.0:

• UpdateWifiModuleAsync_WhenRecoveryBudgetExpiresAfterReconnect_StillFinishesTheBridgeExit
• UpdateWifiModuleAsync_WhenCanceledAfterEnteringUpdateMode_StillTakesDeviceBackOut
• UpdateWifiModuleAsync_WhenFlashToolFails_TakesDeviceBackOutOfLanUpdateMode
• UpdateWifiModuleAsync_WhenCanceledDuringTransparentModeExitSettle_LeavesLanRestoreUnsent
• UpdateWifiModuleAsync_WhenRecoveryBudgetExpiresWaitingForReconnect_SendsNoBridgeExit

Bisect

Tree	Result
#445 branch head (1967c2ee)	Passed — 108/108
#444 branch head (06bcf8ae)	Passed — 111/111
da35eff — #445 merged	Passed — 108/108
7965b26 — #444 merged	Failed — 5/116

Both PRs branched from the same base (4b8eed2) and were green there. They touched different regions of WifiModuleUpdater.cs, so git merged them with no textual conflict — but each changed the SCPI traffic the other's tests assert on. 7965b26 is the commit that broke it only because it merged second.

Which side is wrong

The test expectations, on both sides. Neither production behavior is changed here.

#444 → breaks #445's four recovery tests. Prep now powers the WINC on before LAN:FWUpdate, because LAN commands are rejected (-200) while the module is unpowered, so the mode command would silently not take. That prepends SYSTem:POWer:STATe 1 to every sequence those four tests pin. The behavior is deliberate, hardware-backed, and separately covered by #444's own toggle tests (PowerOnWifiModuleBeforeLanUpdateMode on/off) and by the success-path sequence assertion, all of which still pass.

#445 → breaks #444's settle-placement test. A failed or canceled flash now walks the device back out of bridge mode, so the cancel path gains a second transparent-mode exit and a LAN:APPLY kick. That is the entire point of #445 — a canceled flash mid-bridge is the most likely way to strand the module — and the recovery deliberately omits LAN:ENAbled/LAN:SAVE so it cannot persist a configuration off a flash that did not complete. The redundant SetTransparentMode 0 is a documented no-op; the LAN:APPLY is genuinely needed, because at the cancel point the console has been handed back but the WiFi manager has not been kicked out of its bridge-mode state machine — the one state worse than either end of the sequence.

UpdateWifiModuleAsync_WhenCanceledDuringTransparentModeExitSettle_LeavesLanRestoreUnsent still proves what its name and comment claim: the restore is LAN:ENAbled + LAN:SAVE, and neither is sent. Only the trailing recovery pair is new, and the comment now says so.

Verification

Full solution, both TFMs:

• Daqifi.Core.Tests — 2703 passed, 0 failed, 2 skipped (net9.0 and net10.0)
• Daqifi.Mcp.Tests — 23 passed, 0 failed
• 0 build warnings

Tests only; no production code touched.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix WiFi update SCPI sequence assertions in firmware tests

🧪 Tests 🕐 20-40 Minutes

AI Description

• Align WiFi-update recovery tests with prep’s required WINC power-on-before-LAN:FWUpdate ordering.
• Update cancel/settle test to expect failure recovery (extra transparent-exit + LAN:APPLY), not LAN
 restore.
• Clarify test commentary distinguishing LAN restore (ENAbled/SAVE) from bridge-exit recovery.

Diagram

sequenceDiagram
  actor T as "FirmwareUpdateServiceTests"
  participant S as "FirmwareUpdateService"
  participant W as "WifiModuleUpdater"
  participant D as "Device (SCPI)"
  participant F as "WINC flash tool"

  T->>S: UpdateWifiModuleAsync(...)
  S->>W: RunUpdateAsync(...)

  Note over W,D: Prep (enter LAN FW update mode)
  W->>D: "SYSTem:POWer:STATe 1" (when enabled)
  W->>D: "SYSTem:COMMUnicate:LAN:FWUpdate"

  W->>F: Run flash tool

  alt flash succeeds
    Note over W,D: Restore LAN config
    W->>D: "SYSTem:USB:SetTransparentMode 0"
    W->>D: "LAN:ENAbled 1" + "LAN:APPLY" + "LAN:SAVE"
  else flash fails or caller cancels
    Note over W,D: Failure/cancel recovery (bridge exit only)
    W->>D: "SYSTem:USB:SetTransparentMode 0"
    W->>D: "SYSTem:COMMunicate:LAN:APPLY"
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Assert subsequences instead of full SCPI transcript

• ➕ Less brittle when benign command ordering/extra commands change
• ➕ Focuses on critical invariants (e.g., restore not sent on cancel)
• ➖ May miss regressions in ordering that matter for real hardware behavior
• ➖ Reduces documentation value of tests as an executable protocol spec

2. Centralize expected sequences via a helper/builder

• ➕ Keeps strict ordering while reducing duplication across tests
• ➕ Makes option-dependent expectations (e.g., power-on toggle) explicit and reusable
• ➖ Adds indirection that can make tests harder to read at a glance
• ➖ Risk of encoding the implementation logic into the test helper

3. Use named protocol events (semantic assertions)

• ➕ Tests read as behaviors (enter update mode, exit bridge, restore LAN) rather than raw strings
• ➕ Allows evolving SCPI spelling without touching every assertion
• ➖ Requires extra abstraction layer/mapping between events and SCPI strings
• ➖ Can hide low-level protocol regressions unless carefully designed

Recommendation: Keep the PR’s approach (update the pinned command sequences) because these tests intentionally act as an exact, hardware-backed SCPI protocol spec—especially around recovery paths where ordering matters. If churn continues, consider introducing a small expected-sequence builder to reduce duplication while preserving strict ordering.

Files changed (1) +23 / -4

Tests (1) +23 / -4

FirmwareUpdateServiceTests.csReconcile WiFi update SCPI sequence expectations across cancel/failure tests +23/-4

Reconcile WiFi update SCPI sequence expectations across cancel/failure tests

• Updates pinned SCPI command lists to include prep’s leading power-on before LAN:FWUpdate and to reflect cancel/failure recovery appending an additional transparent-mode exit and LAN:APPLY. Clarifies commentary to distinguish the LAN restore (ENAbled/SAVE) from the separate bridge-exit recovery behavior.

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

[tylerkron]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Imprecise restore definition ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The updated cancellation-test comment describes the LAN restore as only LAN:ENAbled + LAN:SAVE,
but the normal restore path is LAN:ENAbled + LAN:APPLY + LAN:SAVE. In this test, LAN:APPLY is
still expected (from failure recovery), so the comment should explicitly distinguish “restore APPLY”
vs “recovery APPLY” to avoid confusion when maintaining the sequence assertions.

Code

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[R1728-1731]

+        // The restore is what the settle holds back, and the restore is LAN:ENAbled + LAN:SAVE —
+        // neither appears. The trailing pair is not the restore resuming: it is the failure
+        // recovery, which the cancel itself arms and which walks the device out of bridge mode
+        // with its own transparent-mode exit and a LAN:APPLY kick. It deliberately persists

Relevance

●●● Strong

Team commonly accepts updating comments/docs to match actual behavior and avoid misleading
maintenance cues.

PR-#357
PR-#435

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test comment calls the restore LAN:ENAbled + LAN:SAVE, but the production restore includes
LAN:APPLY as well; separately, the recovery path also sends LAN:APPLY without persisting config,
which is why the test still expects an APPLY even when restore is held back by cancellation.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[1728-1739]
src/Daqifi.Core/Firmware/WifiModuleUpdater.cs[179-210]
src/Daqifi.Core/Firmware/WifiModuleUpdater.cs[949-956]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A test comment introduced/edited in this PR states the LAN restore is `LAN:ENAbled + LAN:SAVE`. Production code’s successful restore sequence includes `LAN:ENAbled`, `LAN:APPLY`, and `LAN:SAVE`. In the cancellation test, `LAN:APPLY` is still present, but it comes from the failure-recovery bridge-exit path, not from the held-back restore. The comment should be clarified to prevent maintainers from misunderstanding what the restore step actually is.

## Issue Context
- Normal post-flash restore in `WifiModuleUpdater.RunUpdateAsync` sends `EnableNetworkLan`, `ApplyNetworkLan`, `SaveNetworkLan`.
- Failure/cancel recovery (`TryLeaveLanUpdateModeAfterFailureAsync`) sends `SetUsbTransparencyMode(0)` then `ApplyNetworkLan` (no persistence).
- The cancellation test can only reliably assert absence of `LAN:ENAbled` and `LAN:SAVE` from the restore, because `LAN:APPLY` will occur via recovery.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[1673-1677]
- src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[1728-1732]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

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

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3ffc516