Fix a contradiction in the test-standards skill and add suite granularity rules - #111
Merged
Conversation
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.
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new “standing example” claim about TEST_P usage is inaccurate relative to tests/test_protocol.cpp, so the guidance text needs a small correction before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the test-standards review skill documentation to remove a contradictory recommendation (clock injection as a “cheap” fix) and to add explicit guidance on test suite granularity/independence, strengthening how reviewers evaluate test quality in this codebase.
Changes:
- Adds a new “Granularity and independence” section with guidance on one-behavior-per-test,
ASSERT_*vsEXPECT_*,TEST_Pusage, and independence expectations. - Clarifies that “honest gaps” should be named without recommending production test seams (e.g., injectable clocks), and suggests extracting pure predicates for timing decisions.
- Distinguishes deadline-advancing sleeps from synchronization sleeps and requires stating deadline margin before calling timing tests fragile.
File summaries
| File | Description |
|---|---|
| .claude/skills/test-standards/SKILL.md | Refines test review guidance to avoid recommending production seams and adds suite granularity/independence rules. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes to
.claude/skills/test-standards/SKILL.md. No test or productioncode is touched.
Do not recommend a production clock seam
"Honest gaps" named an injectable clock as its example of a gap that cannot be
closed cheaply. That reads as a remedy to recommend, and it contradicts "No test
seams in production" stated earlier in the same file. A review of #110 followed
it and asked for a clock seam this project deliberately does not have.
Naming the gap is now the finding, and an extracted pure predicate is the way to
cover a timing decision directly.
The same commit separates a sleep that advances wall-clock time toward a
deadline under test from a sleep used as synchronization, and requires the
margin be computed before a timing test is called fragile.
Granularity and independence
The checklist judged whether an individual test was strong but said nothing
about how the suite is carved up. A reviewer could not flag a long test covering
several behaviors, or a test that depends on another having run first.
The new section covers one behavior per test, test naming,
ASSERT_*abortingwhere
EXPECT_*continues, and test independence. It citesMetadataNullClearsAndAbsentPreservesintests/test_protocol.cppas theexample of several assertions belonging to one behavior.
A bullet recommending value-parameterized tests was written and then dropped:
it pointed at the malformed-input tests in
tests/test_protocol.cppascopy-pasted near-duplicates, and they are not. Those cases each cover a distinct
rule while sharing the control-plus-rejection template this file already
prescribes. The suite has no instance of the pattern that rule described.
pre-commit run --all-filespasses. Frontmatter is unchanged.