test(consumers): make the parser routing tests actually test the routing

[tylerkron]

What was wrong

Ten tests across CompositeMessageParserTests and ProtobufMessageParserTests carried the names of routing tests — ShouldUseProtobufParser, ShouldDetectAsBinary, WithMultipleMessages_ShouldParseFirst — but their only assertion was Assert.True(consumedBytes >= 0). Nothing in them said which parser had run, which is the entire behaviour they were named for. A change that routed every buffer to the wrong parser would have left all ten green. One was a strict tautology (Assert.True(binaryMessages.Count() >= 0) — Enumerable.Count() cannot be negative), and five hid their only real assertion behind if (messages.Any()), so they asserted nothing whenever the parser returned empty — which, for the buffers they used, was always. Three of them ran on an empty byte array despite being named for valid protobuf. The suite was reporting coverage of the parser routing that the project did not actually have.

How it was fixed

Each test now pins the real behaviour: which parser was consulted and in what order, how many messages came out, of what concrete type and with what decoded field values, and what consumedBytes exactly equals. Two recording parsers write into one shared ordered call log, so a test can assert the routing order (["protobuf", "text"]) rather than merely that a parser was reached — and can prove the negative, that the second parser was never consulted when the first answered. Every if (messages.Any()) guard is gone. The buffers that were fake "mock protobuf" bytes (not valid length-delimited frames at all, which is why nothing ever parsed) are now real frames built with WriteDelimitedTo.

Two things a reviewer should push back on if they disagree:

1. WithNullBytes_ShouldDetectAsBinary was named for the opposite of what the code does. "Hello\0World" is 90.9% printable ASCII and 9.1% null, and DetectMessageType weighs printable ratio before null density on purpose (the 10% null threshold isn't even met). It routes to text. That is deliberate and documented in the classifier — the genuinely binary case it guards against is the 40%-null / 24%-printable SYSInfoPB? reply from issue Make serial-discovery protobuf parsing resilient to non-protobuf noise (so the probe needn't disable echo) #268 — so I renamed the test to WithOneNullByteInPrintableData_ShouldDetectAsText rather than pin a behaviour the code doesn't have. This is not a bug report; no parser bug was found. If you'd rather the classifier did treat any null byte as binary, that's a separate design change and this test would need to move with it.

2. ReturnsCorrectMessageType is a tenth test, not in issue The composite/protobuf parser tests named for routing behavior assert almost nothing #653's list of nine. It sits in the same file with the identical defect (empty buffer, if (messages.Any()), consumedBytes >= 0). Leaving it would have shipped a file that still contained the exact pattern this PR claims to remove.

Mutation evidence — every rewritten test was confirmed able to fail

Each mutation was applied to production code, the tests run, then reverted. Verified red:

Mutation	Tests it turns red
M1 — swap the two routing branches in CompositeMessageParser.ParseMessages	WithBinaryData_ShouldUseProtobufParser, WithOneNullByteInPrintableData_ShouldDetectAsText, WithNullTextParser_UsesDefaultLineParser, WithNullProtobufParser_UsesDefaultProtobufParser, WhenPreferredParserFindsNothing_FallsBackWithFullBuffer
M2 — TryParse adopts Math.Max(consumedBytes, parserConsumed) instead of only adopting on a produced message	WithMixedScenarios rows truncated protobuf frame, binary with nulls
M3 — currentIndex += messageLength (drop the length prefix from the advance)	WithValidProtobuf_ShouldReturnMessage, WithMultipleMessages_ShouldParseAll, ReturnsCorrectMessageType, ReturnsDifferentMessageTypes, WithNullProtobufParser_UsesDefaultProtobufParser
M4 — resync advances by 2 instead of 1 in the invalid-length branch	WithNullBytes_ResyncsAndPreservesTrailingPrefix (and only that one)
M5 — break after the first parsed frame	WithMultipleMessages_ShouldParseAll
M6 — TextPrintableRatio 0.8 → 0.95	WithOneNullByteInPrintableData_ShouldDetectAsText
M7 — LineBasedMessageParser reports consumedBytes without the line ending	ReturnsDifferentMessageTypes, WithNullTextParser_UsesDefaultLineParser, WithMixedScenarios rows SCPI command, SCPI query
M8 — LineBasedMessageParser swallows an unterminated tail as a line	WithMixedScenarios rows lone non-printable byte, truncated protobuf frame, binary with nulls

The one input I could not make failable is the empty buffer, because CompositeMessageParser returns before routing — so I dropped it from the theory rather than keep a row no mutation can kill; the existing WithEmptyData_ShouldReturnEmpty already pins that contract exactly. There is a comment in the theory data saying so.

Verification

Test-only change; no production code is modified. Full suite green on both frameworks: net9.0 Core 3838 passed / 2 skipped, net10.0 Core 3838 passed / 2 skipped, Mcp 217 passed, 0 failed, 0 warnings. Line coverage 88.48%. Bench validation does not apply — nothing here touches the device, and the board was never acquired.

The diff also drops trailing whitespace from four adjacent tests I did not otherwise change, a side effect of rewriting the file.

closes #653

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix parser routing tests to assert actual parser selection and consumption

🧪 Tests 🕐 40+ Minutes

AI Description

• Replace tautological assertions with checks for routing order, message types, and consumedBytes.
• Use real length-delimited protobuf frames (WriteDelimitedTo) instead of non-parsable mock bytes.
• Add recording parsers + shared call log to prove fallback behavior and “negative” routing cases.

Diagram

graph TD
  T["Routing test suite"] --> CMP["CompositeMessageParser"] --> RP["RecordingProtobufParser"] --> RL[("RoutingLog")] --> A["Assert order + outputs"]
  CMP --> RT["RecordingTextParser"] --> RL
  T --> PMP["ProtobufMessageParser"] --> A

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use a mocking framework (e.g., Moq) to spy on parsers

• ➕ Less custom test helper code (no Recording*Parser classes)
• ➕ Can verify call count/order via mock invocations
• ➖ Order assertions across two independent mocks can be more awkward than a shared log
• ➖ Mock setups can obscure intent compared to small explicit spy implementations

2. Expose routing decision/hooks from CompositeMessageParser for tests

• ➕ Tests could assert routing choice directly without indirection
• ➕ Potentially simpler assertions for “preferred vs fallback” behavior
• ➖ Requires production-code changes purely for testability
• ➖ Risks baking test-only concerns into public API or internals

Recommendation: Keep the PR’s approach (explicit recording parsers + shared ordered call log + real delimited frames). It provides strong behavioral guarantees (including negative assertions like “fallback not consulted”) without modifying production code, and it avoids brittle/opaque mock configurations while pinning the consumedBytes contract that matters to stream integrity.

Files changed (2) +297 / -129

Tests (2) +297 / -129

CompositeMessageParserTests.csRewrite routing tests to assert parser order, outputs, and fallback invariants +230/-85

Rewrite routing tests to assert parser order, outputs, and fallback invariants

• Replaces non-asserting/tautological routing tests with concrete checks for which parser ran (and in what order), the returned message types/values, and exact consumedBytes. Adds a shared RoutingLog plus RecordingTextParser/RecordingProtobufParser spies to prove both positive routing and the negative case where fallback must not run when the first parser answers. Converts “mock binary” inputs to real length-delimited protobuf frames and expands mixed-scenario coverage via a parameterized Theory.

src/Daqifi.Core.Tests/Communication/Consumers/CompositeMessageParserTests.cs

ProtobufMessageParserTests.csMake protobuf parser tests use real frames and assert resync/consumption behavior +67/-44

Make protobuf parser tests use real frames and assert resync/consumption behavior

• Updates tests to build real length-delimited protobuf frames via WriteDelimitedTo and assert decoded field values and full-frame consumption. Strengthens the null-byte resync test to pin the exact consumedBytes behavior (preserving a possible trailing varint prefix). Updates the multiple-message test to assert that all frames are parsed in order and the entire buffer is consumed.

src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.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 09c66ee: Qodo Bugs 0 / Rule violations 0 / Requirement gaps 0 with zero review threads (settle re-check 4 min later unchanged; anti-artifact check on #627 returned its 2 threads), CI build success on that exact SHA, dotnet build --no-incremental -warnaserror clean at 0 warnings on both TFMs, and the full suite green (Core 3838 net9.0 + 3838 net10.0, Mcp 217, 0 failed). Re-derived three of the mutations myself rather than trusting the table — M1 (swap the routing branches) killed exactly the 5 listed tests, M3 (drop the length prefix from the advance) killed the 5 listed plus 4 pre-existing, and M4 (resync advance of 2) killed exactly WithNullBytes_ResyncsAndPreservesTrailingPrefix. Also confirmed the WithOneNullByteInPrintableData_ShouldDetectAsText rename against the classifier source: "Hello\0World" is 10/11 printable content bytes (0.909 > TextPrintableRatio 0.8) so heuristic 1 returns LikelyText — and its null ratio is 1/11 = 0.0909, which is not above BinaryNullRatio 0.1, so the buffer classifies as text under either ordering of the two heuristics. The rename is therefore not papering over a precedence question; a reviewer who wants any null byte to force binary is asking for a separate design change.