From 0cfc0a16b4942410780b5edddb24b6f76c881179 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Sun, 5 Jul 2026 12:49:52 -0700 Subject: [PATCH 1/6] refactor(skills): reframe testing-anti-patterns as writing-good-tests The disclosure doc becomes a catalog of what to do: six positively named rules (assert on real behavior, cleanup in test utilities, mock at the right level, mirror real data, tests ship with implementation, prefer real components), each leading with the GOOD example and keeping the violation as contrast. Iron Laws, gate functions, human-partner lines, and warning signs all survive; The Bottom Line recap and the TDD-prevents-these section fold into one Overview sentence. SKILL.md's pointer moves into the Good Tests section it belongs with. Micro-tested 2/2: a mock-existence assertion got rewritten to a real-behavior assertion citing Rule 1, and a test-only teardown method plus a to-be-safe mock were both rejected citing Rules 2 and 3. --- skills/test-driven-development/SKILL.md | 12 +- .../testing-anti-patterns.md | 299 ------------------ .../writing-good-tests.md | 248 +++++++++++++++ 3 files changed, 253 insertions(+), 306 deletions(-) delete mode 100644 skills/test-driven-development/testing-anti-patterns.md create mode 100644 skills/test-driven-development/writing-good-tests.md diff --git a/skills/test-driven-development/SKILL.md b/skills/test-driven-development/SKILL.md index 60d2609ca5..158cb0b513 100644 --- a/skills/test-driven-development/SKILL.md +++ b/skills/test-driven-development/SKILL.md @@ -203,6 +203,11 @@ Next failing test for next feature. | **Clear** | Name describes behavior | `test('test1')` | | **Shows intent** | Demonstrates desired API | Obscures what code should do | +When adding mocks or test utilities, read [writing-good-tests.md](writing-good-tests.md) for the rules that keep tests honest: +- Assert on real behavior, never on mock behavior +- Keep test-only code in test utilities, out of production classes +- Understand a dependency's side effects before mocking it + ## Why Order Matters **"I'll write tests after to verify it works"** @@ -354,13 +359,6 @@ Bug found? Write failing test reproducing it. Follow TDD cycle. Test proves fix Never fix bugs without a test. -## Testing Anti-Patterns - -When adding mocks or test utilities, read [testing-anti-patterns.md](testing-anti-patterns.md) to avoid common pitfalls: -- Testing mock behavior instead of real behavior -- Adding test-only methods to production classes -- Mocking without understanding dependencies - ## Final Rule ``` diff --git a/skills/test-driven-development/testing-anti-patterns.md b/skills/test-driven-development/testing-anti-patterns.md deleted file mode 100644 index e77ab6b6d6..0000000000 --- a/skills/test-driven-development/testing-anti-patterns.md +++ /dev/null @@ -1,299 +0,0 @@ -# Testing Anti-Patterns - -**Load this reference when:** writing or changing tests, adding mocks, or tempted to add test-only methods to production code. - -## Overview - -Tests must verify real behavior, not mock behavior. Mocks are a means to isolate, not the thing being tested. - -**Core principle:** Test what the code does, not what the mocks do. - -**Following strict TDD prevents these anti-patterns.** - -## The Iron Laws - -``` -1. NEVER test mock behavior -2. NEVER add test-only methods to production classes -3. NEVER mock without understanding dependencies -``` - -## Anti-Pattern 1: Testing Mock Behavior - -**The violation:** -```typescript -// ❌ BAD: Testing that the mock exists -test('renders sidebar', () => { - render(); - expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument(); -}); -``` - -**Why this is wrong:** -- You're verifying the mock works, not that the component works -- Test passes when mock is present, fails when it's not -- Tells you nothing about real behavior - -**your human partner's correction:** "Are we testing the behavior of a mock?" - -**The fix:** -```typescript -// ✅ GOOD: Test real component or don't mock it -test('renders sidebar', () => { - render(); // Don't mock sidebar - expect(screen.getByRole('navigation')).toBeInTheDocument(); -}); - -// OR if sidebar must be mocked for isolation: -// Don't assert on the mock - test Page's behavior with sidebar present -``` - -### Gate Function - -``` -BEFORE asserting on any mock element: - Ask: "Am I testing real component behavior or just mock existence?" - - IF testing mock existence: - STOP - Delete the assertion or unmock the component - - Test real behavior instead -``` - -## Anti-Pattern 2: Test-Only Methods in Production - -**The violation:** -```typescript -// ❌ BAD: destroy() only used in tests -class Session { - async destroy() { // Looks like production API! - await this._workspaceManager?.destroyWorkspace(this.id); - // ... cleanup - } -} - -// In tests -afterEach(() => session.destroy()); -``` - -**Why this is wrong:** -- Production class polluted with test-only code -- Dangerous if accidentally called in production -- Violates YAGNI and separation of concerns -- Confuses object lifecycle with entity lifecycle - -**The fix:** -```typescript -// ✅ GOOD: Test utilities handle test cleanup -// Session has no destroy() - it's stateless in production - -// 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)); -``` - -### Gate Function - -``` -BEFORE adding any method to production class: - Ask: "Is this only used by tests?" - - IF yes: - STOP - Don't add it - Put it in test utilities instead - - Ask: "Does this class own this resource's lifecycle?" - - IF no: - STOP - Wrong class for this method -``` - -## Anti-Pattern 3: Mocking Without Understanding - -**The violation:** -```typescript -// ❌ BAD: Mock breaks test logic -test('detects duplicate server', () => { - // Mock prevents config write that test depends on! - vi.mock('ToolCatalog', () => ({ - discoverAndCacheTools: vi.fn().mockResolvedValue(undefined) - })); - - await addServer(config); - await addServer(config); // Should throw - but won't! -}); -``` - -**Why this is wrong:** -- Mocked method had side effect test depended on (writing config) -- Over-mocking to "be safe" breaks actual behavior -- Test passes for wrong reason or fails mysteriously - -**The fix:** -```typescript -// ✅ GOOD: Mock at correct level -test('detects duplicate server', () => { - // Mock the slow part, preserve behavior test needs - vi.mock('MCPServerManager'); // Just mock slow server startup - - await addServer(config); // Config written - await addServer(config); // Duplicate detected ✓ -}); -``` - -### Gate Function - -``` -BEFORE mocking any method: - STOP - Don't mock yet - - 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 depends on side effects: - Mock at lower level (the actual slow/external operation) - OR use test doubles that preserve necessary behavior - NOT the high-level method the test depends on - - IF unsure what test depends on: - Run test with real implementation FIRST - Observe what actually needs to happen - THEN add minimal mocking at the right level - - Red flags: - - "I'll mock this to be safe" - - "This might be slow, better mock it" - - Mocking without understanding the dependency chain -``` - -## Anti-Pattern 4: Incomplete Mocks - -**The violation:** -```typescript -// ❌ BAD: Partial mock - only fields you think you need -const mockResponse = { - status: 'success', - data: { userId: '123', name: 'Alice' } - // Missing: metadata that downstream code uses -}; - -// Later: breaks when code accesses response.metadata.requestId -``` - -**Why this is wrong:** -- **Partial mocks hide structural assumptions** - You only mocked fields you know about -- **Downstream code may depend on fields you didn't include** - Silent failures -- **Tests pass but integration fails** - Mock incomplete, real API complete -- **False confidence** - Test proves nothing about real behavior - -**The Iron Rule:** Mock the COMPLETE data structure as it exists in reality, not just fields your immediate test uses. - -**The fix:** -```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 -}; -``` - -### Gate Function - -``` -BEFORE creating mock responses: - Check: "What fields does the real API response contain?" - - Actions: - 1. Examine actual API response from docs/examples - 2. Include ALL fields system might consume downstream - 3. Verify mock matches real response schema completely - - Critical: - If you're creating a mock, you must understand the ENTIRE structure - Partial mocks fail silently when code depends on omitted fields - - If uncertain: Include all documented fields -``` - -## Anti-Pattern 5: Integration Tests as Afterthought - -**The violation:** -``` -✅ Implementation complete -❌ No tests written -"Ready for testing" -``` - -**Why this is wrong:** -- Testing is part of implementation, not optional follow-up -- TDD would have caught this -- Can't claim complete without tests - -**The fix:** -``` -TDD cycle: -1. Write failing test -2. Implement to pass -3. Refactor -4. THEN claim complete -``` - -## When Mocks Become Too Complex - -**Warning signs:** -- Mock setup longer than test logic -- Mocking everything to make test pass -- Mocks missing methods real components have -- Test breaks when mock changes - -**your human partner's question:** "Do we need to be using a mock here?" - -**Consider:** Integration tests with real components often simpler than complex mocks - -## TDD Prevents These Anti-Patterns - -**Why TDD helps:** -1. **Write test first** → Forces you to think about what you're actually testing -2. **Watch it fail** → Confirms test tests real behavior, not mocks -3. **Minimal implementation** → No test-only methods creep in -4. **Real dependencies** → You see what the test actually needs before mocking - -**If you're testing mock behavior, you violated TDD** - you added mocks without watching test fail against real code first. - -## Quick Reference - -| Anti-Pattern | Fix | -|--------------|-----| -| Assert on mock elements | Test real component or unmock it | -| Test-only methods in production | Move to test utilities | -| Mock without understanding | Understand dependencies first, mock minimally | -| Incomplete mocks | Mirror real API completely | -| Tests as afterthought | TDD - tests first | -| Over-complex mocks | Consider integration tests | - -## Red Flags - -- Assertion checks for `*-mock` test IDs -- Methods only called in test files -- Mock setup is >50% of test -- Test fails when you remove mock -- Can't explain why mock is needed -- Mocking "just to be safe" - -## The Bottom Line - -**Mocks are tools to isolate, not things to test.** - -If TDD reveals you're testing mock behavior, you've gone wrong. - -Fix: Test real behavior or question why you're mocking at all. diff --git a/skills/test-driven-development/writing-good-tests.md b/skills/test-driven-development/writing-good-tests.md new file mode 100644 index 0000000000..ad8c6023f3 --- /dev/null +++ b/skills/test-driven-development/writing-good-tests.md @@ -0,0 +1,248 @@ +# Writing Good Tests + +**Load this reference when:** writing or changing tests, adding mocks, or +adding cleanup/helper methods for tests. + +## Overview + +Good tests verify real behavior. Mocks exist to isolate the code under +test — they are never the thing being tested. + +**Core principle:** Test what the code does, not what the mocks do. + +Strict TDD produces every rule below naturally: a test written first and +watched failing against real code 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. Assert on real behavior, never on mock behavior +2. Production classes carry production methods only +3. Understand a dependency's side effects before mocking it +``` + +## Rule 1: Assert on Real Behavior + +```typescript +// ✅ GOOD: Test the real component +test('renders sidebar', () => { + render(); // Sidebar unmocked + expect(screen.getByRole('navigation')).toBeInTheDocument(); +}); +``` + +If the sidebar must be mocked for isolation, assert on Page's behavior +with the sidebar present — the mock itself earns no assertions. + +```typescript +// ❌ The violation: asserting that the mock exists +test('renders sidebar', () => { + render(); + expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument(); +}); +``` + +A mock assertion passes when the mock is present and fails when it is +absent — it says nothing about the component. **your human partner's +correction:** "Are we testing the behavior of a mock?" + +### Gate Function + +``` +BEFORE asserting on any mock element: + Ask: "Am I testing real component behavior or just mock existence?" + + IF testing mock existence: + STOP - Delete the assertion or unmock the component + + Test real behavior instead +``` + +## Rule 2: Keep Test Cleanup in Test Utilities + +```typescript +// ✅ GOOD: Test utilities own test cleanup +// Session has no destroy() - it's stateless in production + +// 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 3: 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. + +```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 4: 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 5: 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. + +## Rule 6: 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?" + +## Quick Reference + +| When you... | Do | +|-------------|-----| +| Want to assert on a mocked element | Test the real component, or unmock it | +| 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 | +| Finish an implementation | Tests already exist (TDD) — or it is unfinished | +| Watch mock setup balloon | Switch to an integration test with real components | + +## 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" From 6f3eca4f2eca4cca90de8d67a826a85ef8830016 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Sun, 5 Jul 2026 12:51:57 -0700 Subject: [PATCH 2/6] fix(skills): broaden writing-good-tests trigger to any test writing The pointer fired only on adding mocks or test utilities; the doc's own load-when line already says writing or changing tests. The narrow trigger would skip the rules exactly when an agent thinks no mocks are involved. --- skills/test-driven-development/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/test-driven-development/SKILL.md b/skills/test-driven-development/SKILL.md index 158cb0b513..d6f4e7c798 100644 --- a/skills/test-driven-development/SKILL.md +++ b/skills/test-driven-development/SKILL.md @@ -203,7 +203,7 @@ Next failing test for next feature. | **Clear** | Name describes behavior | `test('test1')` | | **Shows intent** | Demonstrates desired API | Obscures what code should do | -When adding mocks or test utilities, read [writing-good-tests.md](writing-good-tests.md) for the rules that keep tests honest: +When writing or changing any test, read [writing-good-tests.md](writing-good-tests.md) for the rules that keep tests honest: - Assert on real behavior, never on mock behavior - Keep test-only code in test utilities, out of production classes - Understand a dependency's side effects before mocking it From 78fb4643da43152410e718c121d3521082cffa60 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Sun, 5 Jul 2026 12:59:56 -0700 Subject: [PATCH 3/6] feat(skills): absorb falsifiability discipline into writing-good-tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalized from agentsview's testing-without-tautologies skill: a new Iron Law and lead rule (name the production change that would fail the test, derive expectations independently of the code under test), a test-your-code-not-the-framework rule with the characterization-test exception and the trivial-code guidance, branch-specific doubles folded into Mock at the Right Level, a closing Mutation Check, and six new warning-sign smells. Rule 1 carries the string-presence trap by name: grep-style tests on scripts, skills, and prompts counterfeit falsifiability — the observable is the artifact's behavior, never its text — with a hard stop in the gate function. Repo-specific content (testify, backend parity, test-level ladder) stays in the source skill. Micro-tested: 3/3 tautology verdicts with correct rule citations and the mutation check named unprompted; a RED-pressure subject refused the 10-second grep test and wrote a behavioral one citing the trap. --- skills/test-driven-development/SKILL.md | 1 + .../writing-good-tests.md | 157 ++++++++++++++++-- 2 files changed, 145 insertions(+), 13 deletions(-) diff --git a/skills/test-driven-development/SKILL.md b/skills/test-driven-development/SKILL.md index d6f4e7c798..3eccc65859 100644 --- a/skills/test-driven-development/SKILL.md +++ b/skills/test-driven-development/SKILL.md @@ -204,6 +204,7 @@ Next failing test for next feature. | **Shows intent** | Demonstrates desired API | Obscures what code should do | When writing or changing any test, read [writing-good-tests.md](writing-good-tests.md) for the rules that keep tests honest: +- Name the production change that would make the test fail — before writing it - Assert on real behavior, never on mock behavior - Keep test-only code in test utilities, out of production classes - Understand a dependency's side effects before mocking it diff --git a/skills/test-driven-development/writing-good-tests.md b/skills/test-driven-development/writing-good-tests.md index ad8c6023f3..81e9728a41 100644 --- a/skills/test-driven-development/writing-good-tests.md +++ b/skills/test-driven-development/writing-good-tests.md @@ -8,22 +8,86 @@ adding cleanup/helper methods for tests. Good tests verify real behavior. Mocks exist to isolate the code under test — they are never the thing being tested. -**Core principle:** Test what the code does, not what the mocks do. +**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 only earns a mock when the real -dependency proves slow or external. A test asserting on a mock means TDD -was skipped somewhere. +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. Assert on real behavior, never on mock behavior -2. Production classes carry production methods only -3. Understand a dependency's side effects before mocking it +1. Every test can fail — name the production change that would fail it +2. Assert on real behavior, never on mock behavior +3. Production classes carry production methods only +4. Understand a dependency's side effects before mocking it ``` -## Rule 1: Assert on Real Behavior +## Rule 1: Write Tests That Can Fail + +Before writing or changing a test, name the production change that would +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, +hand-checked fixtures, small worked examples, or invariant assertions. +Keep test logic simple enough to review by inspection — table-driven +tests with literal `want` values are the preferred shape. + +```typescript +// ✅ GOOD: literal, hand-derived expectation +test('builds tag query', () => { + expect(buildSearchQuery({ tag: 'urgent' })).toBe('tag:"urgent"'); +}); +``` + +```typescript +// ❌ The violation: expectation computed by the logic under test +test('builds tag query', () => { + const expected = buildSearchQuery({ tag: 'urgent' }); // same builder! + expect(buildSearchQuery({ tag: 'urgent' })).toBe(expected); // always true +}); + +// ❌ Subtler: the expectation reuses the same helper the code calls +test('formats timestamp', () => { + expect(render(entry)).toContain(formatTime(entry.ts)); // mirrors implementation +}); +``` + +A mirror assertion re-derives the answer with the answer's own machinery: +it passes no matter what that machinery does. + +**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 +falsifiability while asserting only that the source is the source. It +breaks on every legitimate rewording and survives every real regression. +The observable for a script is what it does — run it against controlled +inputs and assert outputs, side effects, or exit codes. The observable +for a document that instructs an agent is the consuming agent's behavior +— pressure-test it. Text containment is never the observable. + +### Gate Function + +``` +BEFORE writing the test body: + Ask: "What production change should make this test fail?" + + IF you cannot name one: + STOP - Redesign the test around an observable behavior + + IF the only answer is "the source text changed": + STOP - Run the artifact and assert its effects 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 ```typescript // ✅ GOOD: Test the real component @@ -60,7 +124,7 @@ BEFORE asserting on any mock element: Test real behavior instead ``` -## Rule 2: Keep Test Cleanup in Test Utilities +## Rule 3: Keep Test Cleanup in Test Utilities ```typescript // ✅ GOOD: Test utilities own test cleanup @@ -110,12 +174,18 @@ BEFORE adding any method to a production class: STOP - Wrong class for this method ``` -## Rule 3: Mock at the Right Level +## 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', () => { @@ -165,7 +235,7 @@ BEFORE mocking any method: - Mocking before tracing the dependency chain ``` -## Rule 4: Mirror Real Data Completely +## Rule 5: Mirror Real Data Completely Mock the COMPLETE data structure as it exists in reality, not just the fields your immediate test uses. @@ -209,13 +279,50 @@ BEFORE creating mock responses: If uncertain: include all documented fields ``` -## Rule 5: Tests Ship With the Implementation +## 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).toBe('session not found'); +}); +``` + +```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, 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. -## Rule 6: Prefer Real Components Over Complex Mocks +## 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: @@ -227,15 +334,33 @@ mocks. Reach for one when you see: **your human partner's question:** "Do we need to be using a mock here?" +## The Mutation Check + +Before finishing, mentally mutate the production code. At least one test +should fail for each realistic mutation: + +- Wrong constant or argument +- Wrong branch handler +- Missing state change or side effect (row not written, event not emitted) +- Empty or default return +- Missing validation for zero, empty, nil, unauthorized, or malformed input + +A mutation no test can catch marks the behavior as unprotected — or the +test as tautological. + ## Quick Reference | When you... | Do | |-------------|-----| +| Write any test | Name the production change that would make it fail | +| Build an expected value | Derive it independently — literal or hand-checked fixture | | Want to assert on a mocked element | Test the real component, or unmock it | | 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 | | Finish an implementation | Tests already exist (TDD) — or it is unfinished | +| Finish a test file | Run the mutation check | | Watch mock setup balloon | Switch to an integration test with real components | ## Warning Signs @@ -246,3 +371,9 @@ mocks. Reach for one when you see: - 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 +- The test can fail only through a panic, crash, or missing selector +- The test would still matter if only the framework remained +- 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 From deb9d855cb190afb0e8eb8a60af2e9185145134c Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Sun, 5 Jul 2026 13:03:35 -0700 Subject: [PATCH 4/6] fix(skills): close the change-detector hole in writing-good-tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../writing-good-tests.md | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/skills/test-driven-development/writing-good-tests.md b/skills/test-driven-development/writing-good-tests.md index 81e9728a41..6fba134289 100644 --- a/skills/test-driven-development/writing-good-tests.md +++ b/skills/test-driven-development/writing-good-tests.md @@ -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 From 8afa64b49dfd5e0a0e9e6f62485631d9e7452273 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Sun, 5 Jul 2026 17:47:02 -0400 Subject: [PATCH 5/6] refactor(skills): compress writing-good-tests additions; doc changes earn no tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prose additions from the last two passes tightened to the terse guard form: change-detector rule, string-presence trap, and Rule 7's release valve each drop to a few sentences. Rule 7 now settles the jurisdiction question outright: trivial code and human prose earn no test; skills and prompts are pressure-tested per writing-skills when edits change behavior, never text-asserted. Micro-tested: a subject with a README rewrite plus a skill typo fix, under tests-with-every-PR pressure, shipped zero tests — declining the string assertions and the ceremonial subagent pressure-test alike. --- .../writing-good-tests.md | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/skills/test-driven-development/writing-good-tests.md b/skills/test-driven-development/writing-good-tests.md index 6fba134289..3cae73bc26 100644 --- a/skills/test-driven-development/writing-good-tests.md +++ b/skills/test-driven-development/writing-good-tests.md @@ -59,23 +59,18 @@ 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 -falsifiability while asserting only that the source is the source. It -breaks on every legitimate rewording and survives every real regression. -The observable for a script is what it does — run it against controlled -inputs and assert outputs, side effects, or exit codes. The observable -for a document that instructs an agent is the consuming agent's behavior -— pressure-test it. Text containment is never the observable. +**Name the break, not just the change.** A test earns its place by +catching a wrong branch, missing side effect, wrong argument, boundary, +or broken contract. If only intentional decisions can fail it — a +constant's value, exact message wording — it is a change detector: it +fires on redesign and sleeps through bugs. + +**The string-presence trap.** Asserting that a script, skill, or config +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 @@ -336,9 +331,12 @@ 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. +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 From 0e69a4d32c2db00ebc012310d303907cc5507c6f Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Sun, 5 Jul 2026 18:47:55 -0400 Subject: [PATCH 6/6] experiment: ground-up two-principle rewrite of writing-good-tests 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. --- .../writing-good-tests.md | 445 +++++------------- 1 file changed, 123 insertions(+), 322 deletions(-) diff --git a/skills/test-driven-development/writing-good-tests.md b/skills/test-driven-development/writing-good-tests.md index 3cae73bc26..d3c4482fd3 100644 --- a/skills/test-driven-development/writing-good-tests.md +++ b/skills/test-driven-development/writing-good-tests.md @@ -5,393 +5,194 @@ adding cleanup/helper methods for tests. ## Overview -Good tests verify real behavior. Mocks exist to isolate the code under -test — they are never the thing being tested. - -**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 +A test exists to catch a specific break. Two principles govern everything +here: ``` -1. Every test can fail — name the production change that would fail it -2. Assert on real behavior, never on mock behavior -3. Production classes carry production methods only -4. Understand a dependency's side effects before mocking it +1. Every test names the break it catches +2. Every test exercises the real thing ``` -## 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 -make it fail. If you cannot, redesign the test around an observable -behavior — a test that cannot fail protects nothing. +## Principle 1: Name the Break -Derive expected values independently of the code under test: literals, -hand-checked fixtures, small worked examples, or invariant assertions. -Keep test logic simple enough to review by inspection — table-driven -tests with literal `want` values are the preferred shape. +Before writing the test body, answer: **what production change should +make this test fail — and is that change a bug or a decision?** A test +earns its place by catching a wrong branch, missing side effect, wrong +argument, boundary case, or broken contract. -```typescript -// ✅ GOOD: literal, hand-derived expectation -test('builds tag query', () => { - expect(buildSearchQuery({ tag: 'urgent' })).toBe('tag:"urgent"'); -}); -``` +**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 -// ❌ The violation: expectation computed by the logic under test -test('builds tag query', () => { - const expected = buildSearchQuery({ tag: 'urgent' }); // same builder! - expect(buildSearchQuery({ tag: 'urgent' })).toBe(expected); // always true -}); - -// ❌ Subtler: the expectation reuses the same helper the code calls -test('formats timestamp', () => { - expect(render(entry)).toContain(formatTime(entry.ts)); // mirrors implementation -}); -``` - -A mirror assertion re-derives the answer with the answer's own machinery: -it passes no matter what that machinery does. - -**Name the break, not just the change.** A test earns its place by -catching a wrong branch, missing side effect, wrong argument, boundary, -or broken contract. If only intentional decisions can fail it — a -constant's value, exact message wording — it is a change detector: it -fires on redesign and sleeps through bugs. - -**The string-presence trap.** Asserting that a script, skill, or config -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. +// ❌ Mirror assertion: the same builder computes both sides — always true +const expected = buildSearchQuery({ tag: 'urgent' }); +expect(buildSearchQuery({ tag: 'urgent' })).toBe(expected); + +// ✅ Hand-derived literal +expect(buildSearchQuery({ tag: 'urgent' })).toBe('tag:"urgent"'); +``` + +**No change detectors.** If only intentional decisions can fail a test — +a constant's value, exact message wording, private structure — it fires +on redesign and sleeps through bugs. Test the behavior that depends on +the decision: not `expect(MAX_RETRIES).toBe(5)` but "a failing call is +retried 5 times and the 6th attempt never happens." + +**Behavior, not text.** Asserting that a script, skill, or config +contains an exact line proves only that the source is the source. Run +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. + +**Your code, not the framework.** Test the contract your code makes at +its boundaries — the route you register, the query you emit, the payload +you produce. Upstream mechanics are their maintainers' tests to write +(the classic: asserting your router invokes a registered handler — that +is the framework's test, not yours). When upstream behavior genuinely +surprised you, write one narrow characterization test naming the +assumption. The same boundary applies inside your code: constructors, +getters, constants, and trivial forwarding 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. ### Gate Function ``` BEFORE writing the test body: - Ask: "What production change should make this test fail?" - - IF you cannot name one: - STOP - Redesign the test around an observable behavior + Name the production change that would make this test fail. - IF the only answer is "the source text changed": - STOP - Run the artifact and assert its effects instead + Cannot name one → redesign 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 - 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 + Confirm the expected value is derived without the code under test. + IF it reuses the code's logic or helpers: + Replace it with a literal or hand-checked fixture ``` -## Rule 2: Assert on Real Behavior - -```typescript -// ✅ GOOD: Test the real component -test('renders sidebar', () => { - render(); // Sidebar unmocked - expect(screen.getByRole('navigation')).toBeInTheDocument(); -}); -``` +## Principle 2: Exercise the Real Thing -If the sidebar must be mocked for isolation, assert on Page's behavior -with the sidebar present — the mock itself earns no assertions. +**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 -// ❌ The violation: asserting that the mock exists -test('renders sidebar', () => { - render(); - expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument(); -}); -``` - -A mock assertion passes when the mock is present and fails when it is -absent — it says nothing about the component. **your human partner's -correction:** "Are we testing the behavior of a mock?" - -### Gate Function +// ✅ Real behavior +expect(screen.getByRole('navigation')).toBeInTheDocument(); +// ❌ Mock existence +expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument(); ``` -BEFORE asserting on any mock element: - Ask: "Am I testing real component behavior or just mock existence?" - IF testing mock existence: - STOP - Delete the assertion or unmock the component +**your human partner's correction:** "Are we testing the behavior of a +mock?" - Test real behavior instead -``` - -## Rule 3: Keep Test Cleanup in Test Utilities - -```typescript -// ✅ GOOD: Test utilities own test cleanup -// Session has no destroy() - it's stateless in production - -// 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)); -``` +**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 -// ❌ 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 +// ❌ The mock swallows the config write that duplicate detection reads +vi.mock('ToolCatalog', () => ({ + discoverAndCacheTools: vi.fn().mockResolvedValue(undefined) +})); +// ✅ Mock only the slow server startup; the config write stays real +vi.mock('MCPServerManager'); ``` -BEFORE adding any method to a production class: - Ask: "Is this only used by tests?" - IF yes: - STOP - Put it in test utilities instead +**Make doubles specific.** When arguments, call counts, or ordering are +part of the contract, assert them — a fake that accepts anything verifies +nothing. Give each branch (success, error, malformed) its own fixture or +spy, so the wrong branch cannot satisfy the expectation. - Ask: "Does this class own this resource's lifecycle?" +**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. - IF no: - STOP - Wrong class for this method -``` +**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. -## 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! -}); -``` +**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 ``` -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 -}; -``` +BEFORE adding a mock or test helper: + List the real method's side effects; keep the ones the test + depends on real — mock the slow/external level below them. -```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 -``` + Mock responses mirror the complete real structure. -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 -}); -``` + A method only tests call lives in test utilities, not production. -```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(); -}); + About to assert on the mock itself? + Unmock it or delete the assertion. ``` -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. +## Tests Ship With the Implementation -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 TDD cycle — failing test, minimal implementation, refactor — is what +"complete" means. Ship the tests the behavior needs and only those: +trivial code and human prose earn none, and a test written to satisfy +process costs maintenance forever. ## 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: - Wrong constant or argument - 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 - 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. ## Quick Reference | When you... | Do | |-------------|-----| -| Write any test | Name the production change that would make it fail | -| Build an expected value | Derive it independently — literal or hand-checked fixture | +| Write any test | Name the break it catches — a bug, not a decision | +| Build an expected value | Derive it by hand; never with the code under test | +| Test a script or document | Run it / pressure-test its consumer; never grep its text | +| Reach for a dependency test | Test your boundary contract, not their documented mechanics | | Want to assert on a mocked element | Test the real component, or unmock it | -| 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 | +| Are about to mock a method | Learn its side effects; 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 | -| Finish an implementation | Tests already exist (TDD) — or it is unfinished | -| Finish a test file | Run the mutation check | +| Need cleanup only tests use | Put it in test utilities | | Watch mock setup balloon | Switch to an integration test with real components | +| Finish a test file | Run the mutation check | ## 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 - 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 -- 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 +- The test greps source text, or asserts a removed symbol stays removed +- The test would still matter if only the framework remained +- The test exists for coverage, checking no side effect or outcome +- 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"