diff --git a/packages/agents/content/_partials/test-structure-audit-checklist.md b/packages/agents/content/_partials/test-structure-audit-checklist.md new file mode 100644 index 00000000..cf573c18 --- /dev/null +++ b/packages/agents/content/_partials/test-structure-audit-checklist.md @@ -0,0 +1,7 @@ +Before saving a spec file, scan adjacent tests for shared-setup runs. + +- **Parameterize** with `it.each` when the variation is small and the test body is structurally identical. +- **Extract a named helper** when the variation is bigger, when shared setup should disappear into a helper signature, or when assertion shapes differ per case. +- **Check the assertions, too.** When the rule is a count or predicate, specific-fixture-label assertions should not repeat per row. Assert specific data flow _once_ in a focused test; assert structurally for the rest. + +Three or more `it` blocks with identical setup and a single varying input is the most common failure mode in interactive sessions. That material belongs behind a helper or a parameterized table, not in N copies of the same shape. diff --git a/packages/agents/content/skills/code-patterns/SKILL.md b/packages/agents/content/skills/code-patterns/SKILL.md index 7294cee4..0c22d592 100644 --- a/packages/agents/content/skills/code-patterns/SKILL.md +++ b/packages/agents/content/skills/code-patterns/SKILL.md @@ -60,7 +60,10 @@ Follow the naming rules in [naming-conventions.md](../_data/naming-conventions.m ### Test structure -- **Use parameterized tests** with `it.each([...])` to avoid verbose, repetitive test cases +Rules live in [`testing-conventions/SKILL.md`](../testing-conventions/SKILL.md): parameterization, helpers, and signal-to-noise as the bar for both. + +### Test organization + - **Use function/class reference as describe argument** - `describe(myFunction, ...)` instead of `describe('myFunction', ...)` - **Place tests in `__tests__` directory** as sibling to the file being tested diff --git a/packages/agents/content/skills/testing-conventions/SKILL.md b/packages/agents/content/skills/testing-conventions/SKILL.md index abf69da7..afe00318 100644 --- a/packages/agents/content/skills/testing-conventions/SKILL.md +++ b/packages/agents/content/skills/testing-conventions/SKILL.md @@ -67,6 +67,30 @@ Comment discipline applies to test files the same as to source. The full rule se Everything else is over-commenting. Test names already communicate intent; assertions communicate the check. +## Test structure + +Tests should make their variation easy to see. When N adjacent tests differ only in one input but share a wall of identical setup, the reader has to diff three or four nearly-identical render calls to find what's actually being tested. The fix is not "delete things"; it's "factor the shared part out so the variation reads as variation." + +This is the same signal-buried-in-noise failure mode that [`comment-discipline.md`](../_data/comment-discipline.md) addresses on the comment side. Different mechanism, same principle. + +### Rules + +1. **Test bodies should be mostly variation and assertion, not setup.** Any prop, fixture, or boilerplate identical across N adjacent tests is noise. Factor it into a default-bearing helper, or collapse the tests into a single parameterized test where the variation reads as a table. +2. **Use `it.each` when N adjacent tests differ only in a small set of inputs and the body is structurally identical.** Use a named helper when the variation is bigger, when shared setup should disappear into a helper signature, or when assertion shapes differ per case. +3. **Test the rule, not the data.** Prefer counts, predicates, and structural assertions (`expect(getVisibleChipCount()).toBe(N)`, `expect(queryPillElement()).toBeInTheDocument()`) over enumerating specific fixture labels in every row. Assert specific data flow _once_, in a focused test, not per row of a table. + +### Diagnostic + +Before writing a third test in the same `describe` block, scan the previous two: How many tokens does a reader have to diff to find what's actually different between them? If the answer is more than a handful, the signal is buried. Parameterize the trio or extract a helper before continuing. + +### When N copies are right + +The smell is shared _setup with one variable_, not shared _shape with different intents_. Three tests that read as genuinely distinct behavioral claims should stay as three `it` blocks even if their bodies superficially resemble each other. Do not collapse distinct intents into a table. + +### Audit before save + + + ## Additional patterns ### Omit "should" from test names diff --git a/packages/agents/content/subagents/code-simplification-reviewer.md b/packages/agents/content/subagents/code-simplification-reviewer.md index 2491b745..6cbff4dc 100644 --- a/packages/agents/content/subagents/code-simplification-reviewer.md +++ b/packages/agents/content/subagents/code-simplification-reviewer.md @@ -90,6 +90,10 @@ Focus exclusively on simplification opportunities in changed code: - Domain leaks in shared/common code - Inline "what" comments that describe what the code does instead of explaining why a non-obvious decision was made - `eslint-disable` rationales that explain the surrounding decision rather than the specific suppression +- Test-structure violations. See `{platform_home_dir}/skills/testing-conventions/SKILL.md` for the full rule set. Common patterns to flag: + - Adjacent tests with near-identical setup where only one input varies (parameterize with `it.each` or extract a helper) + - Specific-fixture-label assertions repeated per row when the rule is a count or predicate + - Helpers whose parameters the test bodies still re-specify (the abstraction failed to absorb the duplication) - Logic that can be consolidated without sacrificing clarity ### Simplification principles @@ -125,8 +129,8 @@ See the "Finding references" section in the `review-criteria` skill for path-for Classify the overall review into exactly one level (none/low/medium/high) per the `review-criteria` skill. Domain context for this reviewer: - `none`: Code is already clean and well-structured — no simplification opportunities -- `low`: Minor opportunities (e.g., a handful of redundant or paraphrasing comments, one unused import) -- `medium`: Several meaningful simplification opportunities, including file-header-scale comment violations (tutorial headers, repeated conversation memorialization, broad library re-teaching) +- `low`: Minor opportunities (e.g., a handful of redundant or paraphrasing comments, one or two adjacent-test runs with shared-setup duplication, one unused import) +- `medium`: Several meaningful simplification opportunities, including file-header-scale comment violations (tutorial headers, repeated conversation memorialization, broad library re-teaching) or pervasive shared-setup duplication across a spec file - `high`: Pervasive unnecessary complexity indicating the code needs a simplification pass ## Output format