# PR Test Coverage Review

> Review pull requests for test coverage like a senior SDET, map the diff to required test classes, spot untested branches and missing regression tests, judge test quality not just presence, and write actionable review comments.

- Skill: `pramoddutta/pr-test-coverage-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add pramoddutta/pr-test-coverage-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/pramoddutta/pr-test-coverage-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- License: MIT
- Author: PramodDutta (https://skillmd.com/u/pramoddutta)
- Updated: 2026-09-10
- Page: https://skillmd.com/skills/pramoddutta/pr-test-coverage-review

---


# PR Test Coverage Review Skill

You are a senior SDET reviewing pull requests specifically for test coverage and test quality. When asked to review a PR, diff, or branch, follow this procedure and produce concrete, actionable findings.

## Core Principles

1. **The diff defines the obligation.** Every behavior change in the PR creates a specific testing debt; enumerate it before reading the tests.
2. **Presence is not coverage.** A test file touching the changed module proves nothing; the NEW branches and edge cases must be exercised.
3. **Bug fixes REQUIRE a regression test.** A fix without a failing-then-passing test is the top predictor of the bug returning.
4. **Judge tests as code.** Tautological, over-mocked, or assertion-free tests are negative value; call them out.
5. **Comments must be actionable.** Every finding names the file, the untested path, and the concrete case to add.

## Review Procedure

### Step 1: Classify each change in the diff

| Change type | Testing obligation |
|---|---|
| New function/endpoint | Happy path + boundaries + error contract |
| Changed conditional/branch | Both sides of the new/modified branch |
| Bug fix | Regression test reproducing the original bug |
| New error handling | Test that triggers the error path |
| Schema/type change | Serialization + validation + migration cases |
| Config/feature flag | Behavior with flag on AND off |
| Refactor (claimed no-behavior-change) | Existing tests pass UNCHANGED; edited assertions are a red flag |
| Concurrency/async change | Rejection, timeout, ordering cases |
| Removed code | Corresponding dead tests removed too |

### Step 2: Map obligations to the tests in the PR

For each obligation, find the covering test. Practical commands:

```bash
gh pr diff 123 --name-only                      # changed files
gh pr diff 123 | grep -E '^\+.*\b(if|catch|throw|raise|case )' | head -30
# new branches introduced; each needs both sides covered

# does a changed source file have a changed test file?
gh pr diff 123 --name-only | grep -v test > /tmp/src.txt
gh pr diff 123 --name-only | grep -E '(test|spec)' > /tmp/tests.txt
```

Coverage tooling as evidence, not verdict: run the suite with coverage on the PR branch and inspect the changed lines specifically (`vitest --coverage` + diff-cover, or `pytest --cov` + `diff-cover coverage.xml --compare-branch=main`). Diff coverage under ~80% almost always hides an untested branch worth naming; 100% diff coverage can still miss behavior (see step 3).

### Step 3: Judge the quality of the tests that exist

Red flags to flag explicitly:

- **Tautology:** expectation recomputes the implementation (`expect(fn(x)).toBe(sameFormulaInline)`)
- **Over-mocking:** the unit's own collaborators mocked, so the test verifies the mock
- **Assertion-free:** calls the function, asserts nothing (or only `toBeDefined`)
- **Snapshot dumping:** giant snapshots instead of targeted assertions on the changed behavior
- **Edited assertions in a "refactor":** behavior changed silently; ask which is intended, old or new
- **Happy-path-only for error-handling PRs:** the new catch/except never triggered
- **Flake bait:** real time, real network, order-dependent tests

### Step 4: Write the review

Comment format, one per finding:

```text
[tests] payment.ts:42 introduces the `card.expired` branch; no test exercises it.
Add: "declines expired card with CARD_EXPIRED error" in payment.spec.ts,
fixture with expiry in the past, assert error code + no charge call.
```

Severity ladder: BLOCKER (bug fix without regression test; new error path untested on a money path), MAJOR (new branch one-sided; over-mocked core logic), MINOR (naming, missing boundary on non-critical path). Approve only when blockers and majors are resolved or explicitly risk-accepted by the owner.

## Worked Example

Diff adds to `refund.ts`:

```typescript
if (order.ageDays > 30) {
  if (order.plan === 'annual') return partialRefund(order);   // NEW
  throw new RefundWindowError(order.ageDays);
}
```

Obligations derived: (1) annual + over-30 returns partial (new happy path), (2) monthly + over-30 still throws (old behavior preserved), (3) annual + exactly 30 and 31 (boundary of the OUTER condition now matters for a new reason), (4) partialRefund amount correctness (what fraction?), (5) if this PR fixes a reported bug, the ticket's exact scenario as a named regression test.

PR contains only: `it('refunds annual plans', ...)`. Review verdict: MAJOR x2 (obligations 2 and 3 untested), question on 4 (amount unasserted), BLOCKER if a linked bug ticket exists without its regression case.

## CI Support for This Review

- Enforce diff coverage threshold (diff-cover, Codecov patch status) so line-level gaps surface automatically; reviewers then spend attention on quality, not counting
- Label PRs `needs-regression-test` automatically when the title/branch references a bug ticket but no test file changed
- Mutation testing (Stryker, mutmut) on core modules weekly; surviving mutants seed review checklists

## Common Mistakes

- Reviewing test PRESENCE ("has tests, LGTM") instead of mapping diff obligations
- Demanding tests for pure refactors while missing that assertions were edited
- Coverage percentage worship; 100% diff coverage with tautological tests is worse than 70% with real ones
- Vague comments ("add more tests"); every comment names the branch and the case
- Blocking on style nits while a money-path error branch ships untested

## Checklist

- [ ] Every diff change classified against the obligation table
- [ ] Bug fix PRs contain a reproducing regression test (verified it fails on main)
- [ ] Both sides of every new/changed branch exercised
- [ ] Test quality reviewed: no tautologies, over-mocking, assertion-free tests
- [ ] Findings written with file:line, missing case, and severity; blockers gate merge