rogee/superpowers
1
0
Fork 0
mirror of https://github.com/obra/superpowers.git synced 2026-07-07 18:49:05 +08:00
Code Issues Packages Projects Releases Wiki Activity

experiment: ground-up two-principle rewrite of writing-good-tests

Browse Source 
Re-derived from scratch: every rule becomes a corollary of two principles
(every test names the break it catches; every test exercises the real
thing), one consolidated gate per principle, four example pairs kept, the
rest carried by prose. Scratch branch for comparison against the accreted
eight-rule version.
This commit is contained in:
Jesse Vincent
2026-07-05 18:47:55 -04:00
parent 8afa64b49d
commit 0e69a4d32c
1 changed files with 129 additions and 328 deletions
Download Patch File Download Diff File Expand all files Collapse all files

457
skills/test-driven-development/writing-good-tests.md
View File

@@ -5,393 +5,194 @@ adding cleanup/helper methods for tests.
## Overview ## Overview
Good tests verify real behavior. Mocks exist to isolate the code under A test exists to catch a specific break. Two principles govern everything
test — they are never the thing being tested. here:
**Core principle:** Test what the code does, not what the mocks do — and
make every test able to fail.
Strict TDD produces every rule below naturally: a test written first and
watched failing against real code has already proven it can fail, and
only earns a mock when the real dependency proves slow or external. A
test asserting on a mock means TDD was skipped somewhere.
## The Iron Laws
``` ```
1. Every test can fail — name the production change that would fail it 1. Every test names the break it catches
2. Assert on real behavior, never on mock behavior 2. Every test exercises the real thing
3. Production classes carry production methods only
4. Understand a dependency's side effects before mocking it
``` ```
## Rule 1: Write Tests That Can Fail Strict TDD produces both naturally: a test written first and watched
failing against real code has already proven it can fail, and only earns
a mock when the real dependency proves slow or external.
Before writing or changing a test, name the production change that would ## Principle 1: Name the Break
make it fail. If you cannot, redesign the test around an observable
behavior — a test that cannot fail protects nothing.
Derive expected values independently of the code under test: literals, Before writing the test body, answer: **what production change should
hand-checked fixtures, small worked examples, or invariant assertions. make this test fail — and is that change a bug or a decision?** A test
Keep test logic simple enough to review by inspection — table-driven earns its place by catching a wrong branch, missing side effect, wrong
tests with literal `want` values are the preferred shape. argument, boundary case, or broken contract.
**Derive expectations independently.** Use literals and hand-checked
fixtures; table-driven tests with literal `want` values are the preferred
shape. An expectation computed by the code under test — or its helpers —
passes no matter what that code does:
```typescript ```typescript
// ✅ GOOD: literal, hand-derived expectation // ❌ Mirror assertion: the same builder computes both sides — always true
test('builds tag query', () => { const expected = buildSearchQuery({ tag: 'urgent' });
expect(buildSearchQuery({ tag: 'urgent' })).toBe('tag:"urgent"'); expect(buildSearchQuery({ tag: 'urgent' })).toBe(expected);
});
// ✅ Hand-derived literal
expect(buildSearchQuery({ tag: 'urgent' })).toBe('tag:"urgent"');
``` ```
```typescript **No change detectors.** If only intentional decisions can fail a test —
// ❌ The violation: expectation computed by the logic under test a constant's value, exact message wording, private structure — it fires
test('builds tag query', () => { on redesign and sleeps through bugs. Test the behavior that depends on
const expected = buildSearchQuery({ tag: 'urgent' }); // same builder! the decision: not `expect(MAX_RETRIES).toBe(5)` but "a failing call is
expect(buildSearchQuery({ tag: 'urgent' })).toBe(expected); // always true retried 5 times and the 6th attempt never happens."
});
// ❌ Subtler: the expectation reuses the same helper the code calls **Behavior, not text.** Asserting that a script, skill, or config
test('formats timestamp', () => { contains an exact line proves only that the source is the source. Run
expect(render(entry)).toContain(formatTime(entry.ts)); // mirrors implementation scripts against controlled inputs and assert outputs, side effects, or
}); exit codes. Documents that instruct agents are tested by the consuming
``` agent's behavior (superpowers:writing-skills); prose for humans earns no
test at all.
A mirror assertion re-derives the answer with the answer's own machinery: **Your code, not the framework.** Test the contract your code makes at
it passes no matter what that machinery does. its boundaries — the route you register, the query you emit, the payload
you produce. Upstream mechanics are their maintainers' tests to write
**Name the break, not just the change.** A test earns its place by (the classic: asserting your router invokes a registered handler — that
catching a wrong branch, missing side effect, wrong argument, boundary, is the framework's test, not yours). When upstream behavior genuinely
or broken contract. If only intentional decisions can fail it — a surprised you, write one narrow characterization test naming the
constant's value, exact message wording — it is a change detector: it assumption. The same boundary applies inside your code: constructors,
fires on redesign and sleeps through bugs. getters, constants, and trivial forwarding earn tests only when they
validate, normalize, default, derive, enforce, or cause side effects —
**The string-presence trap.** Asserting that a script, skill, or config otherwise assert the first consumer-visible result that depends on them.
contains an exact line counterfeits falsifiability: it proves only that
the source is the source, breaking on every rewording and surviving every
real regression. Run scripts and assert outputs, side effects, or exit
codes; test agent-instructing documents by their consumer's behavior.
Text containment is never the observable.
### Gate Function ### Gate Function
``` ```
BEFORE writing the test body: BEFORE writing the test body:
Ask: "What production change should make this test fail?" Name the production change that would make this test fail.
IF you cannot name one: Cannot name one → redesign around an observable behavior
STOP - Redesign the test around an observable behavior "The source text changed" → run the artifact and assert its effects
Only intentional decisions → change detector; test the behavior
that depends on the decision
IF the only answer is "the source text changed": Confirm the expected value is derived without the code under test.
STOP - Run the artifact and assert its effects instead IF it reuses the code's logic or helpers:
Replace it with a literal or hand-checked fixture
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:
STOP - Replace it with a literal or hand-checked fixture
``` ```
## Rule 2: Assert on Real Behavior ## Principle 2: Exercise the Real Thing
**The mock earns no assertions.** A mock assertion passes when the mock
is present and fails when it is absent — it says nothing about the
component. Assert the real component's behavior; if the mock is what you
are checking, unmock it or delete the assertion.
```typescript ```typescript
// ✅ GOOD: Test the real component // ✅ Real behavior
test('renders sidebar', () => { expect(screen.getByRole('navigation')).toBeInTheDocument();
render(<Page />); // Sidebar unmocked
expect(screen.getByRole('navigation')).toBeInTheDocument(); // ❌ Mock existence
}); expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument();
``` ```
If the sidebar must be mocked for isolation, assert on Page's behavior **your human partner's correction:** "Are we testing the behavior of a
with the sidebar present — the mock itself earns no assertions. mock?"
**Mock at the right level.** Learn every side effect of the real method
before replacing it; mock the slow or external operation and keep what
the test depends on real. When unsure, run the test against the real
implementation first and observe what actually needs to happen.
```typescript ```typescript
// ❌ The violation: asserting that the mock exists // ❌ The mock swallows the config write that duplicate detection reads
test('renders sidebar', () => { vi.mock('ToolCatalog', () => ({
render(<Page />); discoverAndCacheTools: vi.fn().mockResolvedValue(undefined)
expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument(); }));
});
// ✅ Mock only the slow server startup; the config write stays real
vi.mock('MCPServerManager');
``` ```
A mock assertion passes when the mock is present and fails when it is **Make doubles specific.** When arguments, call counts, or ordering are
absent — it says nothing about the component. **your human partner's part of the contract, assert them — a fake that accepts anything verifies
correction:** "Are we testing the behavior of a mock?" nothing. Give each branch (success, error, malformed) its own fixture or
spy, so the wrong branch cannot satisfy the expectation.
**Mirror real data completely.** Mock the complete structure as it exists
in reality — all documented fields — not just the ones your test reads.
Partial mocks fail silently when downstream code reads an omitted field:
the test passes while integration breaks.
**Production classes carry production methods only.** Cleanup that only
tests need lives in test utilities, never as a `destroy()` on the
production class. Ask: is this method called only from tests? Does this
class own this resource's lifecycle? Wrong answers → test utility.
**Prefer real components over complex mocks.** When mock setup outgrows
the test logic, mocks miss methods the real components have, or tests
break when the mock changes, switch to an integration test with real
components. **your human partner's question:** "Do we need to be using a
mock here?"
### Gate Function ### Gate Function
``` ```
BEFORE asserting on any mock element: BEFORE adding a mock or test helper:
Ask: "Am I testing real component behavior or just mock existence?" List the real method's side effects; keep the ones the test
depends on real — mock the slow/external level below them.
IF testing mock existence: Mock responses mirror the complete real structure.
STOP - Delete the assertion or unmock the component
Test real behavior instead A method only tests call lives in test utilities, not production.
About to assert on the mock itself?
Unmock it or delete the assertion.
``` ```
## Rule 3: Keep Test Cleanup in Test Utilities ## Tests Ship With the Implementation
```typescript The TDD cycle — failing test, minimal implementation, refactor — is what
// ✅ GOOD: Test utilities own test cleanup "complete" means. Ship the tests the behavior needs and only those:
// Session has no destroy() - it's stateless in production trivial code and human prose earn none, and a test written to satisfy
process costs maintenance forever.
// In test-utils/
export async function cleanupSession(session: Session) {
const workspace = session.getWorkspaceInfo();
if (workspace) {
await workspaceManager.destroyWorkspace(workspace.id);
}
}
// In tests
afterEach(() => cleanupSession(session));
```
```typescript
// ❌ The violation: destroy() exists only for tests
class Session {
async destroy() { // Looks like production API!
await this._workspaceManager?.destroyWorkspace(this.id);
// ... cleanup
}
}
// In tests
afterEach(() => session.destroy());
```
A test-only method pollutes the production class, is dangerous if
production code ever calls it, and confuses object lifecycle with entity
lifecycle.
### Gate Function
```
BEFORE adding any method to a production class:
Ask: "Is this only used by tests?"
IF yes:
STOP - Put it in test utilities instead
Ask: "Does this class own this resource's lifecycle?"
IF no:
STOP - Wrong class for this method
```
## Rule 4: Mock at the Right Level
Learn what the real method does — every side effect — before replacing
it. Mock the slow or external operation and preserve the behavior your
test depends on.
Make doubles specific to their contract: when arguments, call counts, or
ordering matter, assert them — a fake that accepts anything verifies
nothing. And give each branch its own double: success, error, and
malformed paths each get their own fixture or spy, so the wrong branch
cannot satisfy the expectation.
```typescript
// ✅ GOOD: Mock the slow part, preserve behavior the test needs
test('detects duplicate server', () => {
vi.mock('MCPServerManager'); // Just mock slow server startup
await addServer(config); // Config written
await addServer(config); // Duplicate detected ✓
});
```
```typescript
// ❌ The violation: the mock swallows the side effect the test depends on
test('detects duplicate server', () => {
// Mock prevents the config write that duplicate detection reads!
vi.mock('ToolCatalog', () => ({
discoverAndCacheTools: vi.fn().mockResolvedValue(undefined)
}));
await addServer(config);
await addServer(config); // Should throw - but won't!
});
```
### Gate Function
```
BEFORE mocking any method:
STOP - Understand before replacing
1. Ask: "What side effects does the real method have?"
2. Ask: "Does this test depend on any of those side effects?"
3. Ask: "Do I fully understand what this test needs?"
IF the test depends on side effects:
Mock at the lower level (the actual slow/external operation)
OR use test doubles that preserve the necessary behavior
— keep the high-level method the test depends on real
IF unsure what the test depends on:
Run the test with the real implementation FIRST
Observe what actually needs to happen
THEN add minimal mocking at the right level
Warning signs:
- "I'll mock this to be safe"
- "This might be slow, better mock it"
- Mocking before tracing the dependency chain
```
## Rule 5: Mirror Real Data Completely
Mock the COMPLETE data structure as it exists in reality, not just the
fields your immediate test uses.
```typescript
// ✅ GOOD: Mirror real API completeness
const mockResponse = {
status: 'success',
data: { userId: '123', name: 'Alice' },
metadata: { requestId: 'req-789', timestamp: 1234567890 }
// All fields real API returns
};
```
```typescript
// ❌ The violation: only the fields you thought you needed
const mockResponse = {
status: 'success',
data: { userId: '123', name: 'Alice' }
// Missing: metadata that downstream code uses
};
// Later: breaks when code accesses response.metadata.requestId
```
Partial mocks hide structural assumptions and fail silently when
downstream code reads an omitted field: the test passes while integration
breaks.
### Gate Function
```
BEFORE creating mock responses:
Check: "What fields does the real API response contain?"
Actions:
1. Examine the actual API response from docs/examples
2. Include ALL fields the system might consume downstream
3. Verify the mock matches the real response schema completely
If uncertain: include all documented fields
```
## Rule 6: Test Your Code, Not the Framework
Test the contract your code makes at its boundaries — the route you
register, the query you emit, the payload shape you produce, the value
handoff between layers. Dependencies' documented mechanics are their
maintainers' tests to write.
```typescript
// ✅ GOOD: your contract at the boundary
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).toMatch(/not found/); // contract, not exact copy
});
```
```typescript
// ❌ The violation: re-proving the router works as documented
test('router calls handler for matching route', () => {
const handler = vi.fn();
router.get('/x', handler);
router.handle(makeRequest('/x'));
expect(handler).toHaveBeenCalled();
});
```
When upstream behavior genuinely surprised you (a quoting rule, an event
ordering), write one narrow characterization test around your integration
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, 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
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. Trivial-code changes
(Rule 6) and prose for humans (READMEs, comments, docs) earn no test:
there is no behavior to protect, and a test written to satisfy process
costs maintenance forever. Skills and prompts follow their own discipline
— pressure-test the consuming agent when an edit changes behavior
(superpowers:writing-skills) — never their text.
## Rule 8: Prefer Real Components Over Complex Mocks
Integration tests with real components are often simpler than elaborate
mocks. Reach for one when you see:
- Mock setup longer than the test logic
- Mocking everything to make the test pass
- Mocks missing methods the real components have
- Tests breaking when the mock changes
**your human partner's question:** "Do we need to be using a mock here?"
## The Mutation Check ## The Mutation Check
Before finishing, mentally mutate the production code. At least one test Before finishing, mentally mutate the production code; at least one test
should fail for each realistic mutation: should fail for each realistic mutation:
- Wrong constant or argument - Wrong constant or argument
- Wrong branch handler - Wrong branch handler
- Missing state change or side effect (row not written, event not emitted) - Missing state change or side effect
- Empty or default return - Empty or default return
- Missing validation for zero, empty, nil, unauthorized, or malformed input - Missing validation for zero, empty, nil, unauthorized, or malformed input
A mutation no test can catch marks the behavior as unprotected — or the A mutation nothing catches marks the behavior as unprotected — or the
test as tautological. test as tautological.
## Quick Reference ## Quick Reference
| When you... | Do | | When you... | Do |
|-------------|-----| |-------------|-----|
| Write any test | Name the production change that would make it fail | | Write any test | Name the break it catches — a bug, not a decision |
| Build an expected value | Derive it independently — literal or hand-checked fixture | | Build an expected value | Derive it by hand; never with the code under test |
| Want to assert on a mocked element | Test the real component, or unmock it | | Test a script or document | Run it / pressure-test its consumer; never grep its text |
| Need cleanup that only tests use | Put it in test utilities |
| Are about to mock a method | Learn its side effects first; mock the slow/external level |
| Build a mock response | Mirror the real structure completely |
| Reach for a dependency test | Test your boundary contract, not their documented mechanics | | Reach for a dependency test | Test your boundary contract, not their documented mechanics |
| Finish an implementation | Tests already exist (TDD) — or it is unfinished | | Want to assert on a mocked element | Test the real component, or unmock it |
| Finish a test file | Run the mutation check | | Are about to mock a method | Learn its side effects; mock the slow/external level |
| Build a mock response | Mirror the real structure completely |
| Need cleanup only tests use | Put it in test utilities |
| Watch mock setup balloon | Switch to an integration test with real components | | Watch mock setup balloon | Switch to an integration test with real components |
| Finish a test file | Run the mutation check |
## Warning Signs ## Warning Signs
- An assertion checks for a `*-mock` test ID
- A method is called only from test files
- Mock setup is more than half the test
- The test fails when you remove the mock
- You can't explain why the mock is needed
- Mocking "just to be safe"
- Setup and assertion share the same object, guaranteeing equality - Setup and assertion share the same object, guaranteeing equality
- The test can fail only through a panic, crash, or missing selector - The test can fail only through a panic, crash, or missing selector
- The test would still matter if only the framework remained - The test fails on every intentional change, never on accidental breakage
- Expected values are hidden behind loops, builders, or helpers - Expected values are hidden behind loops, builders, or helpers
- The test greps source text instead of observing behavior - The test greps source text, or asserts a removed symbol stays removed
- The test asserts that a removed function, file, or symbol stays removed - The test would still matter if only the framework remained
- The test exists for coverage, checking no side effect, boundary, or outcome - The test exists for coverage, checking no side effect or outcome
- The test fails on every intentional change and never on accidental breakage - An assertion checks a `*-mock` test ID, or fails if you remove the mock
- A method is called only from test files
- Mock setup is more than half the test, or you can't explain why the mock is needed
- Mocking "just to be safe"