go-test-review

Go Test Review

Purpose

Audit Go test code for quality and coverage effectiveness. Reviews HOW tests are written — not the production code being tested.

This skill is conditionally triggered — only when _test.go files are in the diff. If a PR has only implementation code with no tests, this skill may suggest "missing test coverage" but does not deep-dive.

This skill does NOT cover: security, concurrency, performance, quality, error handling, or logic of production code — those belong to sibling vertical skills.

When To Use

• PR contains _test.go files
• Code uses httptest, testing.B, testing.F
• Code includes testdata/ directory changes
• Need to evaluate test coverage

When NOT To Use

• Reviewing production code security/performance/logic → use corresponding sibling skill
• Only implementation code changed, no tests → may note "missing tests" but no deep review

Mandatory Gates

1) Go Version Gate

Read go.mod. Key version gates:

2) Anti-Example Suppression Gate

MUST quote specific evidence. Category match alone insufficient.

Embedded anti-examples:

• "Testing standard library behavior" — test that json.Marshal produces valid JSON, or that strings.Contains works. These test Go's stdlib, not your code.
• "Test only asserts err == nil" — BUT: do NOT flag if function genuinely has no meaningful return value (void-like operations where error is the only output, e.g., Close(), Flush()).
• "Should use integration test" — when unit test with mock is the correct choice for fast, isolated testing. Not every test needs a real database.
• "Missing test for unexported function" — when function is simple helper fully covered by exported function tests.
• "Missing benchmark" — when code is not on a hot path and benchmarking provides no actionable insight.

3) Generated Code Exclusion Gate

mock_*.go from mockgen: review for usage patterns only, not mock implementation itself.

Workflow

1. Define scope — identify _test.go files in diff.
2. Run go test -cover for impacted packages — record coverage percentage.
3. Load references — always load go-test-quality.md.
4. Evaluate ALL 10 checklist items.
5. Apply suppression → format output.

Grep-Gated Execution Protocol

This skill uses mechanical grep pre-scanning to guarantee zero missed checklist items. 8 of 10 items are grep-gated; 2 are semantic-only.

Execution Order

1. Identify target test files (from dispatch prompt)
2. Run grep for all grep-gated checklist items against target files
3. HIT → run semantic analysis to confirm or reject
4. MISS → auto-mark NOT FOUND, skip semantic analysis
5. For compound patterns (items 1, 2): run both grep patterns, apply logic
6. For semantic-only items (items 4, 9): full model reasoning
7. Report only FOUND items

Grep Audit Line

Include in Execution Status: Grep pre-scan: X/8 items hit, Z confirmed as findings (2 semantic-only)

Test Quality Checklist (10 Items)

Severity Rubric

High — Missing critical coverage (changed behavior untested), assertion that can never fail (false confidence).

Medium — Test quality issue reducing diagnostic value but not creating false confidence.

Evidence Rules

• For coverage gaps: identify which changed behavior lacks test coverage
• For weak assertions: show what the test checks vs what it should check
• For false-confidence: show why the assertion always passes regardless of implementation
• Merge rule: same pattern at ≥3 tests → one finding with location list

Output Format

Findings

[High|Medium] Short Title

• ID: TEST-NNN
• Location: path:line
• Impact: What could be missed by this test gap
• Evidence: Missing assertion/boundary/pattern
• Recommendation: Specific test improvement with code example
• Action: must-fix | follow-up

Suppressed Items

[Suppressed] Short Title

• Reason: Anti-example matched + evidence cited

Execution Status

• Go version: X.Y
• Grep pre-scan: X/8 items hit, Z confirmed as findings (2 semantic-only)
• go test -cover: coverage% for impacted packages
• References loaded: list

Summary

1-2 lines. Count by severity + coverage status.

Example Output

### Findings

#### [High] False-Confidence Assertion — Only Checks err == nil
- **ID:** TEST-001
- **Location:** `internal/service/user_test.go:45`
- **Impact:** Test passes even if CreateUser returns wrong user — only error checked, return value ignored
- **Evidence:** `err := svc.CreateUser(ctx, input); assert.NoError(t, err)` — no assertion on returned User (name, email, ID)
- **Recommendation:**
  ```go
  user, err := svc.CreateUser(ctx, input)
  assert.NoError(t, err)
  assert.Equal(t, input.Name, user.Name)
  assert.Equal(t, input.Email, user.Email)
  assert.NotEmpty(t, user.ID)

• Action: must-fix

[Medium] Missing Boundary Cases in Table-Driven Test

• ID: TEST-002
• Location: internal/validator/email_test.go:20-55
• Impact: Edge cases could slip through — only happy path and obvious invalid tested
• Evidence: Cases: "[email protected]", "invalid", "" — missing: Unicode local part, max-length (254 chars), consecutive dots, leading/trailing spaces
• Recommendation: Add: {"unicode: ü@domain.com", true}, {"max-length-254", ...}, {" [email protected]", false}
• Action: follow-up

Execution Status

• Go version: 1.21
• go test -cover: internal/service 72%, internal/validator 85%
• References loaded: go-test-quality.md

Summary

1 High (false-confidence assertion), 1 Medium (missing boundary cases). Coverage: service 72% (below 80% threshold).

## No-Finding Case
If no issues found: state `No test quality findings identified.` Still output coverage numbers in Execution Status.

## Load References Selectively

| Reference | Load When |
|-----------|-----------|
| `references/go-test-quality.md` | Always |
| `references/go-review-anti-examples.md` | Always |

## Review Discipline
- **Test quality only** — not production code security/performance/logic
- Execute ALL 10 checklist items
- Coverage threshold: 80% for business logic packages — flag if below
- Do not review mock implementation code (generated mocks); review mock usage patterns