Repository navigation
test: make the assertions that could not fail able to fail - #81
Conversation
Closes #51. Five tests passed for reasons other than the behaviour they name, and three pieces of RawCan behaviour had no coverage at all. Every change below was verified by breaking the shipping code and watching the test go red; no shipping code is changed by this commit. Deferred-echo bus double (the reusable part) ControllableBus gains an EchoDelivery mode and a DeferredEchoQueue (tests/CanKit.Pro.Tests/Infrastructure/DeferredEchoQueue.cs). In Deferred mode Transmit parks the echo instead of raising it, and the test decides when each one is delivered. This is what makes more than one pending send exist at a time: a synchronous echo re-enters CanBusService's pending-send lock on the transmitting thread, so the pending list only ever holds that thread's own entry. Issue #24 (an expired pending send poisoning the echo FIFO) needs exactly this, so the queue also supports discarding an echo and releasing one late. FR-RAW-031 FIFO matching, TxConfirmTests Echo_Bus_Matches_Identical_Pending_Sends_In_Fifo_Order registers four byte-identical sends against the deferred bus and asserts the n-th echo confirms the n-th transmitted send. The existing concurrency test is kept, on the synchronous bus, and its comment now says what it does and does not prove. IsoTpChannelIntegrationTests Send_Faults_On_Codec_Throw_And_Channel_Remains_Usable never reached the actor -- SendAsync's pre-check enforces the same 4095-byte limit the codec does -- and accepted three exception types. It is split in two: one test pins the pre-check exactly (ArgumentOutOfRangeException, ParamName, nothing on the wire), and a new one drives the actor-side failure contract over a reachable failure, a bus layer that throws out of SendConfirmed, asserting the exception reaches the caller unrewritten and the channel stays usable. Dispose_Unblocks_Pending_ReceiveAsync no longer accepts any exception: it pins InvalidOperationException by exact type (ObjectDisposedException derives from it) and bounds the wait so a Dispose that fails to unblock fails the test instead of hanging the run. J1939NodeTests RebindTransport_DoesNotDeliverBamMoreThanOncePerRebind asserted only received <= sent, which a build that lost every BAM also satisfied. Each BAM now carries a sequence number; the test asserts no sequence number is delivered twice, every delivered one was actually sent, and more than half arrive. Measured margin: ~137 sent, ~121 delivered, the loss being one BAM per rebind window. CanOpenNodeIntegrationTests Sdo_ExpeditedInitiate_ClearsStaleSegmentedServerSession asserted an untouched OD entry, which is equally true when the stray segments are never delivered. A third bus now observes the slave's SDO TX and the test waits for the server's own responses -- the session ack, then one CommandSpecifierInvalid abort per stray segment -- which is the proof the segments arrived and were refused. The two Task.Delay calls this replaces are gone. RawCan coverage that was missing outright Drop-oldest (FR-RAW-011) -- bounded alone was all that was asserted, and drop-newest is also bounded; TryRead, including the empty-buffer case; Reconfigure(predicate), including the null "accept all" form; and Reconfigure after Dispose on both overloads. Flake candidates made deterministic Nmt_StopAndPreOp_TransitionsWork waits for the heartbeat the slave emits with each new state instead of three Task.Delay(30). SendCm_CancelInFlight_SendsConnectionAbort cancels once the RTS is observed on the wire instead of after Task.Delay(50). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR SummaryLow Risk Overview Adds
Integration test hardening: ISO-TP splits oversize-PDU pre-check vs reachable bus-layer Reviewed by Cursor Bugbot for commit bedb449. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: In-flight BAM sequence bound is wrong
- Relaxed the uniqueness bound from seq < finalSent to seq <= finalSent so a cancelled in-flight BAM that still reassembles is accepted rather than treated as corruption.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 0f82d06. Configure here.
Sequence numbers are stamped before SendBamAsync and incremented only after it completes, so cancelling the peer leaves the in-flight BAM uncounted. That datagram can still reassemble during the 150 ms wait with sequence equal to finalSent. Bound delivered sequences with <= so a cancelled send that reached the wire is not treated as corruption.
…action RebindTransport_DoesNotDeliverBamMoreThanOncePerRebind failed on the Windows CI leg of #81 and passed on Ubuntu and macOS. The delivery floor I added (delivered > sent/2) measured the runner, not the node. How much of a free-running BAM stream survives a rebind is the ratio of two unrelated clocks: the rebind cadence (ClaimAnnounceTimeout, 40 ms) against how long one BAM occupies the wire (Th between DT frames). A BAM straddling a rebind is dropped by the disposed channel -- by design, TP reassembly state is not carried across a rebind -- so as the BAM period approaches the rebind spacing, every BAM straddles one and delivery collapses. Windows' ~15.6 ms timer granularity stretches the peer's requested Th of 2 ms to ~15 ms, which is enough to cross that line. Measured here by stretching Th, which reproduces the CI failure locally: Th = 2 ms -> 118 sent, 101 delivered, losses in 15 runs of max 2 Th = 16 ms -> 17 sent, 2 delivered, losses in 2 runs, longest 12 Th = 32 ms -> 8 sent, 0 delivered, one run of 8 That also explains why Windows lost 19 BAMs against 16 rebind windows, in 7 clustered runs rather than 16 single losses: the loss unit is not one BAM per rebind, it is however many BAMs fit inside the window. No duplicate was observed in any configuration, so this is the assertion being wrong about the product, not the product being wrong. The fix splits the two invariants, because they want opposite conditions: * "never twice" needs concurrent traffic, so the background stream stays free-running and keeps a BAM in flight across the rebinds. Nothing is asserted about how much of it arrives -- only that no (PGN, sequence) is delivered more than once. * "still delivering" needs quiet traffic, so each claim is now followed by one probe BAM from a second source address, awaited to delivery. No rebind is due until the test issues the next claim, so a probe cannot be straddled: all 8 arrive, exactly, at any machine speed. This also fires at the instant a fire-and-forget-disposed predecessor would still be subscribed, so it strengthens the duplicate check too. The background provenance check is stated as NotContain rather than OnlyContain: OnlyContain fails on an empty collection, which would smuggle "at least one background BAM arrived" back in as a hidden throughput assumption -- caught by the Th sweep at 32 ms. Verified passing with all three Th settings at 2, 16, 32, 64 and 120 ms (60x the requested value, well past the Windows stretch), and still verified red by both mutations: not starting the reader for the rebound channel, and double-invoking MessageReceived for multi-frame messages. No production-code changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Closes #51.
Five tests passed for reasons other than the behaviour they name, and three pieces of RawCan behaviour had no coverage at all. No shipping code changes here. The acceptance criterion for this PR is not "the suite is green" — it is that each of these tests now goes red when the behaviour it names is broken, so every one was verified by temporarily breaking the production code and watching it fail. The mutation used is listed with each item below; all were reverted.
dotnet test CanKit.Pro.sln -c Release: 409 passed, 0 failed.The deferred-echo bus double
The reusable artifact of this PR, and what item 1 needed:
tests/CanKit.Pro.Tests/Infrastructure/DeferredEchoQueue.cs— the parking lot.ControllableBus.DeferredEchoCapable(session)/EchoDelivery.Deferred/bus.DeferredEchoes— the bus side.In
DeferredmodeTransmitparks the echo instead of raising it, and the test decides when — and in which order — each one is delivered. This is the only way to have more than one send pending at a time: a synchronous echo (what a real echo-mode adapter does) re-entersCanBusService's pending-send lock on the transmitting thread, so the pending list only ever holds that thread's own entry, and every FIFO-ordering rule is vacuously satisfied no matter how the matching code is written.#24 (an expired pending send poisoning the echo FIFO) needs exactly this capability, so the queue also exposes
DiscardNext()(an echo that never arrives while later ones do) and lets an echo be released long after its own send has timed out.WaitForEnqueuedAsyncis the only wait in the class and it waits on a transmit having actually happened, not on a duration. Also mentioned inCONTRIBUTING.mdnext toControllableBus, so it is findable.Tests that can now fail
TxConfirmTests.Echo_Bus_Matches_Identical_Pending_Sends_In_Fifo_Order(new)TryMatchEcho→list.Last/RemoveLast(LIFO). Fails in 52 ms with "expected the 1. echo to confirm the 1. transmitted send"IsoTp…SendAsync_Rejects_Oversized_Pdu_Before_Anything_Reaches_The_Bus(renamed)ArgumentOutOfRangeException,ParamName == "pdu", zero frames on the wireSendAsync's oversize pre-check (the throw then comes from the codec with a differentParamName)IsoTp…Send_Faults_With_The_Bus_Layer_Exception_And_Channel_Remains_Usable(new)OnSendConfirmedrewriting the failure as a genericIsoTpExceptionIsoTp…Dispose_Unblocks_Pending_ReceiveAsyncThrowAsync<Exception>()InvalidOperationExceptionviaBeOfType(ObjectDisposedExceptionderives from it), message check, bounded waitReceiveAsyncthrowingObjectDisposedExceptioninsteadJ1939NodeTests.RebindTransport_DoesNotDeliverBamMoreThanOncePerRebindreceived <= sent— losing every BAM passed(PGN, sequence)uniqueness over a concurrent background stream, plus one awaited probe BAM after each claim — all 8 must arrive, exactlyMessageReceivedfor multi-frame → uniqueness failsCanOpen…Sdo_ExpeditedInitiate_ClearsStaleSegmentedServerSessionCommandSpecifierInvalidabort per stray segmentThe J1939 liveness assertion is a probe, not a fraction (see the follow-up commit). My first version asserted a delivery floor of
sent/2over the free-running background stream. That measured the runner, not the node, and failed on the Windows CI leg: how much of that stream survives is the ratio of the rebind cadence (40 ms) to how long one BAM occupies the wire (Thbetween DT frames), and Windows' ~15.6 ms timer granularity stretches the peer's requestedThof 2 ms until nearly every BAM straddles a rebind. Reproduced locally by stretchingTh:ThThat is also why Windows lost 19 BAMs against 16 rebind windows in 7 clustered runs: the loss unit is not one BAM per rebind, it is however many fit inside the window. No duplicate ever appeared — the product invariant held throughout; the assertion was wrong about the product. A BAM straddling a rebind is dropped by design (TP reassembly state is deliberately not carried across a rebind), so there is no defect here and nothing to file.
The two invariants now have separate traffic, because they need opposite conditions: "never twice" needs concurrency (background stream, nothing asserted about its delivery rate), "still delivering" needs quiet (one probe BAM per claim from a second source address, awaited — it cannot be straddled, so all 8 arrive at any machine speed). Verified passing with
That 2, 16, 32, 64 and 120 ms.Coverage that was missing outright
In
RawCanSubscriptionTests, all driven throughControllableBusso delivery is synchronous and there is nothing to race:BoundedChannelFullMode.DropWrite.TryRead— consumes, returns the next frame on the next call, and reports an empty buffer. Verified red by stubbing it to always return false.Reconfigure(predicate)— including replacing an ID filter outright and thenull"accept all" form. Verified red by making the swap a no-op.Reconfigureafter dispose — both overloads throwObjectDisposedException. Verified red by turning the disposed check into a silent return.Flake candidates
Both made deterministic, no
Task.Delayleft in either:Nmt_StopAndPreOp_TransitionsWork— waits for the heartbeat the slave emits with each new state (it applies the command and sends that heartbeat in the same actor-loop work item) instead of threeTask.Delay(30). Still verified red by making the node ignoreNmtCommand.Stop.SendCm_CancelInFlight_SendsConnectionAbort— cancels once the RTS is observed on the wire instead of afterTask.Delay(50). This removes both failure modes of the delay: cancelling too early under load (nothing registered yet, so no abort is due) and paying 50 ms every run.The two
Task.Delaycalls in the CANopen SDO test are also gone, replaced by waits on the server's own responses.Production bugs found
None that need fixing. One design observation worth recording, but not a bug and not something to change on 1.x:
IsoTpChannel.BeginSendOnLoop's codec-throwcatchis unreachable through the public API.SendAsync's pre-check enforcesMaxClassicFirstFrameLength(4095), which is exactly the limitBuildFirstFrameenforces, and the single-frame path is likewise pre-checked. That duplication is what made the old test misleading — it named a path it could not reach. Thecatchis correct defensive code and should stay; the reachable half of the same contract is now covered by the new bus-layer-throw test. Raise an issue only if someone wants the duplication collapsed, which would be an API-shaping change and does not belong on 1.x.🤖 Generated with Claude Code