redesign-tests

Files (36)

• SKILL.md
• acceptance.feature
• expected.md · self-test/01-baseline-red-stop/
• manifest.json · self-test/01-baseline-red-stop/
• orders.py · self-test/01-baseline-red-stop/
• test_orders.py · self-test/01-baseline-red-stop/
• expected.md · self-test/02-postapply-red-autorevert/
• manifest.json · self-test/02-postapply-red-autorevert/
• move.patch · self-test/02-postapply-red-autorevert/
• orders.py · self-test/02-postapply-red-autorevert/
• test_orders.py · self-test/02-postapply-red-autorevert/
• expected.md · self-test/03-approve-commit-on-green/
• manifest.json · self-test/03-approve-commit-on-green/
• move.patch · self-test/03-approve-commit-on-green/
• orders.py · self-test/03-approve-commit-on-green/
• test_orders.py · self-test/03-approve-commit-on-green/
• expected.md · self-test/04-reject-reverts-clean/
• manifest.json · self-test/04-reject-reverts-clean/
• move.patch · self-test/04-reject-reverts-clean/
• orders.py · self-test/04-reject-reverts-clean/
• test_orders.py · self-test/04-reject-reverts-clean/
• expected-backlog.md · self-test/05-review-seeds-backlog/
• expected.md · self-test/05-review-seeds-backlog/
• manifest.json · self-test/05-review-seeds-backlog/
• orders.py · self-test/05-review-seeds-backlog/
• test_orders_smells.py · self-test/05-review-seeds-backlog/
• expected.md · self-test/06-missing-fake-skips/
• manifest.json · self-test/06-missing-fake-skips/
• orders.py · self-test/06-missing-fake-skips/
• test_orders.py · self-test/06-missing-fake-skips/
• expected-backlog.md · self-test/07-unmanaged-mock-is-behaviour/
• expected.md · self-test/07-unmanaged-mock-is-behaviour/
• manifest.json · self-test/07-unmanaged-mock-is-behaviour/
• orders.py · self-test/07-unmanaged-mock-is-behaviour/
• test_orders_boundaries.py · self-test/07-unmanaged-mock-is-behaviour/
• README.md · self-test/

SKILL.md (first lines)

# Redesign Tests

Rewrite tests that verify **how** the code works into tests that verify **what** it does — turning
implementation-coupled, over-mocked and flaky tests into behaviour-focused ones. Work a prioritized
backlog one item at a time; **every rewrite is committed only after the developer approves the diff *and*
its coverage-equivalence claim**, reviewing it applied in their editor.

**This is behaviour-CHANGING, human-gated work — the opposite pole from `phil:refactor-tests`.**
`refactor-tests` is structure-only and can promise coverage never shrank. This skill deliberately
changes *what a test asserts*, so it **cannot** make that promise automatically. The test is the
oracle, and this skill changes the test — so **the human is the sole oracle**. A green
suite is only a secondary sanity check; it can never prove a rewrite preserved coverage. That is
why every proposal carries a **coverage-equivalence claim** and why nothing is committed without
per-diff human approval.

> **Relationship to `phil:refactor-tests`.** If the smell is a structure-only one (duplicated setup,
> missing AAA, vague name, long test), that is `refactor-tests`' job — do not handle it here. This
> skill handles only behaviour-changing smells (below). The two use **separate** backlog files and
> never collide.

## The behavioural smells in scope (and their moves)

Only these three families. If a smell is not one of these, do **not** report or rewrite it.

Each family groups rows of the Smell Catalogue in `${CLAUDE_PLUGIN_ROOT}/rules/test-patterns.md` —
the rows whose route is `/phil:redesign-tests`. Read that rule before detecting: it names the smells,
and its stance section decides which mocks are smells at all.

| Smell family | Signal | Named move(s) | What the rewrite does |
|--------------|--------|---------------|------------------------|
| **Implementation-coupling** (Overspecified Software, Sensitive Equality, Private Access) | asserts on a mock interaction with an in-process collaborator (`mock.x.assert_called`), a private member (`obj._field`), an internal call, or a `str()`/`repr()`/serialized form — not on an observable outcome | **Assert on observable outcome** | replace the how-assertion with one on the public result / persisted state / returned value |
| **Excessive-mocking** (Mock Inside a Boundary) | mocks a collaborator that could be real (or a fake exists) and asserts only on the mock | **Replace mock with real collaborator**, **Replace mock with fake** | drive the real collaborator/fake; assert on the resulting state |
| **Flakiness / determinism** (Erratic Test) | depends on `datetime.now()`, unseeded RNG, UUIDs, wall-clock, ordering, or hidden shared state | **Inject clock / static timestamp**, **Seed RNG**, **Isolate shared state** | make the test deterministic while preserving what it checks |

**A mock at an unmanaged dependency is not implementation-coupling and not excessive-mocking.** An
email gateway, a bus another team reads, a third-party API: the message the code sends it is observable
behaviour, so asserting that message *is* the observable outcome. Rewriting it to assert on state would
discard the one thing the test exists to check. **Judge by what the mock stands in for, not who owns its
type** — your own `EmailSender` port wrapping a gateway is the gateway. A mock of an owned collaborator
that stands for nothing outside the process stays in scope, and so does the flakiness family: a gateway
mock asserting a wall-clock value is still an Erratic Test.
(Fixture `07-unmanaged-mock-is-behaviour`.)

**Never in scope:** structure-only cleanup (that is `refactor-tests`); deleting dead/duplicate
tests; splitting or merging test files; changing non-test (SUT) code; concurrency-race fixes that
require production changes. When unsure whether a flag is a true behavioural smell, **skip it** —
false positives erode trust (the precision target: at most 20% rejected on first review).

## Detection (reuse `review-code` Priority 6)

Detect against the catalogue rows routed here — a subset of the rows `review-code`'s **Priority 6 —
Test Quality** names — and do not fork a divergent list. Detect only test
files matching the globs in `${CLAUDE_PLUGIN_ROOT}/rules/testing.md` frontmatter (the same set
`testing.md` scopes itself to). Non-test files (the SUT) are never scanned or rewritten. Act on
**Python + TypeScript/React only**; files matched by other globs are recognized as tests but skipped with
a one-line note.

## Parse the argument

| Pattern | Mode | Action |
|---------|------|--------|
| `--review <path>` | Review | Detect behavioural smells under `<path>`, write `.test-redesign-backlog.md`, **stop** (no changes). |
| A file path or `test-id` | Scoped loop | Run the approval loop scoped to that file/test only. |
| No argument | Backlog loop | Work the existing `.test-redesign-backlog.md` one approved item at a time. |

---

## Prerequisites (all modes except review-only detection)

Run these three checks once, **in this order**, before the loop starts.

### 1. Locate the test runner
Follow `${CLAUDE_PLUGIN_ROOT}/skills/shared/test-runner-detection.md` (reuse). Prefer the `CLAUDE.md` test command; else
auto-detect. Run it once to confirm it works before touching anything. If none is found, warn and
use AskUserQuestion before proceeding — redesigning tests without a runnable suite removes even the
secondary sanity check.

### 2. Confirm a clean working tree
The loop applies changes to the working tree and relies on `git checkout` to revert. A rewrite can
write beyond the target test file — `conftest.py`, a new helper module — so check that `git status
--porcelain` shows no change outside the backlog files; otherwise ask the developer to commit or stash
first, since a revert could discard their work. Do this **before** the baseline run.

### 3. Establish a green baseline (safety rule — never redesign on red)
Run the suite **before** proposing any rewrite. **If the baseline is red, STOP and report — do not
propose or apply anything.** (Fixture `01-baseline-red-stop`.)

---

## Review mode — `--review <path>` (seeds the backlog)

1. Enumerate test files under `<path>` matching the `testing.md` globs. Ignore non-test files.
2. Detect only the three behavioural smell families (above), reusing `review-code` Priority 6. One
   backlog item per smell instance.
3. Write `.test-redesign-backlog.md` in the project root, in `review-code`'s backlog **format** (by
   convention — do not modify `review-code`). Keep it **separate** from `.test-refactoring-backlog.md`
   so the two tools never collide. Use this shape:

   ```markdown
   # Test Redesign Backlog

   Generated: {date}
   Scope: --review {path}

   ## Summary

   - **Total items**: {count}
   - **Implementation-coupling**: {count}
   - **Excessive-mocking**: {count}
   - **Flakiness/determinism**: {count}

   ## Backlog

   ### [RD001] {smell family} — {one-line description}

   - **File**: `{file-path}`
   - **Lines**: {start}-{end}
   - **Priority**: {1-3}
   - **Smell**: {smell family} — {catalogue smell from test-patterns.md}

From pmvanev/phil-claude-plugin, MIT.

More from pmvanev/phil-claude-plugin