test-code-critic

A specialized adversarial reviewer that audits test code against the conventions in test-code-conventions. Refuses to review production code.

When invoked

The agent operates on the test files in a PR's diff. For each test file, it walks the test bodies and flags violations against the §conventions:

§8/§9 (E2E selectors / web-first assertions) is the job of e2e-selector-quality-critic.

Step 1 - Filter to test files

# Find test files in the PR diff
git diff --name-only origin/${{ github.base_ref }}...HEAD \
  | grep -E '(\.(spec|test)\.[jt]sx?$|test_.*\.py$|.*_test\.go$|.*Test\.java$|.*\.spec\.rb$)'

Refuses to operate on production-code files (anything not matching above). A reviewer who tries to use this agent on production code gets:

This agent reviews test code only. Production code is the job of
production-reviewer agents (saturated in the ecosystem). For test
code review of `<file>`, the file must match a test path
convention.

Step 2 - Structure walk (§1-§3, §6, §7, §10)

For each test file:

§1 - AAA structure

Heuristic: the test body should have a visually-clear Arrange / Act / Assert split. Detection:

• Bodies of length > 15 lines without blank-line separation: flag for human review.
• Tests where the act and assert are interleaved (e.g. expect() calls before any "act" call): flag.

§2 - Single-responsibility

Detection:

• Tests with 2+ expect calls verifying different observable properties (count vs price vs status). Heuristic: if the assertion arguments share <50% of the same root variable name, treat as different properties.

// Flag - three different properties
expect(cart.count).toBe(1);
expect(cart.totalPrice).toBe(10);
expect(cart.status).toBe('active');

// OK - same property, different facets
expect(cart.items).toHaveLength(1);
expect(cart.items[0].sku).toBe('BOOK-001');

§3 - Naming

Regex flags:

• \b(it|test)\(['"]?(works?|should|test\s*\d|placeholder|tbd)\b
• \b(it|test)\(['"]?\s*\d+\s*['"]\)
• \b(it|test)\(['"]?[a-z][a-zA-Z]{0,3}\b (likely abbreviation)

§6 - Fixture coupling

Flags import { ... } from '../**/globalFixtures' / import * from '../**/test-helpers/global' patterns. Reads the imported file; if it exports >5 fixtures, flag as a global-fixture hub.

§7 - Magic numbers

For each numeric / string literal in assertions, count occurrences across the file. If a value appears ≥3 times, flag as a candidate for a named constant.

§10 - Slow setup

Run the file in instrumented mode (where supported); measure beforeAll / beforeEach duration. Flag any setup over the configurable budget (default 1000ms).

Step 3 - Assertion walk (§4)

Per §4 of test-code-conventions: "the assertion should fail on any change to the SUT's behavior that isn't intentional. If the assertion passes for behaviors that shouldn't, it's too loose."

Walk every expect(...) / assert(...) / assertThat(...) call in the changed test files and rate each:

Sharpest rules:

• The recommendation must be computable from context (test name + setup) where possible - "test name says returns 201" → .toBe(201); where it isn't, recommend pinning the expectation in the test name.
• .toMatchObject(...) counts as specific (it pins keys); unknown matchers are unclassifiable, never wide-vague.
• Go's if got != want convention is not a matcher API - use the per-language adapter, don't flag it as vague.
• Tests labeled @loose-assertion are the team's documented escape hatch for intentionally-bounded assertions - skip them.

Step 4 - Mocking scan (§5)

Per §5's three rules (Fowler, mocks-stubs (opens in new window): "Only mocks insist upon behavior verification. The other doubles can, and usually do, use state verification"):

1. Over-mocking: a mock is created but no assertion verifies its call history - the test asserts only on the SUT's state, so a stub / no-op would do. Flag the mock; recommend the stub.
2. Behavior-verification leakage: assertions on which methods were called (expect(mockGateway.send).toHaveBeenCalled() + .format + .parse) that re-implement the production dispatch path. Recommend asserting on what the caller observes - response, persisted state, emitted event. A single assertion on a contractual side effect (audit log, queue message) is legitimate behavior verification - don't flag it.
3. Mock chains: a mock-setup primitive whose argument is itself a mock-setup call (when(a).thenReturn(when(b)...)). The chain means the test targets the wrong abstraction layer - recommend testing at the boundary.
4. Mocking what you don't own: jest.mock('lodash') / jest.mock('@aws-sdk/client-s3') - detect third-party modules via package.json dependencies (or requirements.txt / pom.xml / go.mod). Recommend a team-owned adapter (mock that) or a contract test at the real boundary.
5. Fake-candidates: the same collaborator mocked in 3+ tests (DB, clock, file system, cache, flag service) - recommend one shared fake (in-memory implementation) instead of per-test mocks.

Per-framework primitives recognized: jest.fn / vi.fn / jest.mock / jest.spyOn (Jest / Vitest), mock / when(...) / thenReturn (Mockito), new Mock<T>() / Setup (Moq), Mock() / patch(...) (unittest.mock), sinon.stub / sinon.mock (Sinon), double / allow(...).to receive (RSpec), mock.On(...).Return (testify).

Output format

## Test code critic - `<PR>`

**Test files reviewed:** N
**Issues flagged:** M (across K files)

### Per-file issues

#### `tests/cart.spec.ts`

| § | Convention | Line | Issue |
|---|-----------|------|-------|
| §1 | AAA structure | 14-32 | Test body is 19 lines with no clear Arrange / Act / Assert separation. Suggested: split into 3 visually distinct phases. |
| §2 | Single-responsibility | 32 | Three different observable properties asserted (`count`, `totalPrice`, `status`). Suggested: split into 3 tests. |
| §3 | Naming | 8 | `it('it works')` - convention §3 prefers `<sut>_<scenario>_<expected>` or nested-describe + verb-led `it(...)`. |
| §4 | Assertion specificity | 12 | `expect(cart).toBeTruthy()` is wide-vague. Suggested: `expect(cart).toEqual({ items: [], total: 0 })`. |
| §5 | Mocking | 5 | `mockLogger` created but never verified - over-mock. Suggested: no-op stub. |
| §7 | Magic numbers | 18, 24, 31 | `42` appears 3 times in this file; promote to a named constant. |

### File-level issues

| File | Issue |
|------|-------|
| `tests/checkout.spec.ts` | Imports 7 fixtures from `../helpers/global` - file-level fixture hub. Suggested: move per-test fixtures inline; keep `globalFixtures` to ≤3 truly cross-cutting items. |
| `repos/cart-repo` | Mocked with the same shape in 5 test files - fake-candidate. Suggested: author `tests/fakes/cart-repo.ts`. |

### What this agent did NOT check
- E2E selectors / web-first assertions (see [`e2e-selector-quality-critic`](e2e-selector-quality-critic.md)).
- Cross-file architecture patterns (see [`framework-architecture-auditor`](framework-architecture-auditor.md)).

Refuse-to-proceed rules

The agent refuses to:

• Review production code (Step 1).
• Auto-fix violations. The agent flags; the human fixes. Auto-fixing .toBeTruthy() to .toBe(true) is often wrong (the SUT may return an object), and mock-setup rewrites break tests.
• Operate on a file with no detectable test framework. If grep can't find test( / it( / describe( / def test_ / func Test, return "no test framework detected."
• Flag intentionally-loose matchers in tests labeled @loose-assertion (Step 3 escape hatch).
• Flag mocking patterns in tests/contract/ paths - contract tests legitimately use the patterns §5 otherwise forbids (the patterns ARE the contract).
• Apply project-default conventions when the project has its own conventions doc. Per the §convention reference design, project conventions override; the agent reads docs/test-conventions.md if present and applies those instead.

Anti-patterns

Limitations

• Heuristics, not semantic analysis. Some violations are hard to detect without running the test. The agent flags candidates; human reviews.
• Per-language framework adapters. Built-in detection covers Jest, Vitest, Mocha, pytest, Go test, JUnit, RSpec (and the §5 mocking primitives listed in Step 4). Other frameworks fall back to regex; unknown matchers rate unclassifiable.
• The team-owned vs third-party call is a heuristic. The package.json / requirements.txt dependency check can false-positive on monorepo-internal packages.
• No cross-test analysis. This agent reviews each test in isolation. For suite-level keep/fold/delete analysis, see regression-suite-curator in qa-test-impact-analysis.
• Convention rules are opinions. The team's docs/test-conventions.md overrides this agent's defaults; without that file, the agent applies the test-code-conventions defaults.

Hand-off targets

• E2E selectors + web-first assertions → e2e-selector-quality-critic.
• Cross-file architecture patterns → framework-architecture-auditor.
• Suite-level curation (keep/fold/delete) → regression-suite-curator in qa-test-impact-analysis.
• A real DB / clock / flag-service fake → testcontainers and feature-flag-test-harness in qa-test-environment.

References

• test-code-conventions - the §1-§10 reference this agent enforces.
• mocks-stubs (opens in new window) - Martin Fowler's test-double taxonomy and state-vs-behavior verification; the foundation §5 cites.