test-review

Files (4)

• SKILL.md
• DETECTION-PLAYBOOK.md · references/
• QUALITY-FILTER.md · references/
• REVIEW-VOICE.md · references/

SKILL.md (first lines)

# Test Review

The testing lens of the suite. Layered on top of [`pr-review`](https://github.com/priyank766/OpenSource-SKILL/blob/main/skills/pr-review/SKILL.md) —
that skill's diff isolation, context building and posting rules still apply. This one
replaces the line-by-line audit with a test-specific one.

**Read first:** [`DETECTION-PLAYBOOK.md`](references/DETECTION-PLAYBOOK.md) ·
[`QUALITY-FILTER.md`](references/QUALITY-FILTER.md) ·
[`REVIEW-VOICE.md`](references/REVIEW-VOICE.md)

> **Where the time goes.** Finding the flaw is the review. The playbook above is the
> general flaw-hunting procedure; this file is the domain-specific lens on top of it.
> Scoring and writing the comment are the last ten minutes, not the work.

**Severity floor: 8.0.** Comment budget: one per ~200 changed lines, max 5.

---

## The governing rule

A test has exactly one job: **fail when the behaviour breaks.** Everything else — naming,
structure, arrangement, parametrisation style — is decoration on top of that job, and none
of it clears 8.0 on its own.

The decisive technique is **mutation thinking**. For every test in the diff, break the
implementation in your head and ask whether this test notices. Delete the line the test is
supposedly about. Flip a comparison. Return an empty list instead of the real one. If the
test still passes, it is not a test, it is a ritual — and that finding scores 9.0+ no
matter how clean the test reads.

Do the real version where it is cheap: comment out the implementation line, run the new
test, confirm it fails. Thirty seconds of that outranks any amount of reading. Every
finding in this skill must be justified against the governing rule, in one sentence, before
it is written up. "This test would still pass if X broke" is a finding. "This test could be
named better" is not.

---

## Why this exists

Bad tests are the most expensive thing a PR can add, because their cost is deferred and
paid by someone else. A test that cannot fail contributes nothing but a green tick and a
false sense of cover. A flaky test costs the maintainer a re-run today and a habit of
ignoring red CI within a month — after which the suite stops protecting anything. Both are
nearly free to fix at review time and nearly impossible to fix later, once the test is load
bearing in everyone's mental model of "the suite passes".

Review is the only moment where the fix costs one comment.

### What each role gets out of it

| Role | Time this gives back |
| :--- | :--- |
| **Maintainer** | The flake never lands. No 2 a.m. re-runs, no bisecting a red `main` to a `sleep(0.1)` merged eight months ago, no triage queue of "restarted, passed, closing". |
| **Contributor** | The mutation check before you push, so the first maintainer response is "merged" and not "does this actually test anything?" |
| **Project owner** | A suite whose red means red. Once contributors trust CI, review time drops for every PR, not just this one. |

---

## Procedure

```mermaid
flowchart TD
    A[Start] --> B[1. Isolate the test diff]
    B --> C[2. Map tests to the behaviour they claim to cover]
    C --> D[3. Run the changed tests alone]
    D --> E[4. Run the suite shuffled, reversed, repeated]
    E --> F[5. Mutation check: break the impl, expect red]
    F --> G[6. Behaviour-coverage read of the changed lines]
    G --> H[7. Smell catalogue pass]
    H --> I[8. Score with the 10-point filter]
    I -->|"score below 8.0"| J[Discard or batch to an issue]
    I -->|"score 8.0 or above"| K[9. Frame in review voice]
    K --> L[10. Get explicit approval, then post]
```

### 1. Isolate the test diff

```bash
git diff --stat upstream/main...pr-<N> -- '**/test*' '**/*_test.go' '**/*.spec.*'
git diff upstream/main...pr-<N> -- tests/ conftest.py           # fixtures matter most
git diff upstream/main...pr-<N> --diff-filter=D -- tests/       # what got deleted
```

Deleted and weakened tests deserve more attention than added ones. An assertion loosened
from `== 3` to `>= 0` inside a large diff is the single highest-value thing to catch.

### 2. Map each test to a behaviour

For every new test, write down in one line the behaviour it pins. If you cannot, the test
is decorative and the mutation check will confirm it. For every non-test line the PR
changes, write down which test covers it. The gaps in that second list are your missing-test
candidates — nothing else is.

### 3. Run only the changed tests

```bash
pytest tests/test_runner.py::test_empty_items -q
go test ./pkg/runner -run 'TestEmptyItems$' -v
npx jest tests/runner.test.ts -t 'empty items'
cargo test --package runner empty_items -- --exact
```

### 4. Run for isolation, order and flake

```bash
pytest                                  # pytest-randomly shuffles by default once installed
pytest -p no:randomly                   # disable it, for the deterministic comparison run
pytest --lf                             # last-failed only, reproduces a partial run
go test ./... -shuffle=on -count=5
npx jest --randomize                    # Jest 29.5+
npx jest --runInBand                    # serial, exposes worker-isolation assumptions
cargo test -- --test-threads=1
```

Reverse order catches setup that silently depends on an earlier test:

```bash
pytest -p no:randomly --reverse         # pytest-reverse

From priyank766/OpenSource-SKILL, Apache-2.0.

More from priyank766/OpenSource-SKILL