mirror of
https://github.com/obra/superpowers.git
synced 2026-07-08 02:59:04 +08:00
fix(skills): close the change-detector hole in writing-good-tests
Fresh-eyes review found falsifiable-but-worthless tests passed every rule: a constant assertion can fail, uses a literal, mocks nothing — and protects nothing, firing on intentional decisions while sleeping through bugs. Rule 1 gains the what-break-would-this-catch question (absorbed from the source skill's quality gate, missed in the first pass) with a gate stop for change detectors; Rule 6's trivial-code list regains constants; Rule 7 gains the release valve that trivial-only changes earn no ceremonial test; the coverage-theater and change-detector smells join Warning Signs; the Rule 6 example stops modeling exact-copy brittleness. Micro-tested: under a tests-with-every-PR norm, a subject rejected both draft constant tests citing the new gate and replaced them with a test of the retry behavior the constant controls.
This commit is contained in:
@@ -59,6 +59,14 @@ test('formats timestamp', () => {
|
||||
A mirror assertion re-derives the answer with the answer's own machinery:
|
||||
it passes no matter what that machinery does.
|
||||
|
||||
**Falsifiable is necessary, not sufficient — name the break.** A test must
|
||||
fail for the right reason: name the wrong branch, missing side effect,
|
||||
wrong argument, boundary case, or contract violation it would catch. If
|
||||
every change that could fail it is an intentional decision — a constant's
|
||||
value, the exact wording of a message, private structure — you have
|
||||
written a change detector, not a test: it fires on redesign and sleeps
|
||||
through bugs. Test the behavior that depends on the decision instead.
|
||||
|
||||
**The string-presence trap.** For a script, skill, prompt, or config, a
|
||||
test that asserts the source contains an exact line counterfeits this
|
||||
rule: it can fail (delete the line), so it passes the letter of
|
||||
@@ -81,6 +89,12 @@ BEFORE writing the test body:
|
||||
IF the only answer is "the source text changed":
|
||||
STOP - Run the artifact and assert its effects instead
|
||||
|
||||
Ask: "What BREAK would this catch?"
|
||||
|
||||
IF every failing change is an intentional decision, never a bug:
|
||||
STOP - That is a change detector; test the behavior that
|
||||
depends on the decision instead
|
||||
|
||||
Ask: "Is the expected value derived independently of the code under test?"
|
||||
|
||||
IF it reuses the code's own logic or helpers:
|
||||
@@ -291,7 +305,7 @@ maintainers' tests to write.
|
||||
test('GET /sessions/:id returns 404 for unknown id', async () => {
|
||||
const res = await request(app).get('/sessions/nope');
|
||||
expect(res.status).toBe(404);
|
||||
expect(res.body.error).toBe('session not found');
|
||||
expect(res.body.error).toMatch(/not found/); // contract, not exact copy
|
||||
});
|
||||
```
|
||||
|
||||
@@ -311,9 +325,9 @@ point and name the assumption in the test name or a comment.
|
||||
|
||||
The same boundary applies inside your own code: test behavior, not that
|
||||
the implementation is written the way it is currently written. Plain
|
||||
constructor assignment, getters, trivial forwarding, and data-only
|
||||
structs earn tests only when they validate, normalize, default, derive,
|
||||
enforce, or cause side effects — otherwise assert the first
|
||||
constructor assignment, getters, constants, trivial forwarding, and
|
||||
data-only structs earn tests only when they validate, normalize, default,
|
||||
derive, enforce, or cause side effects — otherwise assert the first
|
||||
consumer-visible result that depends on them.
|
||||
|
||||
## Rule 7: Tests Ship With the Implementation
|
||||
@@ -322,6 +336,10 @@ Testing is part of implementation. The TDD cycle — failing test, minimal
|
||||
implementation, refactor — is what "complete" means; "implementation
|
||||
complete, ready for testing" describes an unfinished task.
|
||||
|
||||
Ship the tests the behavior needs — and only those. A change that touches
|
||||
only trivial code (Rule 6) earns no ceremonial test: a test written to
|
||||
satisfy process protects nothing and costs maintenance forever.
|
||||
|
||||
## Rule 8: Prefer Real Components Over Complex Mocks
|
||||
|
||||
Integration tests with real components are often simpler than elaborate
|
||||
@@ -377,3 +395,5 @@ test as tautological.
|
||||
- Expected values are hidden behind loops, builders, or helpers
|
||||
- The test greps source text instead of observing behavior
|
||||
- The test asserts that a removed function, file, or symbol stays removed
|
||||
- The test exists for coverage, checking no side effect, boundary, or outcome
|
||||
- The test fails on every intentional change and never on accidental breakage
|
||||
|
||||
Reference in New Issue
Block a user