Skip to content

Echo semantics are untestable on the default test adapter, and it hid two regressions #94

Description

@dborgards

Found while implementing #23 in #93. Promised there, filed here.

What happens

Almost every L2/L3/L4 test opens CanKit.Adapter.Virtual through CanBus.Open("virtual://…"). That adapter echoes without flagging. VirtualBusHub.Broadcast (CanKit v0.5.6) builds

var data = new CanReceiveData(frame) { ReceiveTimestamp = TimeSpan.Zero }; // simple timestamp

— IsEcho is never assigned, so it defaults to false — and then hands that same record back to the sender when it is in ChannelWorkMode.Echo.

Consequence: includeEcho is a no-op on the default test adapter. A subscription that withholds echoes and one that does not behave identically, because no frame is ever flagged as an echo. Any code path whose correctness depends on the echo gate is, on this adapter, exercised in only one of its two states.

tests/CanKit.Pro.Tests/Infrastructure/ControllableBus.cs:41 already records the fact, in the context of TX-confirm:

whether a given adapter flags its self-echo that way is an adapter detail — CanKit.Adapter.Virtual, for instance, echoes in ChannelWorkMode.Echo without setting the flag

docs/migration-from-legacy.md:149 records it from the other side: the pre-fork repository had to patch IsEcho into the Virtual adapter for TxConfirmTests to match anything at all.

So the knowledge exists. What is missing is any structural reason for a test to act on it.

Why this is worth a ticket rather than a note

It is not hypothetical. In #93 the same mistake was made twice in two rounds, and the full suite was green both times:

Regression Found by What the green suite missed
J1939-TP dropped its source-address self-check as "redundant now that echoes are gated" Codex + Cursor Bugbot, independently On a flagging adapter the check is redundant; on Virtual it is the only defence, and SendBamAsync reassembles its own broadcast
CANopen stopped delivering the node's own SYNC to HandleSync, silencing its synchronous TPDOs Codex Sync_Producer_TriggersReceiver asserts on the remote consumer; nothing covered the producer's own SYNC, and on Virtual the unflagged echo arrives anyway

Both were caught by review, not by tests. Both regression tests written for #93 had to deliberately pick a bus other than the default:

  • Bam_Sender_On_An_Unflagged_Echo_Bus_Does_Not_Receive_Its_Own_Broadcast uses the real Virtual adapter in ChannelWorkMode.Echo — because the point is an adapter that does not flag.
  • Sync_Producer_Still_Sees_Its_Own_Sync_On_A_Flagging_Echo_Bus uses ControllableBus — because the point is an adapter that does.

That the two tests need opposite buses is the whole issue in miniature: there are two distinct echo worlds, and the default adapter only reaches one of them.

What would fix it

Not a single change; a choice between these, in rough order of value:

  1. Make the pairing explicit and cheap. A test fixture or [Theory] data source that runs an echo-sensitive test against both a flagging bus (ControllableBus.EchoCapable) and a non-flagging one (Virtual in ChannelWorkMode.Echo), so "which world is this asserting?" becomes a parameter instead of an unstated assumption.
  2. Name the gap where it bites. Every protocol layer that has a self-traffic guard (J1939-TP source address, J1939 NAME, CANopen SYNC opt-in) should have one test per world. Today that coverage is exactly the two tests feat(rawcan)!: carry the echo flag and receive timestamp to subscribers #93 added.
  3. Consider whether Virtual should flag at all. It is upstream's call, not ours, but it is a genuine upstream-candidate: an adapter that echoes and does not say so makes CanReceiveDataView.IsEcho unusable as a contract. docs/upstream-candidates.md:51 already lists the fork's IsEcho change as a candidate. If upstream took it, option 1 stops being a workaround and becomes a real two-adapter matrix.

Non-goals

Not about widening tolerances or quarantining anything — unrelated to #92, which is about wall-clock flakiness in tests that do cover their subject. This is the opposite failure: tests that pass reliably while covering only half of what they appear to.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: testsTest suite, coverage, API approvalstype: testsTest coverage — a missing, weak or flaky test

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions