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:

Question	If yes, smell
Does the unit under test new its collaborators directly?	Missing DI seam
Are decisions and side effects in the same function body?	Missing pure/impure split
Is there module-level or static state that leaks between tests?	Hidden state
Does the public API expose getCache, _internal, or resetForTesting?	Over-broad public surface
Do tests depend on the order of prior tests?	Temporal coupling
Are Date.now() / DateTime.Now / new Date() called directly?	Non-injectable clock
Does async completion have no observable signal from outside?	Fire-and-forget with no seam
Are static methods called on classes the test can't substitute?	Untestable statics
Do tests assert on internal counters or private maps?	Implementation peeking

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);