evals/eval.json
{
"skills": [
"reviewing-test-quality"
],
"scenarios": [
{
"query": "Review this change:\n```java\n@Test\npublic void sendsWelcome() {\n UserService svc = new UserService(repoMock, mailerMock, auditMock);\n when(repoMock.insert(any())).thenReturn(new User(1L));\n svc.register(\"a@b.com\");\n verify(repoMock).insert(any());\n verify(auditMock).log(eq(\"register\"), any());\n verify(mailerMock, times(1)).send(any());\n}\n```",
"expected_behavior": [
"Flags the implementation-coupled assertions: the test only verifies which mocks were called, not any observable outcome",
"Notes a refactor would break it while real bugs (wrong email address/content, wrong user state) would pass",
"Recommends asserting behavior (e.g. the recipient and content of the sent mail, the persisted user) with real/fake collaborators"
]
},
{
"query": "Review this change:\n```js\nlet cache = {};\ndescribe('pricing', () => {\n it.only('applies discount', async () => {\n cache.rate = await fetchRateFromApi(); // real network call\n expect(price(100, cache.rate)).toBeLessThan(100);\n });\n it('uses cached rate', () => {\n expect(price(100, cache.rate)).toBeDefined();\n });\n});\n```",
"expected_behavior": [
"Flags the focused test (it.only) which silently disables the rest of the suite",
"Flags the real network call making the test nondeterministic/flaky (stub or inject the rate)",
"Flags the shared mutable cache creating order dependence between the two tests",
"Notes the weak assertions (toBeLessThan(100) / toBeDefined) would not catch a wrong discount"
]
},
{
"query": "Review this change:\n```python\ndef test_parse_quantity_rejects_zero():\n with pytest.raises(InvalidQuantity):\n parse_quantity(\"0\")\n\ndef test_parse_quantity_accepts_minimum():\n assert parse_quantity(\"1\") == Quantity(1)\n\ndef test_parse_quantity_rejects_garbage():\n with pytest.raises(InvalidQuantity):\n parse_quantity(\"banana\")\n```",
"expected_behavior": [
"Reports no findings",
"Recognizes deterministic, isolated, behavior-level tests covering the boundary (0 vs 1) and the error path",
"Does not demand mocks, more tests, or restructuring of already-good tests"
]
},
{
"query": "Review this change:\n```python\ndef test_report():\n data = load_fixture(\"sample_report.csv\") # fixture defined elsewhere\n r = build_report(data)\n assert r.total == 1500\n assert r.currency == \"USD\"\n assert r.rows[0].name == \"Acme\"\n assert len(r.rows) == 12\n assert r.generated_at is not None\n```",
"expected_behavior": [
"Flags Assertion Roulette: several unrelated assertions with no messages, so a failure does not say which expectation broke — give each a message or split into focused tests with a single reason to fail",
"Flags the Mystery Guest: the test depends on an external fixture file (load_fixture(\"sample_report.csv\")) whose contents are not visible in the test, obscuring what is exercised and coupling the test to a file on disk",
"Does not demand mocks for build_report"
]
},
{
"query": "Review this change:\n```js\ntest('discount pricing', () => {\n const cases = [[100, 'gold', 80], [50, 'silver', 47.5], [0, 'none', 0]];\n for (const [amount, tier, expected] of cases) {\n if (tier === 'none') {\n expect(price(amount, tier)).toBe(amount);\n } else {\n expect(price(amount, tier)).toBeCloseTo(expected);\n }\n }\n});\n```",
"expected_behavior": [
"Flags Conditional Test Logic: the if/else and loop inside the test mean a branch can silently not run and a failure does not identify which case broke",
"Recommends a readable, table-driven form (e.g. it.each / parameterized cases) with one case per row so each failure localizes the cause",
"Notes the single test bundles several behaviors (not atomic / single reason to fail)"
]
}
]
}
examples.md
# Examples — reviewing-test-quality
A test diff often contains several independent problems. Check every line against
the checklist and report each distinct issue as its own numbered finding. When the input is correct, the entire response is exactly "No findings" — never produce a numbered list of findings for correct code.
## Bad → finding
**Input (diff):**
```js
test("processOrder", () => {
const validator = { check: jest.fn().mockReturnValue(true) };
const repo = { save: jest.fn() };
const mailer = { send: jest.fn() };
processOrder(order, validator, repo, mailer);
expect(validator.check).toHaveBeenCalledTimes(1);
expect(repo.save).toHaveBeenCalledBefore(mailer.send);
});
```
**Expected finding:**
1. **Over-mocked, implementation-coupled test:** every collaborator is a mock and
the assertions only verify which internals were called and in what order — any
refactor breaks it while a real bug (wrong total, wrong recipient) passes.
Assert observable behavior instead: the saved order's state and the email's
recipient/content, using real or fake collaborators.
2. **No behavior assertion at all:** nothing checks an output or side effect a
user/caller could observe.
## Bad → finding
**Input (diff):**
```python
processed = []
@pytest.mark.skip(reason="flaky")
def test_expiry():
assert is_expired(make_token(ttl=1), now=datetime.now())
def test_batch():
processed.append(process(next_item())) # appends to module-level list
assert len(processed) == 1 # passes only if run first
```
**Expected finding:**
1. **Shared mutable state:** `test_batch` appends to a module-level list, so it
passes only when run first/alone (order dependence) — isolate with a fixture.
2. **Real clock:** `test_expiry` uses `datetime.now()` — inject/freeze time.
3. **Skipped-as-flaky:** a skipped flaky test is an untested code path; fix the
nondeterminism (the clock) that made it flaky instead of parking it.
## Good → no finding
**Input (diff):**
```python
def test_refund_rejected_after_30_days():
# regression test for #842
order = make_order(paid_at=fixed_now - timedelta(days=31))
result = request_refund(order, now=fixed_now)
assert result.rejected
assert result.reason == RefundReason.WINDOW_EXPIRED
def test_refund_allowed_on_day_30():
order = make_order(paid_at=fixed_now - timedelta(days=30))
assert request_refund(order, now=fixed_now).approved
```
**Expected finding:** None — behavior-level assertions, injected clock
(deterministic), both sides of the boundary covered, regression test linked to its
bug. Report "No findings". Do NOT demand mocks for collaborators that are already
fast and deterministic, and do NOT ask for more tests when the changed behavior's
branches and boundary are covered — coverage padding is not a finding.
reference/heuristics.md
# Reviewable heuristics — reviewing-test-quality
## Contents
- From category #17
## From category #17
### Reviewable heuristics (skill-checklist seeds)
- Do new/changed tests assert **observable behavior** (inputs→outputs, side effects), not internal calls/private state (refactor-resistant)?
- Is coverage **meaningful** on the new code — branches and edge cases, not just lines executed? Don't chase a % with assertion-free or **tautological** tests (asserting the mock, restating the framework, or a **Sensitive Equality** check on a whole serialized blob that breaks on unrelated change), and don't keep tests that pin no real requirement (Farley *necessary*).
- Bug fix → is there a **regression test** that fails before the fix and passes after?
- Are tests **isolated and deterministic** — no shared mutable state, order dependence, or real clock/network/unseeded random (flaky risk)?
- Is the test at the **right level** (pyramid/trophy) — logic in fast unit/integration, e2e reserved for critical journeys?
- **Over-mocking smell**: do mocks assert on implementation calls so a refactor breaks tests without behavior changing? Reach for the least-powerful **test double** — prefer a real collaborator, fake, or stub, and reserve a behavior-verifying mock for true outgoing commands (don't mock queries or value objects) (cross #11).
- Are **edge/boundary** cases covered (empty, null, max, error paths) — where the bugs live? Walk the **CORRECT** dimensions (Conformance, Ordering, Range, Reference, Existence, Cardinality, Time) to surface the missing edge (cross #1).
- Would the suite **catch a real bug**, not just execute lines? Apply mutation intuition — for a pure, deterministic, fast-to-test unit, prefer actually running a mutation tool (cheap, high-signal) over eyeballing it; otherwise high coverage masks weak assertions.
- Any disabled/focused/skipped tests (`.only`, `xit`, `@Disabled`) sneaking in?
- For nondeterministic/concurrent code, is the invariant property-tested and the concurrent path exercised (cross #3)?
- Is each test readable — clear arrange/act/assert, one behavior per test, name reads as a spec?
- Does each test have a **single reason to fail** — assertions that localize the cause when it breaks? Flag **Assertion Roulette** (a pile of asserts with no messages, so a failure doesn't say which expectation broke) and tests that bundle several unrelated behaviors (Beck *specific*; Farley *atomic*).
- Is the test **self-contained**, or a **Mystery Guest** — does it lean on a hidden external resource (file, DB, network) or a fixture defined far from the test, obscuring what's exercised and risking nondeterminism (cross isolation)?
- Is the test **readable and descriptive** rather than over-DRY'd — does **Conditional Test Logic** (branches/loops in the test) or deep helper/fixture indirection hide what actually runs? In tests, readability beats deduplication (Beck *writable*).
- Beyond the happy path, does the suite use **inverse/round-trip** checks (encode→decode), a **cross-check** against an independent oracle, and **forced error conditions** (Right-BICEP) — not a single positive assertion?
---
reference/sources.md
# References to mine — reviewing-test-quality
## Contents
- From category #17
## From category #17
### Key references
- **Mike Cohn — *Succeeding with Agile* (the Test Pyramid)** → mine: more fast unit tests, fewer slow e2e; the cost/speed/stability gradient by test level.
- **Kent C. Dodds — "The Testing Trophy" (2018)**, building on **Guillermo Rauch — "Write tests. Not too many. Mostly integration."** → mine: integration tests give the best ROI — they test units collaborating without e2e fragility. The modern counter to a unit-heavy pyramid; use it to resist *both* over-mocked unit tests and over-heavy e2e.
- **Michael Feathers — *Working Effectively with Legacy Code*** → mine: "legacy code is code without tests"; seams, characterization tests, and **testability as a design property** (if it's hard to test, the design is the problem).
- **Claessen & Hughes — "QuickCheck: Lightweight Tools for Random Testing of Haskell Programs" (ICFP 2000)** → mine: **property-based testing** — assert invariants over generated inputs; the trio of *generators + properties + shrinking*. Ported as Hypothesis/fast-check/jqwik.
- **Mutation testing (PIT, Stryker, mutmut, cargo-mutants)** → mine: a **coverage-quality** signal — does the suite actually *catch injected bugs*? High line coverage + low mutation score = weak assertions. Cheapest and highest-signal on **pure, deterministic, fast-to-test** units (no I/O), where a run is minutes and surviving mutants are an exact list of unasserted behavior — there, prefer running the tool over only intuiting it.
- **"Test behavior, not implementation" (Dodds; Kent Beck)** → mine: tests coupled to internals break on refactor; assert observable behavior so tests survive refactoring (cross #21).
- **Martin Fowler — "Eradicating Non-Determinism in Tests"** `(verify URL)` → mine: flaky tests destroy trust; quarantine, then fix the root cause (time, order, concurrency, shared state).
- **Kent Beck — "Test Desiderata" (2019)** — https://kentbeck.github.io/TestDesiderata/ → mine: twelve properties a good test balances — isolated, composable, deterministic, fast, writable, readable, **behavioral**, **structure-insensitive**, automated, **specific**, predictive, inspiring. The *behavioral + structure-insensitive* pair is the canonical name for the behavior-vs-implementation axis (cross over-mocking); the under-surfaced ones are *specific* (a failure localizes its cause), *writable* (cheap to write), and *predictive* (green ⇒ prod works). Beck frames the twelve as sliders to trade off, not maxima — don't demand all twelve of every test.
- **Dave Farley — "Properties of Good Tests" (*Modern Software Engineering*, Manning, 2021)** — https://www.davefarley.net/ (homepage; the seven-property list is enumerated in the book, not on a single web page) → mine: a good test is **Understandable** (asserts behaviour, not implementation), **Maintainable**, **Repeatable** (deterministic), **Atomic** (one reason to fail), **Necessary** (no redundant/tautological tests), **Granular**, and **Fast** — *seven* properties, not the "eight" some third-party summaries cite (the 8th, "test-first", is not in Farley's own list). *Atomic* and *Necessary* are the crisp net-new additions over Beck.
- **Hunt, Thomas & Langr — *Pragmatic Unit Testing*** → mine: the actionable mnemonic family. **Right-BICEP** = what to test (Right results, Boundary conditions, Inverse relationships, Cross-check against an independent oracle, Error conditions, Performance). **CORRECT** = boundary-condition enumeration (Conformance, Ordering, Range, Reference, Existence, Cardinality, Time) — the sharpest checklist for *which* edges. **FIRST** (Fast, Isolated, Repeatable, Self-validating, Timely — coined by Ottinger & Schuchert, popularized in *Clean Code* ch. 9) and **A-TRIP** overlap our isolation/determinism checks.
- **Gerard Meszaros — *xUnit Test Patterns* (2007)** + **van Deursen, Moonen, van den Bergh & Kok — "Refactoring Test Code" (XP2001)** + **tsDetect (Peruma et al., FSE 2020)** — http://xunitpatterns.com/Test%20Smells.html → mine: the named **test-smell** catalog that turns "this test smells" into specific, often mechanically-detectable findings — **Assertion Roulette** (many unexplained asserts; a failure doesn't localize), **Mystery Guest** (depends on a hidden external file/DB/fixture — not self-contained), **Eager Test** (one test exercises many behaviors), **Conditional Test Logic** (branches/loops in the test hide what runs), **Sensitive Equality** (asserts on a whole toString/serialized form — brittle), **Sleepy Test** (real-time `sleep` — flaky), **Resource Optimism** (assumes an external resource is present). tsDetect's 19 rules show most are lintable (cross tool-rules).
- **Martin Fowler — "Mocks Aren't Stubs"** — https://martinfowler.com/articles/mocksArentStubs.html → mine: the five **test-double** kinds (dummy, fake, stub, spy, mock) and *classicist vs mockist* / *sociable vs solitary* testing. Reach for the least-powerful double — a stub or fake — and reserve a behavior-verifying **mock** for genuine outgoing commands; mocking queries or value objects is the over-mocking that couples tests to implementation (cross #11).
reference/tool-rules.md
# Tool rules to triage — reviewing-test-quality
> **Selecting tools for this stack.** The tools named below are field-tested starting points, not a mandate. Pick the one that fits this codebase's language version, build, and CI — and verify it actually runs on your toolchain before relying on it. A listed tool that is broken, abandoned, or noisy on your setup is a gap to close, not a permanent `continue-on-error`: prefer a working, maintained equivalent (often a younger, less well-known one) over a canonical-but-broken default. The capability is the requirement; the specific tool is replaceable.
## Contents
- From category #17
## From category #17
### Tooling rules worth lifting
- **Coverage:** coverage.py (Python), Istanbul/nyc & V8 (JS), JaCoCo (Java), SimpleCov (Ruby), `go test -cover`, **cargo-llvm-cov / cargo-tarpaulin** (Rust). Lift: track **branch** coverage and the coverage **delta on the diff**, not a global %.
- **Mutation:** PIT/pitest (Java), **Stryker** (JS/TS, C#, Scala — https://stryker-mutator.io/), mutmut & cosmic-ray (Python), Mutant (Ruby), **cargo-mutants** (Rust — https://mutants.rs/), gremlins (Go — https://gremlins.dev/). For a pure crate/module with fast deterministic tests, a mutation run is cheap, and the surviving-mutant list makes a good CI gate.
- **Property-based:** Hypothesis (Python), fast-check (JS/TS), jqwik (Java), **proptest / quickcheck** (Rust), PropEr/QuickCheck.
- **Flaky control:** `pytest-randomly` (random order), `pytest-rerunfailures`, Jest `--detectOpenHandles`, Gradle/Maven retry, flaky trackers (BuildPulse, Datadog Test Optimization).
- **Test linters:** `eslint-plugin-jest` (`no-disabled-tests`, `no-focused-tests`, `expect-expect`, `no-conditional-expect`), `rubocop-rspec`, `flake8-pytest-style`.
SKILL.md
---
name: reviewing-test-quality
description: 'Reviews tests for quality: behavior vs implementation coupling, over-mocking,
meaningful branch/edge coverage on the diff, regression tests for bug fixes, isolation
and determinism (no shared state, real clocks, or unseeded randomness), right level
per the pyramid/trophy, and disabled/focused tests sneaking in. Use when reviewing
test files, test coverage, mocks, fixtures, flaky tests, or a bug fix''s tests.'
provenance:
taxonomy_version: v0.9
built_from:
- category: 17
source: docs/research/cluster-5-verification.md#17
hash: f0616b31db4d8d43239b0d9bf58badf4d25568a4f6d45542055ea74b168ad174
---
# reviewing-test-quality
*Do the tests prove anything? Behavior coupling, over-mocking, edge coverage, determinism.*
## When to use
Reviews tests for quality: behavior vs implementation coupling, over-mocking, meaningful branch/edge coverage on the diff, regression tests for bug fixes, isolation and determinism (no shared state, real clocks, or unseeded randomness), right level per the pyramid/trophy, and disabled/focused tests sneaking in. Use when reviewing test files, test coverage, mocks, fixtures, flaky tests, or a bug fix's tests.
**Shape: diff.** Written for concrete code; not meant for design docs or plans.
## Reviewer discipline
Report only real problems. If the code correctly handles the case, reply "No findings" and stop — do not invent issues. This guards against false positives on correct code; still report every genuine issue you do find, with its full detail.
**Defects are the default; improvements are opt-in.** By default this lens is defect-only: do not suggest changes to code that is already correct. When the team has opted up into improvement suggestions, a finding on already-correct code is admissible only as `nit`-severity, `route: implementer` (the author applies, defers, or ignores), and must clear the non-configurable anti-churn floor: it must genuinely *improve* — never offer a merely equivalent alternative — and must converge (once a dimension is as good as you can confidently make it, stop; never oscillate A→B then B→A, never re-order to an equivalent state). Defects keep the strict bar above regardless of this setting.
**Team preferences.** If the reviewed repo has `.code-quality-atlas/preferences.md`, apply it before reporting: a repo's `.code-quality-atlas/preferences.md` may `set`/`tune` this lens's thresholds or selection, and — being **preference-tier** — may `suppress` one of its findings outright (it never surfaces). Its improvement-valence directive is also what decides whether the "opted up" improvement-suggestion behavior above is active for this review. Absent the file, apply this lens's defaults exactly as written above.
**Pre-existing defects in touched code are surfaceable, not yours to fix.** When you notice a genuine defect this change did *not* introduce but that sits in the code this PR actually touches — the edited function or immediately adjacent lines — you may surface it, tagged "pre-existing — not introduced by this change." Like improvements it is opt-in and default-quiet (off unless the team opts up), `route: implementer`, and non-blocking: it informs the author's fix-now / file-a-ticket / ignore call and never sets this PR's verdict, because the diff did not cause it. Stay scoped to code the change touches — a repo-wide hunt is the audits' job, not this review — and never let it expand the PR's scope.
## Top checks
The head of the full checklist — enough for a first pass without opening any reference file:
- Does each test have a **single reason to fail** — assertions that localize the cause when it breaks? Flag **Assertion Roulette** (a pile of asserts with no messages, so a failure doesn't say which expectation broke) and tests that bundle several unrelated behaviors (Beck *specific*; Farley *atomic*).
- Do new/changed tests assert **observable behavior** (inputs→outputs, side effects), not internal calls/private state (refactor-resistant)?
- Is coverage **meaningful** on the new code — branches and edge cases, not just lines executed? Don't chase a % with assertion-free or **tautological** tests (asserting the mock, restating the framework, or a **Sensitive Equality** check on a whole serialized blob that breaks on unrelated change), and don't keep tests that pin no real requirement (Farley *necessary*).
- Bug fix → is there a **regression test** that fails before the fix and passes after?
- Are tests **isolated and deterministic** — no shared mutable state, order dependence, or real clock/network/unseeded random (flaky risk)?
- Is the test at the **right level** (pyramid/trophy) — logic in fast unit/integration, e2e reserved for critical journeys?
- **Over-mocking smell**: do mocks assert on implementation calls so a refactor breaks tests without behavior changing? Reach for the least-powerful **test double** — prefer a real collaborator, fake, or stub, and reserve a behavior-verifying mock for true outgoing commands (don't mock queries or value objects) (cross #11).
- Are **edge/boundary** cases covered (empty, null, max, error paths) — where the bugs live? Walk the **CORRECT** dimensions (Conformance, Ordering, Range, Reference, Existence, Cardinality, Time) to surface the missing edge (cross #1).
- Would the suite **catch a real bug**, not just execute lines? Apply mutation intuition — for a pure, deterministic, fast-to-test unit, prefer actually running a mutation tool (cheap, high-signal) over eyeballing it; otherwise high coverage masks weak assertions.
## Mechanizing these checks
Where a finding here is one a tool can catch deterministically, surface that as an advisory `route: implementer` note next to the finding: the hand review caught it this time, and wiring the matching tool from [reference/tool-rules.md](reference/tool-rules.md) into CI gates it going forward. This is a suggestion to mechanize, not a defect — it never blocks a verdict, and it falls away on a repo that already runs the tool.
## Going deeper
- [reference/heuristics.md](reference/heuristics.md) — the full checklist; open it when the change sits squarely in this lens's domain.
- [examples.md](examples.md) — concrete good/bad findings, and the output format to match.
- [reference/tool-rules.md](reference/tool-rules.md) — static-analysis rules covering the mechanical subset; for wiring up linters, not needed for the judgment review itself.
- [reference/sources.md](reference/sources.md) — the research behind each check; for provenance, not needed during a review.