test(sdcard): the SD card block had no tests that could see inside a text exchange

[tylerkron]

What was wrong

The SD card block is the biggest single piece of device logic in Core — listing, downloading, deleting, logging, the shared-SPI-bus handover — and nothing tested it on its own. Every test of it ran through the device class it was split out of, which means every test watched it from behind the real text-exchange machinery. From there you can see which commands went out, but not the things that actually go wrong on a real card: how many times an operation retried, whether the bus switch was performed as part of the exchange or loosely around it, whether a device that dropped mid-operation still got commands written at it, or whether a download that timed out left its worker holding the transport. Those behaviours were all built deliberately, in response to specific field failures, and none of them had a test that would notice if they were undone.

How it was fixed

57 tests that drive the SD block directly, through a stand-in device that records everything it is asked to do and refuses anything outside the block's remit. The stand-in reproduces the real exchange's phase order, so a test can now assert that the SPI-bus switch happens inside the exchange, that each attempt hands the bus back before the retry delay, and that a device which drops mid-listing gets no restore written at it. On the download path, tests hold one transfer open to prove a second is refused rather than allowed onto the same stream, and park a transfer where no cancellation can reach it to prove that when it's abandoned Core stops writing to that link entirely.

Worth pushing back on if you disagree: this adds a second test file for the same type rather than reorganising the existing one. SdCardOperationsTests.cs is 131 tests of end-to-end behaviour through the device, and it is the evidence that the extraction changed nothing — rewriting it to reach the collaborator would throw that evidence away to save a filename. It is untouched; the new file says in its header why it exists beside it.

Verification

Full suite green on net9.0 (3342 Core + 192 Mcp) and net10.0 (3342), Release build with 0 warnings. Eight mutations of the production code confirm the tests bite rather than merely execute: scanning the listing terminator from the start instead of the end (2 failures), retrying the "no SD card" reply as if it were transient (1), restoring the LAN interface after the link already dropped (1), restoring it onto an abandoned worker's transport (1), re-enabling LAN over a WiFi connection (1), dropping the retry budget to zero (9), running the SPI switch inline instead of as the exchange's prepare phase (2), and ignoring the listed file size when judging an empty transfer (1). The new tests were also run 8 times consecutively to check the two timing-sensitive download tests are stable.

No bench validation: this is a test-only change and no board is connected to this session.

Part of #464 — slice 4 of the four the issue lists, after #522 (WiFi module updater) and #527 (PIC32 updater/bootloader).

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add collaborator-level tests for SdCardOperations text-exchange behavior

🧪 Tests 🕐 40+ Minutes

AI Description

• Add direct SdCardOperations unit tests via a strict IDeviceOperationHost fake.
• Assert exchange phase ordering, retries, and SPI/LAN handover semantics.
• Cover download concurrency, timeouts, and abandoned-transfer behavior.

Diagram

sequenceDiagram
  participant T as "Collaborator tests"
  participant O as "SdCardOperations"
  participant H as "FakeHost (IDeviceOperationHost)"
  participant X as "Text exchange"
  participant R as "Raw stream"

  T->>O: GetSdCardFilesAsync()
  O->>H: ExecuteTextCommandAsync(prepare/setup/finalize)
  H->>X: begin
  X-->>H: prepareAsync()
  H-->>O: setupAction() (LIST?, ERR?)
  X-->>H: finalizeAsync()
  H->>X: end

  T->>O: DownloadSdCardFileAsync()
  O->>H: ExecuteRawCaptureAsync(rawAction)
  H->>R: Provide Scripted/Parked stream
  R-->>O: bytes... + EOF

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Refactor existing facade tests to target the host seam

• ➕ Single consolidated test file/suite
• ➕ Less duplication of fixtures across test files
• ➖ Loses the end-to-end coverage that proves extraction didn’t change observable wire behavior
• ➖ Still harder to validate facade-specific behavior vs collaborator-only behavior cleanly

2. Use a mocking framework for IDeviceOperationHost instead of a custom fake

• ➕ Less bespoke test infrastructure
• ➕ Built-in call verification and argument matching
• ➖ Harder to model phase-accurate exchange ordering (prepare/setup/finalize) and scripted multi-exchange responses
• ➖ Less strict by default; may miss accidental dependencies unless carefully configured
• ➖ More brittle when asserting ordered call sequences across async worker threads

Recommendation: Keep the new collaborator-focused test file alongside the existing facade tests. The split preserves end-to-end “wire evidence” while adding visibility into exchange internals (phase ordering, retry counts, and transport ownership) that mocks/facade tests can’t reliably observe. The strict FakeHost approach is appropriate here because correctness depends on exact sequencing and on loudly failing any creep beyond SdCardOperations’ remit.

Files changed (1) +1200 / -0

Tests (1) +1200 / -0

SdCardOperationsCollaboratorTests.csAdd direct SdCardOperations tests with strict host + scripted streams +1200/-0

Add direct SdCardOperations tests with strict host + scripted streams

• Introduces a collaborator-level xUnit test suite that drives SdCardOperations via IDeviceOperationHost to validate text-exchange boundaries, retry budgets, and interface handover ordering. Adds a strict FakeHost that records allowed calls, replays scripted exchange responses, and throws on any out-of-remit host API usage, plus ScriptedStream/ParkedStream helpers to deterministically simulate download payloads and stalls.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs

[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. Test can hang indefinitely ✓ Resolved 🐞 Bug ☼ Reliability

Description

DownloadSdCardFileAsync_TransferThatIgnoresItsToken_IsAbandonedWithoutARestore has no test-level
timeout and awaits a call that can block forever if the production hard-deadline logic regresses,
causing the whole test run to hang.

Code

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[R862-863]

+            await Assert.ThrowsAsync<TimeoutException>(
+                () => ops.DownloadSdCardFileAsync("data.bin", destination));

Relevance

●●● Strong

Strong precedent: async tests should be time-bounded to avoid hanging CI when awaits can block
indefinitely.

PR-#454
PR-#440
PR-#364

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test awaits a TimeoutException without any independent timeout, but the underlying stream is
designed to ignore cancellation and block until released; if the production hard-deadline stops
firing, the await never completes (and the finally that releases the stream never runs). Production
code explicitly calls out that this download path exists to bound hangs from native I/O stalls, and
prior accepted guidance recommends bounding async tests to fail fast instead of hanging CI.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[839-873]
src/Daqifi.Core/Device/SdCard/SdCardOperations.cs[821-836]
PR-#454

Agent prompt

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

### Issue description
`DownloadSdCardFileAsync_TransferThatIgnoresItsToken_IsAbandonedWithoutARestore` awaits a timeout from production code while using a `ParkedStream` that ignores cancellation. If the production timeout/hard-deadline regresses, this test can block forever and hang CI.

A related risk exists in `DownloadSdCardFileAsync_WhileAnotherIsParkedOnTheTransport_RefusesInsteadOfSharingIt`: `await first;` in the `finally` can also hang if a regression prevents the first download from completing even after release.

### Issue Context
The parked stream explicitly waits for the test to release it and ignores the cancellation token. The production method’s documentation also explains this path exists specifically to avoid hangs when native I/O cannot be interrupted.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[821-873]

### Suggested fix
- Add an explicit upper bound to the test(s), e.g.:
 - Use `[Fact(Timeout = 5000)]` (or similar) on the parked/timeout tests, **or**
 - Wrap key awaits with `WaitAsync(TimeSpan.FromSeconds(...))`, e.g.:
   - `await Assert.ThrowsAsync<TimeoutException>(...).WaitAsync(TimeSpan.FromSeconds(5));`
   - `await first.WaitAsync(TimeSpan.FromSeconds(10));`
- Ensure the release `finally` still runs and does not depend on the unbounded await completing.

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

---

2. Overlong test timeout budget ✓ Resolved 🐞 Bug ➹ Performance ⭐ New

Description

ParkedTestBudget is 30s despite comments stating the asserted behavior should complete in well under
a second; when a regression occurs, multiple WaitAsync/guard uses can compound into a long delay
before the test fails. This slows CI failure feedback and makes regressions more expensive to
diagnose.

Code

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[R56-59]

+    /// Both assert something the production code should reach in well under a second; the bound is
+    /// only there so a regression that never returns fails the one test instead of hanging the run.
+    /// </summary>
+    private static readonly TimeSpan ParkedTestBudget = TimeSpan.FromSeconds(30);

Relevance

●●● Strong

Team has accepted tightening oversized test timeouts to reduce worst-case CI latency.

PR-#458
PR-#454

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test comment says the production behavior should be reached “in well under a second”, but the
shared guard is 30s and is used to bound several waits. If the awaited condition doesn’t happen
(regression or hang), each wait can consume up to the full budget before failing; since there are
multiple waits, the delay can compound.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[54-60]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[831-846]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[870-881]

Agent prompt

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

### Issue description
`ParkedTestBudget` is set to 30 seconds while the tests’ own comments indicate the relevant code paths should complete in under ~1s. Because this same budget is used in multiple awaits (entry signal, the “second download refused” assertion, cleanup await, and the guard CTS), a failure path can waste tens of seconds before failing, delaying CI feedback.

### Issue Context
These tests intentionally simulate non-cancellable I/O. The goal is not to remove the safety bound, but to choose bounds that are tight enough to fail fast on regressions while still being stable under CI load.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[54-60]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[831-846]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[870-881]

### Suggested fix direction
- Lower `ParkedTestBudget` to a smaller CI-safe value (e.g., a few seconds), OR
- Split into separate budgets:
 - a short budget for “should be immediate” waits (entered signal / semaphore gate), and
 - a slightly larger budget for cleanup/abandonment guard.
This keeps failure feedback fast without removing the hang-prevention guard.

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

---

3. Logging tests add real waits ✗ Dismissed 🐞 Bug ➹ Performance

Description

Several tests call StartSdCardLoggingSessionAsync, which includes multiple real Task.Delay(100ms)
waits; this adds substantial wall-clock time to the suite for tests that only need the busy-state,
slowing CI and local runs.

Code

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[R548-549]

+        var session = await ops.StartSdCardLoggingSessionAsync(format: format);
+

Relevance

●● Moderate

Team often reduces flaky/real-time waits in tests, but removing production-delay waits may require
refactor; uncertainty.

PR-#104
PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The production StartSdCardLoggingSessionAsync includes repeated Task.Delay(100ms) waits. The new
test file calls it in multiple tests (including a [Theory] that runs multiple times) and also in
several busy-state tests that don’t assert on the delayed/settling behavior, adding unnecessary
real-time waits to the suite.

src/Daqifi.Core/Device/SdCard/SdCardOperations.cs[609-631]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[538-588]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[455-463]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[916-926]

Agent prompt

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

### Issue description
Some new tests call `StartSdCardLoggingSessionAsync(...)` even when they only need to place `SdCardOperations` into the “busy/logging” state. That production method performs several `Task.Delay(100, ...)` waits, so these tests incur real-time delays unrelated to what they assert.

### Issue Context
`StartSdCardLoggingSessionAsync` is a production API that includes multiple settling delays. In unit tests, those delays can dominate runtime and are unnecessary for tests that only need the `_isLoggingToSdCard` flag set.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[454-463]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[629-639]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[698-706]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[916-926]
- src/Daqifi.Core/Device/SdCard/SdCardOperations.cs[609-631]

### Suggested fix
For tests that only need the busy state (e.g., `..._WhileLogging_ThrowsBusy`):
- Prefer setting the logging/busy flag without going through the full start sequence. Options:
 - Use reflection in the test to set the private `_isLoggingToSdCard` field to `true` (test-only), or
 - Introduce a small test-only seam/helper (internal method guarded for tests) to toggle the busy state without delays.

Keep the full `StartSdCardLoggingSessionAsync` calls only in tests that explicitly validate logging command ordering/validation.

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

---

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit f6c3243

Results up to commit 2559993 ⚖️ Balanced

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

1. Test can hang indefinitely ✓ Resolved 🐞 Bug ☼ Reliability

Description

DownloadSdCardFileAsync_TransferThatIgnoresItsToken_IsAbandonedWithoutARestore has no test-level
timeout and awaits a call that can block forever if the production hard-deadline logic regresses,
causing the whole test run to hang.

Code

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[R862-863]

+            await Assert.ThrowsAsync<TimeoutException>(
+                () => ops.DownloadSdCardFileAsync("data.bin", destination));

Relevance

●●● Strong

Strong precedent: async tests should be time-bounded to avoid hanging CI when awaits can block
indefinitely.

PR-#454
PR-#440
PR-#364

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test awaits a TimeoutException without any independent timeout, but the underlying stream is
designed to ignore cancellation and block until released; if the production hard-deadline stops
firing, the await never completes (and the finally that releases the stream never runs). Production
code explicitly calls out that this download path exists to bound hangs from native I/O stalls, and
prior accepted guidance recommends bounding async tests to fail fast instead of hanging CI.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[839-873]
src/Daqifi.Core/Device/SdCard/SdCardOperations.cs[821-836]
PR-#454

Agent prompt

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

### Issue description
`DownloadSdCardFileAsync_TransferThatIgnoresItsToken_IsAbandonedWithoutARestore` awaits a timeout from production code while using a `ParkedStream` that ignores cancellation. If the production timeout/hard-deadline regresses, this test can block forever and hang CI.

A related risk exists in `DownloadSdCardFileAsync_WhileAnotherIsParkedOnTheTransport_RefusesInsteadOfSharingIt`: `await first;` in the `finally` can also hang if a regression prevents the first download from completing even after release.

### Issue Context
The parked stream explicitly waits for the test to release it and ignores the cancellation token. The production method’s documentation also explains this path exists specifically to avoid hangs when native I/O cannot be interrupted.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[821-873]

### Suggested fix
- Add an explicit upper bound to the test(s), e.g.:
 - Use `[Fact(Timeout = 5000)]` (or similar) on the parked/timeout tests, **or**
 - Wrap key awaits with `WaitAsync(TimeSpan.FromSeconds(...))`, e.g.:
   - `await Assert.ThrowsAsync<TimeoutException>(...).WaitAsync(TimeSpan.FromSeconds(5));`
   - `await first.WaitAsync(TimeSpan.FromSeconds(10));`
- Ensure the release `finally` still runs and does not depend on the unbounded await completing.

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

---

2. Logging tests add real waits ✗ Dismissed 🐞 Bug ➹ Performance

Description

Several tests call StartSdCardLoggingSessionAsync, which includes multiple real Task.Delay(100ms)
waits; this adds substantial wall-clock time to the suite for tests that only need the busy-state,
slowing CI and local runs.

Code

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[R548-549]

+        var session = await ops.StartSdCardLoggingSessionAsync(format: format);
+

Relevance

●● Moderate

Team often reduces flaky/real-time waits in tests, but removing production-delay waits may require
refactor; uncertainty.

PR-#104
PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The production StartSdCardLoggingSessionAsync includes repeated Task.Delay(100ms) waits. The new
test file calls it in multiple tests (including a [Theory] that runs multiple times) and also in
several busy-state tests that don’t assert on the delayed/settling behavior, adding unnecessary
real-time waits to the suite.

src/Daqifi.Core/Device/SdCard/SdCardOperations.cs[609-631]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[538-588]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[455-463]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[916-926]

Agent prompt

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

### Issue description
Some new tests call `StartSdCardLoggingSessionAsync(...)` even when they only need to place `SdCardOperations` into the “busy/logging” state. That production method performs several `Task.Delay(100, ...)` waits, so these tests incur real-time delays unrelated to what they assert.

### Issue Context
`StartSdCardLoggingSessionAsync` is a production API that includes multiple settling delays. In unit tests, those delays can dominate runtime and are unnecessary for tests that only need the `_isLoggingToSdCard` flag set.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[454-463]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[629-639]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[698-706]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsCollaboratorTests.cs[916-926]
- src/Daqifi.Core/Device/SdCard/SdCardOperations.cs[609-631]

### Suggested fix
For tests that only need the busy state (e.g., `..._WhileLogging_ThrowsBusy`):
- Prefer setting the logging/busy flag without going through the full start sequence. Options:
 - Use reflection in the test to set the private `_isLoggingToSdCard` field to `true` (test-only), or
 - Introduce a small test-only seam/helper (internal method guarded for tests) to toggle the busy state without delays.

Keep the full `StartSdCardLoggingSessionAsync` calls only in tests that explicitly validate logging command ordering/validation.

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

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit a02075f

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit f6c3243

[tylerkron]

Qodo-clean, CI green — ready for review.

3 rounds on head f6c3243.

• Round 1 (2559993) raised two. The High one was real: the two tests that park a transfer on a stream which ignores cancellation had no upper bound, so a regression in the download's hard deadline would have hung the run instead of failing it. Fixed in a02075f — with the wrinkle that the suggested WaitAsync(TimeSpan) would have reported a TimeoutException, the very type the abandon test asserts, so a genuine hang would have passed as a success; the bound is a cancellation token instead. The Medium one (four busy-state tests pay ~500 ms each going through StartSdCardLoggingSessionAsync) was declined: reaching the busy state the way a caller reaches it is what makes those tests also pin that starting a session is what sets the flag, and both suggested fixes — reflection onto a private field, or a test-only mutator in production — would have traded that away for 2 s.
• Round 2 (a02075f) raised one Medium: the 30 s hang guard was used by four awaits, so a regression could burn most of a minute before reporting itself. Taken in f6c3243 as a single 10 s budget; declined the suggested split into two budgets, since one constant is already ~30x the longest wait under it.
• Round 3 (f6c3243) came back empty, and re-checked clean after settling.

Full suite green on net9.0 (3342 Core + 192 Mcp) and net10.0 (3342), Release build 0 warnings. No bench validation — test-only change, and no board is attached to this session.