refactor-tests

Files (27)

• SKILL.md
• acceptance.feature
• cart.py · self-test/01-baseline-red-stop/
• expected.md · self-test/01-baseline-red-stop/
• manifest.json · self-test/01-baseline-red-stop/
• test_cart.py · self-test/01-baseline-red-stop/
• cart.py · self-test/02-postapply-red-autorevert/
• expected.md · self-test/02-postapply-red-autorevert/
• manifest.json · self-test/02-postapply-red-autorevert/
• move.patch · self-test/02-postapply-red-autorevert/
• test_cart.py · self-test/02-postapply-red-autorevert/
• cart.py · self-test/03-approve-commit-on-green/
• 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/
• test_cart.py · self-test/03-approve-commit-on-green/
• cart.py · self-test/04-reject-reverts-clean/
• expected.md · self-test/04-reject-reverts-clean/
• manifest.json · self-test/04-reject-reverts-clean/
• move.patch · self-test/04-reject-reverts-clean/
• test_cart.py · self-test/04-reject-reverts-clean/
• cart.py · self-test/05-review-seeds-backlog/
• 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/
• test_cart_smells.py · self-test/05-review-seeds-backlog/
• README.md · self-test/

SKILL.md (first lines)

# Refactor Tests

Clean test code to the structure standards in `${CLAUDE_PLUGIN_ROOT}/rules/testing.md` and
`${CLAUDE_PLUGIN_ROOT}/rules/test-patterns.md` (and the matching language rules) **without silently
weakening what the tests verify**. Work through a prioritized backlog one item at a time; **every change
is committed only after the developer approves the diff**, which they review applied in their editor.

**This is structure-only, assertion-preserving work.** The test *is* the safety oracle, so a
green suite alone cannot prove an assertion was not weakened — **the human is the oracle**
(a green suite is only a secondary sanity check). Never change what a test asserts, never split
or merge assertions, never delete a test, never fix determinism/flakiness. Those are behavior
changes and are out of scope.

## The only moves to propose (D5)

Only these assertion-preserving structure moves. If a smell is not one of these, do **not**
report or apply it.

| Smell | Named move | Why it is assertion-preserving |
|-------|-----------|--------------------------------|
| Duplicated setup (same arrange across ≥2 tests) | **Extract Fixture/Helper** | moves setup into a fixture or a Creation Method; assertions unchanged |
| Missing AAA (arrange/act/assert interleaved) | **Reorder into Arrange-Act-Assert** | reorders statements; same asserts, same values |
| Vague test name | **Rename** | renames the test; body and asserts unchanged |
| Long test with a repeated/extractable block | **Extract Test Helper** | extracts a block to a helper; each assert preserved verbatim |

Each move is the fix in a Smell Catalogue row of `test-patterns.md` routed to `/phil:refactor-tests`:
Test Code Duplication → Extract Fixture/Helper; Obscure Test → Extract Test Helper, which yields a
Creation Method for a build block or a Custom Assertion for an assert block; Vague Name → Rename;
Interleaved Phases → Reorder into Arrange-Act-Assert. Name the smell
from that row in the backlog. The catalogue's other structural smells — Mystery Guest, General
Fixture, Irrelevant Information, Assertion Roulette, Eager Test — also keep the assertion, but no move
here fixes them yet; `review-code` reports them and a human applies the fix.

**Say when a move hides a value the assertion depends on.** DAMP, not DRY: a fixture holding the
inputs the asserted value follows from moves the cause out of the test body, and trades Test Code
Duplication for an Obscure Test. Prefer a helper that takes those values as arguments. When the move
still extracts them, the proposal's rationale says so in one line, and the human weighs the trade at
the gate.

**Never in scope:** tightening loose assertions, assert-splitting, determinism fixes (static
timestamps, seeded RNG, injected clocks), deleting dead/duplicate tests, mocking changes. These
change what the suite verifies.

## Detection heuristics (per language)

Detect only test files matching the globs in `${CLAUDE_PLUGIN_ROOT}/rules/testing.md` frontmatter —
the **same set that file scopes itself to**. Do not hand-maintain a divergent list; if `testing.md`
changes, this follows it. The current set: `**/*.test.{ts,tsx,js,jsx}`, `**/*.spec.{ts,tsx,js,jsx}`,
`**/test_*.py`, `**/*_test.{go,py,rs}`, `**/*.test.py`, `**/*Test.java`, `**/*_test.rb`, and
anything under `**/tests/**`, `**/__tests__/**`, `**/test/**`. Non-test files (the SUT) are never
scanned or refactored. **Act on Python + TypeScript/React only** — files matched by the
Java/Ruby/Go globs are recognized as tests but skipped with a one-line note until their per-language
heuristics land.

| Smell | Python signal | TypeScript/React signal |
|-------|---------------|-------------------------|
| Duplicated setup | identical arrange statements (same literals/construction) repeated in ≥2 `def test_*` bodies | same arrange repeated across ≥2 `it()`/`test()` blocks, with no helper building it |
| Missing AAA | asserts interleaved between multiple acts; no arrange→act→assert separation | `expect(...)` calls interleaved with actions; no clear setup/act/assert grouping |
| Vague name | name that states no behavior: `test_1`, `test_it_works`, `test_case`, `test_foo`, `test_method`, `test_func` | `it("works")`, `it("test 1")`, `test("case")`, names with no behavior clause |
| Long test w/ extractable block | test body long (~>15 statements) or a repeated multi-line block usable as a helper | long `it()` body or a repeated build/assert block extractable to a helper |

When unsure whether a flag is one of the four smells, **skip it** — false positives erode trust (the
precision target: at most 20% rejected on first review). Be precise, not exhaustive.

## Parse the argument

| Pattern | Mode | Action |
|---------|------|--------|
| `--review <path>` | Review | Detect smells under `<path>`, write `.test-refactoring-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-refactoring-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. Do not begin proposing
moves until all three pass.

### 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 (`package.json`, `pytest.ini`/`pyproject.toml`, `Makefile`, etc.). Run it once
to confirm it works before touching anything. If none is found, warn and use AskUserQuestion
before proceeding — refactoring 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 move 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 so no in-flight
work is ever at risk.

### 3. Establish a green baseline (safety rule — never refactor on red)

Run the suite **before** proposing any move. **If the baseline is red, STOP and report — do not
propose or apply anything.** A red baseline means the sanity check is meaningless and the repo
is mid-change; the developer must get to green first. (This is 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 four smells (heuristics above). One backlog item per smell instance.
3. Write `.test-refactoring-backlog.md` in the project root, in `review-code`'s backlog format
   (by convention — do not modify `review-code`). Use this shape:

   ```markdown
   # Test Refactoring Backlog

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

   ## Summary

   - **Total items**: {count}

From pmvanev/phil-claude-plugin, MIT.

More from pmvanev/phil-claude-plugin