Test Review

---
name: test-review
description: >-
  Test quality checklist, security-testing requirements, and dynamic-analysis
  expectations — language-agnostic, with per-section slots a stack fills in.
  Load when conducting test reviews.
compatibility:
  - claude-code
  - github-copilot
  - opencode
reads:
  - docs/testing-principles.md
  - docs/system-design.md
metadata:
  version: "1.0"
  author: team
---

## Project Testing Policy (read from the brief)

The policy values this review enforces are project-owned and live in [`docs/testing-principles.md`](../../../docs/testing-principles.md): the test pyramid ratios (§ Test Pyramid), the coverage measure (§ Coverage), the mocking policy (§ Mocking Policy), and the naming school (§ Test Naming). Read them before reviewing and enforce what the brief says, not remembered defaults — the brief is the contract that survives harness upgrades. If the brief contradicts itself or the code under review reveals a gap in it, raise a `clarify` finding against the brief instead of silently substituting your own values.

## Test Placement

The brief's pyramid section is a placement rule, not a ratio to eyeball. The design doc's assignment governs. For every new or changed rule in the diff, find the component `docs/system-design.md` assigns it to, then check where its tests landed:

- [ ] A rule the design doc assigns below the boundary (a domain or service seam) has a unit test at that seam. Covering it only through a framework-booted test (a web-layer slice, a container-backed integration test) is a `blocked` finding, severity per impact, carrying `bar_clause: "tested-as-spec"`. Green coverage does not excuse it.
- [ ] A rule the design doc assigns to the boundary layer — request binding, normalization, response shaping — is correctly exercised at that layer. That its arithmetic could be extracted is never a finding by itself; the pyramid's question applies to rules the design assigns below the boundary.
- [ ] When the design doc assigns a lower seam and the rule landed in a handler, controller, or adapter instead, the cause is placement, not testing. Raise the finding against the test location and name the placement cause; the code-quality-reviewer's Design Placement check owns the landing layer.
- [ ] A production helper widened so a framework test can reach a rule the design assigns to the boundary is a `blocked` finding. The rule is tested where the design places it, never widened for the test.

- [ ] A client's suite that repeats a collaborator's case table is a finding carrying `bar_clause: "tested-as-spec"`. The cases belong in the collaborator's own suite; the client keeps one representative path. A rule reachable through no public seam is a placement cause, not a testing gap; name it and leave the landing layer to the code-quality-reviewer's Design Placement check.
- [ ] A test no plausible change to the code would fail is a finding with the same weight as a missing test. It costs reading time on every change and catches nothing. A missing-test finding names the change it would catch.
- [ ] A suite reaches a unit through the module a client imports, never through the file that holds a helper. A test coupled to a file layout moves with every refactor, and that coupling is the finding.
- [ ] On a delta that preserves behavior, more test lines rewritten than source lines changed is a finding against the tests, with the weight of a missing test.

Judge placement against the components `docs/system-design.md` assigns first and `docs/testing-principles.md` § Test Pyramid second, never a remembered ratio. A brief sentence that reads unconditionally yields to the design doc's assignment; a conflict between the two briefs is a `clarify` to the system-design-expert. Every placement finding cites the assignment it enforces.

## Test Quality Checklist

The principles below are language-agnostic. Each **Stack-specific rules** slot is where this project records the test conventions its framework imposes.

### Test Coverage
- [ ] A hand-written structure or algorithm the brief's § Scale and Load records has a test per case in its case table. The table covers empty, one, many, duplicates, boundary sizes, and the access pattern the row names. A hand-written structure's bugs sit at its edges (`tdd-principles` § Fit for the Workload)
- [ ] On a fix round, the fix this review asked for stays on the slice's routes. A delta on a route or flow the slice's bullets do not name is `clarify` to `product-requirements-expert` on `changes_requested`, never approved through (review-workflow tag rule). A bullet the slice does not carry has no test to review
- [ ] Every decision path of a public seam has one test, at the unit that owns it; a second test through the same equivalence class is a finding
- [ ] Boundaries are tested where the code compares, and the error paths the design names once each

### Test Structure
- [ ] Each test reads as Arrange / Act / Assert with one behavior under test
- [ ] Related cases are data-driven rather than copy-pasted
- [ ] Test names follow the brief's naming school (§ Test Naming) and the `test_name_pattern` floor in `scripts/layout.toml`
- [ ] Edge cases are included alongside the nominal case — `python3 scripts/grading.py coverage-map --feature <req_id>` lists the PRD group's numbered cases and the declared tests; cite the map in the finding. A listed case is a prompt to read the tests, never a finding by itself; the finding is a case the slice's requirement owns that no test covers
- [ ] Every Done-when bullet of the slice's requirement has a test whose name states it; the map lists the bullets
- [ ] Test bodies are straight-line — no branches or loops beyond the data-driven mechanism (`tested-as-spec`)
- [ ] Test data names meaningful values by role and marks irrelevant ones; no bare literals (`tested-as-spec`)
- [ ] Comments explain WHY; none narrate what the code or the data already shows; a comment a better name would make redundant is a rename; no requirement ids or edge-case numbers in test comments (`legible-cold`)
- [ ] Construction follows the brief's § Test Data Construction: the type's one entry point and its `with` copies, an irrelevant instance behind the suite's named default; the suite's existing helpers, fixtures, and defaults are reused before new ones are added; a brief that still prescribes test-owned factories binds until it changes. List raw constructions of domain types in the changed test files with `python3 scripts/grading.py conventions-map` (the stack's constructor syntax is the layout's `[conventions]` table), which also lists the literal-bearing lines; each with unnamed arguments, or one filling mandatory parameters inline where a named default exists, is an `autofix` finding, severity `fixable`
- [ ] The brief binds where it speaks (naming school, mocking policy, data tiers, construction); a host file that predates the brief is debt, not a pattern; host conventions apply only where the brief is silent, and copied setup is renamed to its actual role (`consistent-with-codebase`)
- [ ] Every new fake or stub matches the double the `design-block` or a later `consultation-response` names for that boundary; one neither names is an `autofix` finding, severity `fixable`
- [ ] **Stack-specific rules:** {{FILL: parameterized-test idiom, subtest mechanism}}

### Useful Failure Messages
- [ ] The failure names the operation under test
- [ ] The failure shows actual inputs and got-versus-want
- [ ] Composite values are compared with a structural diff, not field-by-field prose
- [ ] **Stack-specific rules:** {{FILL: assertion/diff helper for this stack}}

### Test Helpers
- [ ] Helpers are marked so a failure points at the calling test, not the helper
- [ ] Teardown is registered to run automatically, not left to manual cleanup
- [ ] Helpers return values for the test to assert on; they do not assert themselves
- [ ] Setup failures abort the test; only behavior failures are soft failures
- [ ] **Stack-specific rules:** {{FILL: helper/teardown mechanism}}

### Mocking Policy

The policy is the brief's (§ Mocking Policy). Apply its boundary rule: mock at system boundaries, use real implementations within them.

| Collaborator | Mock? | Why |
|---|---|---|
| External service / network client | Yes | System boundary |
| Internal types | No | Use the real implementation |
| Clock / time / randomness | Yes | Deterministic testing |

The governing principle is `tested-as-spec`: a test asserts observable outcomes, and a mock interaction is asserted only where the interaction itself is the contract.

- [ ] An outcome compares a whole expected object where one can be built (the brief's whole-object comparison). A recorded argument or a picked-field chain in its place is a finding
- [ ] A test constant that copies a production literal (a page size, a limit) is hidden coupling: read the production constant, or assert through the behavior the literal shapes
- [ ] **Stack-specific rules:** {{FILL: mocking/faking tools, boundary seams}}

## Security Testing Requirements

### Boundary Testing
- [ ] Negative and empty inputs (negative numbers, empty strings, null/absent)
- [ ] Overflow conditions (maximum values, very long inputs)
- [ ] Type mismatches (wrong types in structured input)
- [ ] Special characters in string inputs
- [ ] Unicode edge cases (null bytes, right-to-left characters)

### Error Path Testing
- [ ] All error paths have test coverage
- [ ] Error messages do not leak sensitive data
- [ ] Timeout behavior is tested
- [ ] Resource cleanup on error is verified

### State and Idempotency Testing
- [ ] First run creates output
- [ ] Second run with no changes produces identical output
- [ ] New items are detected and processed

### Concurrency Testing
- [ ] Concurrent access patterns are tested
- [ ] Concurrent-unit cleanup is verified
- [ ] Shared-state operations cannot deadlock or race
- [ ] **Stack-specific rules:** {{FILL: race/concurrency tooling, if any}}

### Input Validation Testing
- [ ] Malformed structured input is rejected
- [ ] Invalid patterns are handled, not crashed on
- [ ] Out-of-range values are rejected
- [ ] Missing required fields are caught

## Dynamic Analysis

Run the test verb through the gate (`scripts/gate.sh test`); static analysis rides the gate's lint verb, which the reviewer never re-runs. Where the stack offers stronger dynamic tools, bind them into the relevant verb and require them:

- [ ] Race / data-race detection, if the stack provides it
- [ ] Static analysis blocks merge on failure (via `scripts/gate.sh lint`)
- [ ] Fuzzing for input-parsing code, if the stack provides it — parsers, pattern compilation, and untrusted-input decoders are the priority targets
- [ ] **Stack-specific rules:** {{FILL: race detector, fuzzer, static analyzer commands}}

## Test Organization

- [ ] Test files follow the location and naming declared in `scripts/layout.toml`
- [ ] Integration tests are separated from unit tests by the stack's mechanism
- [ ] Fixtures live in a dedicated test-data directory with descriptive names (`valid_input`, `malformed_response`, `empty`)
- [ ] **Stack-specific rules:** {{FILL: test-file layout, integration-test separation}}

## Common Issues to Flag

### [AUTOFIX] Issues
- Helper not marked, so failures point at the helper instead of the test
- Sleep-based synchronization instead of a deterministic wait
- Hardcoded values that belong in a data-driven case
- Missing error case in a data-driven test

### [ESCALATE] Issues
- No concurrent-access testing for shared state
- Missing integration test for an external service
- A decision path of a public seam with no test, or a second test through the same equivalence class (§ Coverage)

### [CLARIFY:security-reviewer] Issues
- Test exposes sensitive-data handling patterns
- Error-message content needs security review