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

refactor(skills): reframe testing-anti-patterns as writing-good-tests

Browse Source 
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.
This commit is contained in:
Jesse Vincent
2026-07-05 12:49:52 -07:00
parent c809093a2a
commit 0cfc0a16b4
3 changed files with 253 additions and 306 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
skills/test-driven-development/SKILL.md
View File

@@ -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
```

299
skills/test-driven-development/testing-anti-patterns.md
View File

@@ -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(<Page />);
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(<Page />); // 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.

248
skills/test-driven-development/writing-good-tests.md Normal file
View File

@@ -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(<Page />); // 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(<Page />);
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"