Skip to content

test(j1939tp): a paced BAM round trip does not gate on the receiver's default T1 - #279

Merged
dborgards merged 1 commit into
mainfrom
test/277-bam-roundtrip-t1
Oct 4, 2026
Merged

dborgards merged 1 commit into
mainfrom
test/277-bam-roundtrip-t1

Conversation

@dborgards

Copy link
Copy Markdown
Owner

What does this change?

Closes #277. Eight tests in J1939TpTests round-trip a BAM paced at 5 ms on the real clock and assert the reassembled datagram, not timing. Their receivers kept the default T1 of 750 ms, re-armed per TP.DT, while every DT crosses a thread-pool confirmation hop, an actor post and the spacing timer on its way. A host stall longer than 750 ms between any two packets turned a reassembly test into a T1 abort, which is what the macOS leg of #276 produced.

Five tests in the class already pin T1 high for this reason (t1: TimeSpan.FromSeconds(5) "T1 must not fire first", and 30 s elsewhere). The eight now share one PacedRoundtripOptions() with T1 at 30 s, so ShortTimeout is their only clock, and the comment on it names the quantity the host perturbs and the margin that remains. Parallel_Bam_And_TwoCm_Sessions_Do_Not_Interfere lifts T2 and T3 the same way, since its CM sessions run on the same clock. T1 keeps its own tests (Bam_Receiver_T1Timeout_FaultsReceiveAsyncWhenDtStops and the virtual-clock ones), which is where a timer that is the subject belongs.

Not taken from #277: the abort message naming the packet index. That is product code and a fix release for a diagnostic this change makes unnecessary in the suite; if it is wanted for the field, it is a separate decision.

No mutation check: this change removes an incidental gate rather than adding a detection claim. The eight tests' reassembly assertions are unchanged.

Type of change

  • feat — new behaviour (minor release)
  • fix / perf — bug or performance fix (patch release)
  • docs / test / refactor / chore / ci — no release
  • Breaking change (! in the title, plus a BREAKING CHANGE: footer explaining the migration)

Checklist

🤖 Generated with Claude Code

https://claude.ai/code/session_01NPLBiJfQ9po3gRXbWpiQn6


Generated by Claude Code

… default T1

Eight tests round-trip a BAM paced at 5 ms on the real clock and assert the
reassembled datagram. Their receivers kept the default T1 of 750 ms, re-armed
per TP.DT, while every DT crosses a thread-pool confirmation hop, an actor post
and the spacing timer on its way: a host stall longer than 750 ms between any
two packets turned a reassembly test into a T1 abort, which is what the macOS
leg of #276 produced (#277). Five tests in the class already pin T1 high for
this reason ("T1 must not fire first"); these now share one PacedRoundtripOptions
with T1 at 30 s, so ShortTimeout is their only clock. The parallel BAM+CM test
lifts T2 and T3 the same way. T1 itself keeps its own tests.

Closes #277

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NPLBiJfQ9po3gRXbWpiQn6
@cursor

cursor Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Test-only changes to J1939Tp integration options; no runtime or public API impact.

Overview
Stabilizes eight J1939-TP virtual-loopback tests that round-trip a 5 ms paced BAM and assert reassembly, not timing (#277). They previously kept the default 750 ms T1, which re-arms on every TP.DT; on loaded hosts (e.g. macOS CI) a stall between packets could abort reassembly instead of failing the intended assertion.

Adds shared PacedRoundtripOptions() — 5 ms bamPacketSpacing and 30 s t1 — so ShortTimeout is the only meaningful clock for those tests. Parallel_Bam_And_TwoCm_Sessions_Do_Not_Interfere uses the same helper and also sets T2/T3 to 30 s for concurrent CM sessions. T1 timeout behavior stays covered by dedicated tests. No production code changes; reassembly assertions are unchanged.

Reviewed by Cursor Bugbot for commit 1f879ab. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-04T18:39:47.465757Z 1f879ab PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@codecov

codecov Bot commented Oct 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@dborgards
dborgards merged commit 8fc64eb into main Oct 4, 2026
13 checks passed
@dborgards
dborgards deleted the test/277-bam-roundtrip-t1 branch October 4, 2026 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bam_Roundtrip_ReceiverReassemblesIdenticalPayload failed once on macos-latest with a T1 timeout

2 participants