From 4c29cf658ea1c1317e398106846633756d470ec1 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Mon, 31 Aug 2026 12:08:18 -0400 Subject: [PATCH 1/4] Stop the test-standards skill from recommending a production clock seam The "Honest gaps" section named an injectable clock as its example of a gap that cannot be closed cheaply, which reads as a remedy to recommend and contradicts the "No test seams in production" rule stated above it. Name the gap as the finding instead, and point at an extracted pure predicate as the way to cover a timing decision directly. Also separate a sleep that advances wall-clock time toward a deadline under test from a sleep used as synchronization, and require the margin be computed before a timing test is called fragile. --- .claude/skills/test-standards/SKILL.md | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/.claude/skills/test-standards/SKILL.md b/.claude/skills/test-standards/SKILL.md index fb76df5..52b25b5 100644 --- a/.claude/skills/test-standards/SKILL.md +++ b/.claude/skills/test-standards/SKILL.md @@ -83,6 +83,9 @@ logic that ships without a test that could catch its breakage. never assert on a counter that the thread under test would have been the one to advance, and never use fixed sleeps as synchronization; use the code's own observable outputs or event flags. +- A sleep that advances wall-clock time toward a real deadline is not + synchronization; before calling such a test fragile, compute the margin + between its nominal elapsed time and that deadline and state the margin. ## Sanitizers @@ -91,10 +94,16 @@ logic that ships without a test that could catch its breakage. ## Honest gaps -- A coverage gap that cannot be closed cheaply (for example, logic needing an - injectable clock) is named explicitly in the PR rather than papered over - with a test that appears to cover it. Flag apparent coverage that does not - actually exercise the gap. +- A coverage gap that cannot be closed cheaply is named explicitly in the PR + rather than papered over with a test that appears to cover it. Flag + apparent coverage that does not actually exercise the gap. +- Naming the gap is the finding. Do not recommend a production seam to close + it: an injectable clock, a virtual hook, or a swappable transport added to + `src/` or `include/` for a test's benefit is the seam violation above, not + the remedy for it. This project has no clock injection seam and a review + must not propose introducing one; where a timing decision needs direct + coverage, extract it into a pure predicate the test calls with supplied + values. ## Report format From 376ed6e340fffa51a42fde9a1f569af40acdd0e6 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Mon, 31 Aug 2026 12:09:44 -0400 Subject: [PATCH 2/4] Add a granularity and independence section to the test-standards skill The checklist judged whether an individual test was strong but said nothing about how the suite is carved up, so a reviewer could not flag a long test covering several behaviors, a cluster of near-duplicate cases that wants a value-parameterized test, or a test that depends on another having run first. --- .claude/skills/test-standards/SKILL.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/.claude/skills/test-standards/SKILL.md b/.claude/skills/test-standards/SKILL.md index 52b25b5..554ed9f 100644 --- a/.claude/skills/test-standards/SKILL.md +++ b/.claude/skills/test-standards/SKILL.md @@ -65,6 +65,28 @@ logic that ships without a test that could catch its breakage. history ("rejects spectrum config missing n_disp_bins", not "regression test for the config bug"). +## Granularity and independence + +- One behavior per test: not one assertion per test, and not one test per + bug. Several assertions about the same behavior belong together and + splitting them is filler; a long test spanning several behaviors is the + opposite failure and gets split along the behaviors it conflates. + `MetadataNullClearsAndAbsentPreserves` (`tests/test_protocol.cpp`) asserts + three things about the single delta-merge rule it covers. +- A test name describes the behavior closely enough that a red CI run + identifies the break without opening the file (see the naming bullet under + "No filler"). +- `ASSERT_*` aborts the test function while `EXPECT_*` continues, so an + over-merged test masks later failures behind the first one. Reserve + `ASSERT_*` for the point where continuing would be meaningless, such as a + parse that must succeed before its result is read. +- The same behavior over differing inputs belongs in a value-parameterized + test (`TEST_P` with `INSTANTIATE_TEST_SUITE_P`) rather than copy-pasted + near-duplicate cases. The malformed-input rejection tests in + `tests/test_protocol.cpp` are the standing example of that shape. +- No test depends on execution order, on another test having run first, or on + shared mutable global state; each test sets up the world it needs. + ## No test seams in production - Production code in `src/` and `include/` must not acquire friends, From eb86eeabc5998426c6cc5bc9712a18be86a226a4 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Mon, 31 Aug 2026 12:22:44 -0400 Subject: [PATCH 3/4] Say which shape the protocol rejection tests exemplify The bullet pointed at those tests as "the standing example of that shape" after naming both a parameterized suite and the copy-pasted cases it replaces, leaving the referent ambiguous. They are the copy-pasted shape; the file contains no value-parameterized tests. --- .claude/skills/test-standards/SKILL.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/skills/test-standards/SKILL.md b/.claude/skills/test-standards/SKILL.md index 554ed9f..3b1c2dd 100644 --- a/.claude/skills/test-standards/SKILL.md +++ b/.claude/skills/test-standards/SKILL.md @@ -83,7 +83,8 @@ logic that ships without a test that could catch its breakage. - The same behavior over differing inputs belongs in a value-parameterized test (`TEST_P` with `INSTANTIATE_TEST_SUITE_P`) rather than copy-pasted near-duplicate cases. The malformed-input rejection tests in - `tests/test_protocol.cpp` are the standing example of that shape. + `tests/test_protocol.cpp` are the standing example of the copy-pasted + shape, and are what a parameterized suite would replace. - No test depends on execution order, on another test having run first, or on shared mutable global state; each test sets up the world it needs. From 420a073c197aa96b8f9a351b8f1f6507ad357008 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Mon, 31 Aug 2026 12:26:02 -0400 Subject: [PATCH 4/4] Drop the value-parameterized test bullet The bullet pointed at the malformed-input tests in tests/test_protocol.cpp as copy-pasted near-duplicates. They are not: the PlayerCommand cases cover range, type width, and boolean strictness separately, and the FormatClientCommand cases cover a different serializer branch each. They share the control-plus-rejection template this file already prescribes, and parameterizing them would cost the per-case comments. The suite has no instance of the pattern the rule described. --- .claude/skills/test-standards/SKILL.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.claude/skills/test-standards/SKILL.md b/.claude/skills/test-standards/SKILL.md index 3b1c2dd..0e4d769 100644 --- a/.claude/skills/test-standards/SKILL.md +++ b/.claude/skills/test-standards/SKILL.md @@ -80,11 +80,6 @@ logic that ships without a test that could catch its breakage. over-merged test masks later failures behind the first one. Reserve `ASSERT_*` for the point where continuing would be meaningless, such as a parse that must succeed before its result is read. -- The same behavior over differing inputs belongs in a value-parameterized - test (`TEST_P` with `INSTANTIATE_TEST_SUITE_P`) rather than copy-pasted - near-duplicate cases. The malformed-input rejection tests in - `tests/test_protocol.cpp` are the standing example of the copy-pasted - shape, and are what a parameterized suite would replace. - No test depends on execution order, on another test having run first, or on shared mutable global state; each test sets up the world it needs.