# Reviewing Testability

> Diagnose why a module is hard to verify and fix the narrowest seam. Use when specs need heavy mocking, break on refactor, or survive plausible mutations — find missing DI seams, hidden state, temporal coupling, raw clocks. Triggers on "tests are brittle", "mocks everywhere", "too coupled".

- Skill: `ancplua/reviewing-testability` (Agent Skill)
- Install (CLI): `npx skillmds@latest add ancplua/reviewing-testability`
- Raw SKILL.md: https://api.skillmd.com/api/skills/ancplua/reviewing-testability/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: ANcpLua (https://skillmd.com/u/ancplua)
- Updated: 2026-09-10
- Page: https://skillmd.com/skills/ancplua/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:

```typescript
// 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`.