Reviewing Testability

Reviewing Testability

Core principle

Weak tests are usually a symptom of bad seams, not bad testers. If the only way to observe an outcome is by mocking everything in sight or peeking at private state, the architecture is forcing the weakness.

When this skill applies

• A test file has more lines of mock setup than assertion.
• Tests break whenever the implementation is refactored without behavior changing.
• Tests pass when the code is mutated in plausible ways (the suite is permissive).
• You find yourself adding resetForTesting() or exposing internals "just for tests".
• senior-tester-judge returned many WEAK verdicts that trace to the same file.

Diagnostic checklist

Walk the target module. For each of these, answer yes/no:

Each "yes" is a seam missing at a specific line. Name it, cite it, and propose the narrowest refactor that introduces the seam.

The non-injectable clock is the most common one. The untestable version forces a test runner (jest, vitest, or pytest) to monkey-patch global time; the seam lets a fake pass through the same door as production:

// Untestable: reads the clock directly, no seam.
function isExpired(token: Token): boolean {
  return token.expiresAt < Date.now();
}

// Testable: clock injected, so vitest/jest passes a fixed value.
function isExpiredWithClock(token: Token, now: () => number): boolean {
  return token.expiresAt < now();
}

// Test asserts the boundary instead of patching globals.
expect(isExpiredWithClock(token, () => 1000)).toBe(false);

Refactor directions (narrowest-first)

• Direct new → constructor-inject an interface. Test passes a fake through the same door as production.
• Decision + I/O tangled → extract the decision as a pure function. Test the decision with a value; test the I/O orchestration with one test asserting the wiring.
• Hidden state → move to per-instance state. If it must be global, inject a reset hook that is not public API but is called by the test harness.
• Direct clock → inject TimeProvider / () => Date / Clock interface. Production uses TimeProvider.System; test uses a fake.
• Fire-and-forget async → return a promise, or expose a status property that transitions observably.
• Statics → wrap in an interface injected at construction.
• Implementation peeking → ask "what public outcome does this state represent?" If none, the state should not exist. If one exists, assert on the outcome instead.

When to NOT refactor

• The code is third-party and you cannot modify it. Wrap it in an adapter interface; test the adapter's behavior only.
• The "smell" has no test consequence. If tests are strong and mutation- resistant despite the shape, leave it. Architecture is a means, not an end.
• The cost of the refactor exceeds the value. A one-line Date.now() fix is worth it; rewriting a 500-line legacy module to split decision from I/O might not be — hand that to a planned migration.

What this skill will NOT tell you to do

• Apply SOLID broadly.
• Introduce abstractions that hide complexity rather than expose seams.
• Refactor code whose tests are already mutation-resistant.
• Add "testability hooks" that widen the public API.

Output

When invoked as a skill-only reference, the reviewer (human or agent) produces a ranked list of smells with:

1. Smell name and file:line.
2. The specific test weakness it forces (named mutation that survives).
3. The narrowest seam that fixes it.
4. What high-signal tests become possible after the fix.

Rank by test-consequence severity, not by refactor effort.

Related

• For grading the tests themselves: see judging-test-quality.
• For rewriting specific weak tests: see improving-weak-tests.
• For end-to-end branch closure: see mutation-resistant-coverage.