test(sdcard): direct tests for the text log line reader's byte counter and lease discipline

[tylerkron]

The coverage-gap maintenance routine produced this. It is safe because it adds one new test file and changes no production code at all — git diff against main touches nothing under src/Daqifi.Core/.

What was wrong

SdCardTextLineReader is the line pump that every line of a downloaded CSV or JSON log passes through on its way to a parser, and it had no tests of its own. Each line it hands over carries a running byte count, and that count is what a caller turns into a download progress bar — so if it drifts, a user watching a large log parse sees a bar that stalls, jumps, or finishes early, and nothing in the suite notices.

Nothing noticed because the CSV and JSON parser fixtures reach this class only along one narrow path: a small, seekable MemoryStream positioned at byte zero, enumerated once, asserted on by the samples that come out the far end. They never read the byte count at all, and they never take the forward-only branch. That left the interesting half of the class unpinned:

• the byte count means two different things depending on the source. A seekable stream reports its real Position; a forward-only one reports an estimate built from line lengths. Either branch could be deleted and every existing test would still pass.
• blank lines are dropped from the output but still counted toward that estimate. Move the counter one line down, inside the blank check, and a log with blank lines silently under-reports its progress forever.
• reading starts at the stream's current position, not at zero — the parsers hand over a stream they have already read a configuration prefix from, so a rewrite that seeks to zero would re-feed the header as data.
• the source overload holds a lease on the source's single read cursor. It has to give that cursor back when the consumer stops early, not only when the file runs out; a leaked lease makes every later read of the same log throw "already being read".

How it was fixed

Adds SdCardTextLineReaderTests.cs — 13 cases over the two ReadLinesAsync overloads and ToAsyncEnumerable. Tests only, no production change; the reader behaves correctly on every edge probed, so no bug was found and no issue opened.

The byte-count cases are built so the two branches produce visibly different numbers. A 20-byte payload fits in one StreamReader buffer, so a seekable stream is already at EOF when the first line arrives and all three lines report 20; the same payload read forward-only reports 6, 12, 20. Neither branch can be substituted for the other without a failure.

Mutation evidence — eleven mutations of SdCardTextLineReader.cs, each applied from a pristine copy and reverted after. All 13 cases die in at least one:

Mutation	Kills
A — move the counter inside the blank-line check	CountsSkippedBlankLinesTowardTheEstimate
B — estimatedBytes += line.Length (drop the terminator)	the 3 forward-only counter cases
C — always report the estimate, ignore CanSeek	ReportsTheStreamsRealPositionNotTheEstimate
D — always report stream.Position, ignore CanSeek	the 3 forward-only counter cases
E — stop skipping blank lines	DropsBlankAndWhitespaceOnlyLines, OnAStreamOfNothingButBlankLines, CountsSkippedBlankLines…
F — leaveOpen: false	LeavesTheCallersStreamOpen + both lease cases
G — seek to 0 before reading	StartsAtTheStreamsCurrentPositionNotAtZero
H — drop ct from ReadLineAsync	WithAnAlreadyCancelledToken_Throws
I — leak the lease instead of await using	both ReleasesTheLease… cases
J — ToAsyncEnumerable walks its source eagerly	DoesNotTouchTheSourceUntilItIsEnumerated
K — ToAsyncEnumerable reverses its source	PreservesOrderAndContent

What a reviewer might push back on

Two cases were written and then deliberately removed rather than shipped, because neither could be made to fail:

• an empty-stream case (Assert.Empty) survived all eleven mutations — there is no realistic way to break the reader that makes a zero-byte stream yield a line, so it was carrying no information. The blank-lines-only case covers the same "yields nothing" shape and is killed by mutation E.
• a "rewinds to the source's origin, not byte zero" case, which turned out to be asserting SdCardParseSource.Open()'s contract rather than this class's — already pinned by SdCardParseSourceTests from test(sdcard): direct tests for SdCardParseSource's rewind, lease latch and stream ownership #654.

The seekable-stream case does depend on the whole payload fitting in one StreamReader buffer. That is stated in a comment on the test; the assertion is on the reader's contract (report the stream's real position), and the buffering only determines what that position happens to be.

Verification: full solution suite green under -warnaserror on net9.0 and net10.0 — 3915 passed / 2 skipped (hardware) on each, plus 217 in Daqifi.Mcp.Tests. Zero warnings. Bench validation does not apply and was not skipped silently: this PR changes no production code and performs no device interaction.

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add direct tests for SdCardTextLineReader byte counting and source lease release

🧪 Tests 🕐 20-40 Minutes

AI Description

• Add direct unit coverage for SdCardTextLineReader’s byte counter across seekable vs forward-only
 streams.
• Verify blank-line filtering while still accounting bytes for progress reporting.
• Assert correct cancellation behavior and SdCardParseSource lease release on normal and early exit.

Diagram

graph TD
  T["SdCardTextLineReaderTests"] --> R["SdCardTextLineReader"] --> S["Stream (seekable/forward-only)"] --> SR["StreamReader"]
  T --> PS["SdCardParseSource"] --> L["Lease"] --> S
  T --> CT["CancellationToken"] --> R

Loading

High-Level Assessment

The PR’s approach—adding direct, edge-focused unit tests around byte-count semantics and lease/disposal behavior—is the most reliable way to pin this logic. Alternatives like relying on higher-level parser fixtures would continue to miss the forward-only path and BytesRead assertions.

Files changed (1) +299 / -0

Tests (1) +299 / -0

SdCardTextLineReaderTests.csAdd focused unit coverage for SdCardTextLineReader semantics +299/-0

Add focused unit coverage for SdCardTextLineReader semantics

• Introduces 13 direct test cases covering blank/whitespace line filtering, seekable vs forward-only BytesRead behavior (including terminator handling and unterminated final lines), and ensuring reading starts from the stream’s current position. Adds assertions that ReadLinesAsync leaves caller streams open, honors cancellation, and that the SdCardParseSource overload releases its lease on both normal completion and early consumer exit. Also validates ToAsyncEnumerable preserves order and is lazy (no eager enumeration).

src/Daqifi.Core.Tests/Device/SdCard/SdCardTextLineReaderTests.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 switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

Qodo-clean, CI green — ready for review.

Independently re-verified on head 11ebc52 (no production diff; the only changed file is the new test):

• Mutation evidence re-run from a pristine copy, not taken on trust. Six independent mutant builds — C, A, I, G, E+B, and F+H+J+K — killed 1, 1, 2, 1, 5 and 6 cases respectively. The three spot-checked singles matched the A–K table exactly, and the union across all six runs is all 13 cases, so no test in the file is unfailable. The pristine file was restored byte-identical (git status clean).
• No weak shapes. No constant assertions, mock-return round-trips, or auto-property round-trips; no assertion made vacuous by an earlier one in the same test.
• No overlap with test(sdcard): direct tests for SdCardParseSource's rewind, lease latch and stream ownership #654. The two lease cases pin this class's await using on the lease (mutation I kills both), whereas SdCardParseSourceTests pins the source's own latch (Open_While…Outstanding_Throws / Open_AfterTheOutstandingLeaseIsDisposed_Succeeds). StartsAtTheStreamsCurrentPositionNotAtZero runs over a raw MemoryStream, not a source, so it is not a restatement of Open_RewindsToWhereTheStreamWasWhenItWasWrapped_NotToByteZero.
• Re-validated against current main (c546abd, after fix(tests): ask the exchange whether it recognised the reply, not the clock #650/chore(tests): drop a write-timeout assertion that the line above already decides #652/test(sdcard): direct tests for SdCardParseSource's rewind, lease latch and stream ownership #654/test(consumers): make the parser routing tests actually test the routing #656/feat(export): let a live stream be recorded straight to CSV #657). Test-merge is a no-op — the branch already contains origin/main — and the full solution suite is green under -warnaserror: net9.0 3915 passed / 2 skipped, net10.0 3915 passed / 2 skipped, Daqifi.Mcp.Tests 217 passed. Zero warnings.
• Qodo: 1 round, Bugs (0) / Rule violations (0) / Requirement gaps (0), and both surfaces checked — the summary comment and an unfiltered reviewThreads dump returning totalCount: 0. Settled: re-checked against the same head SHA well over 3 minutes later with no change to the review body's timestamp and still zero threads. The zero-thread result was control-validated against chore(firmware): compute a hex record's full address in one place #659, which returns 1 thread through the same query, so it is not an empty-query artefact.
• No collision with feat(export): ship the SD-card log adapter Core made everyone write themselves #668 (the SD-card ISampleSource work) — disjoint file sets.

Separately, the HidStringNormalization.Normalize("\0") == "" wart noted in the handover is still live on main and has been filed on its own as #670. It is deliberately not folded into this PR.