writing-good-tests

What this skill does

# Writing Good Tests

## Philosophy

**"Write tests. Not too many. Mostly integration."** — Kent C. Dodds

Tests verify real behavior, not implementation details. The goal is confidence that your code works. Coverage numbers are neccessary but not sufficient.

**Core principles:**

1. Test behavior, not implementation — refactoring shouldn't break tests
2. Integration tests provide better confidence-to-cost ratio than unit tests
3. Wait for actual conditions, not arbitrary timeouts
4. Mock strategically — real dependencies when feasible, mocks for external systems
5. NEVER pollute production code with test-only methods

## Test Structure

Always prefer and use table driven tests, with a single test function that is then given a name, fed an input, and then asserts output and error return equality.

Once this is implemented, test cases should be cheap and easy to add. Ensure there are cases for all possible success and error cases. If possible exhaustively test for all side effects. If a side effect is contained within the function that is being tested and not part of the return, but has a material effect on how the function behaves consider mocking. This could also be a potential bad code smell, and a refactor of the function to make it more testable should be considered.

```go
package foo

import (
	"errors"
	"fmt"
	"testing"
)

var (
	ErrEmpty      = errors.New("empty")
	ErrNotInt     = errors.New("not an int")
	ErrOutOfRange = errors.New("out of range")
)

func TestParsePort(t *testing.T) {
	t.Parallel()

	type tc struct {
		name string
		in   string
		want int
		err  error
	}

	tests := []tc{
		{
			name: "valid_low",
			in:   "1",
			want: 1,
		},
		{
			name: "valid_high",
			in:   "65535",
			want: 65535,
		},
		{
			name: "empty",
			in:   "",
			err:  ErrEmpty,
		},
		{
			name: "not_int",
			in:   "abc",
			err:  ErrNotInt,
		},
		{
			name: "out_of_range_zero",
			in:   "0",
			err:  ErrOutOfRange,
		},
		{
			name: "out_of_range_too_big",
			in:   "70000",
			err:  ErrOutOfRange,
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			t.Parallel()

			got, err := ParsePort(tt.in)

			if !errors.Is(err, tt.err) {
				t.Fatalf("error mismatch: want %v, got %v", tt.err, err)
			}

			if tt.err != nil {
				return
			}

			if got != tt.want {
				t.Fatalf("value mismatch: want %d, got %d", tt.want, got)
			}
		})
	}
}
```

**One action per test.** Multiple assertions are fine if they verify the same behavior.

## Condition-Based Waiting

Flaky tests often guess at timing. This creates race conditions where tests pass locally but fail in CI.

**Wait for conditions, not time:**

```typescript
// BAD: Guessing at timing
await new Promise((r) => setTimeout(r, 50));
const result = getResult();

// GOOD: Waiting for condition
await waitFor(() => getResult() !== undefined);
const result = getResult();
```

### Generic Polling Function

```typescript
async function waitFor<T>(
  condition: () => T | undefined | null | false,
  description: string,
  timeoutMs = 5000,
): Promise<T> {
  const startTime = Date.now();

  while (true) {
    const result = condition();
    if (result) return result;

    if (Date.now() - startTime > timeoutMs) {
      throw new Error(
        `Timeout waiting for ${description} after ${timeoutMs}ms`,
      );
    }

    await new Promise((r) => setTimeout(r, 10)); // Poll every 10ms
  }
}
```

### Quick Patterns

| Scenario       | Pattern                                              |
| -------------- | ---------------------------------------------------- |
| Wait for event | `waitFor(() => events.find(e => e.type === 'DONE'))` |
| Wait for state | `waitFor(() => machine.state === 'ready')`           |
| Wait for count | `waitFor(() => items.length >= 5)`                   |

### When Arbitrary Timeout IS Correct

Only when testing actual timing behavior (debounce, throttle, intervals):

```typescript
// Testing tool that ticks every 100ms
await waitForEvent(manager, "TOOL_STARTED"); // First: wait for condition
await new Promise((r) => setTimeout(r, 200)); // Then: wait for 2 ticks
// Comment explains WHY: 200ms = 2 ticks at 100ms intervals
```

## Mocking Strategy

> "You don't hate mocks; you hate side-effects." — J.B. Rainsberger

Mocks reveal where side-effects complicate your code. Use them strategically, not reflexively.

### Don't Mock What You Don't Own

Create thin wrappers around third-party libraries. Mock YOUR wrapper, not the library.

```typescript
// BAD: Mock the HTTP client directly
const mockClient = vi.mocked(httpx.Client);

// GOOD: Create your own wrapper
class RegistryClient {
  constructor(private client: HttpClient) {}
  async getRepos() {
    return this.client.get("https://registry.example.com/v2/_catalog");
  }
}

// Mock your wrapper
vi.mock("./registry-client");
```

This simplifies tests AND improves your design.

### Managed vs Unmanaged Dependencies

| Dependency Type              | Example                             | Strategy           |
| ---------------------------- | ----------------------------------- | ------------------ |
| **Managed** (you control it) | Your database, your file system     | Use REAL instances |
| **Unmanaged** (external)     | Third-party APIs, SMTP, message bus | Use MOCKS          |

Communications with managed dependencies are implementation details — you can refactor them freely. Communications with unmanaged dependencies are observable behavior — mocking protects against external changes.

### Anti-Pattern: Testing Mock Behavior

```typescript
// BAD: Testing that the mock exists
test('renders sidebar', () => {
  render(<Page />);
  expect(screen.getByTestId('sidebar-mock')).toBeInTheDocument();
});

// GOOD: Test real behavior
test('renders sidebar', () => {
  render(<Page />);
  expect(screen.getByRole('navigation')).toBeInTheDocument();
});
```

**Gate:** Before asserting on any mock element, ask: "Am I testing real behavior or mock existence?"

### Anti-Pattern: Mocking Without Understanding

```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!
});

// GOOD: Mock at correct level
test("detects duplicate server", () => {
  vi.mock("MCPServerManager"); // Just mock slow server startup
  await addServer(config); // Config written
  await addServer(config); // Duplicate detected
});
```

**Gate:** Before mocking, ask: "What side effects does this have? Does my test depend on them?"

### Anti-Pattern: Incomplete Mocks

Mock the COMPLETE data structure as it exists in reality:

```typescript
// BAD: Partial mock
const mockResponse = {
  status: "success",
  data: { userId: "123" },
  // Missing: metadata that downstream code uses
};

// GOOD: Mirror real API
const mockResponse = {
  status: "success",
  data: { userId: "123", name: "Alice" },
  metadata: { requestId: "req-789", timestamp: 1234567890 },
};
```

### When Mocks Become Too Complex

Warning signs:

- Mock setup longer than test logic
- Mocking everything to make test pass
- Test breaks when mock changes

> "As the number of mocks grows, the probability of testing the mock instead of the desired code goes up." — Codurance

Consider integration tests with real components — often simpler than elaborate mocks.

### Anti-Pattern: Test-Only Methods in Production

```typescript
// BAD: destroy() only used in tests
class Session {
  async destroy() {
    /* cleanup */
  }
}

// GOOD: Test utilities handle cleanup
// test-utils/session-helpers.ts
export async function cleanupSession(session: Session) {
  const workspace = session.getWorkspaceInfo();
  if (workspace) {
    await workspaceManager.destroyWorkspace(workspace.id);
  }
}
```

**Gate:** Before adding any method to production class, ask: "Is this only