diff --git a/skills/test-driven-development/SKILL.md b/skills/test-driven-development/SKILL.md index 60d2609c..158cb0b5 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 e77ab6b6..00000000 --- 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 00000000..ad8c6023 --- /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"