---
name: test-reviewer
description: "Review test coverage, assertions, isolation, and failure cases to assess release confidence."
---

# Test Reviewer

**Audience:** Developers reviewing test suites for quality and completeness.

**Goal:** Assess whether tests prove intended user workflows and independent behavior contracts, with meaningful assertions and failure detection rather than test count or method coverage.

## Review Steps

### 1. Identify Test Context

- Establish the review scope from the request, changed files, or provided diff
- Locate test files related to the changed behavior
- Identify the testing framework and repository conventions

### 2. Map Behavior Contracts to Tests

- Start from user workflows, acceptance criteria, documented contracts, and known failure traces, not the implementation's assumptions
- State the expected outcomes independently of the code; mark missing or ambiguous expectations as evidence gaps
- Read the implementation and relevant callers to trace how each behavior is exercised
- Create a coverage matrix mapping user workflows and behavior contracts to test cases, including the failure each case would detect
- Prefer end-to-end checks that prove a user can complete the workflow. Keep focused unit and integration tests when they detect additional real failures or isolate a failure that a broader check would miss
- Do not require a separate unit test for every public method or enforce a testing pyramid by habit
- Check that each required behavior is proven at its acceptance boundary: system or end-to-end tests through the real interface for user-facing flows, request or integration tests through the public boundary for APIs and non-UI workflows, and unit tests only for pure calculations, parsers, algorithms, and business rules
- Use the smallest boundary that proves the claim. If correctness depends on the database, service wiring, or browser interaction, a unit test with those parts mocked is not acceptance evidence
- Require each new unit test to state a concrete benefit: specific boundary cases or combinations that are costly at acceptance level, materially faster feedback for logic under active development, or more deterministic reproduction of a known defect. "Better coverage" is not a benefit

### 3. Analyze Test Coverage

For each user workflow or behavior contract, check if tests exist for:
- **Happy Path**: Normal successful execution flows
- **Sad Path**: Error conditions, validation failures, edge cases
- **Boundary Conditions**: Empty values, nil/null, maximum limits
- **State Transitions**: Before/after states, side effects
- **Authorization**: Permission checks (if applicable)
- **Integration Points**: External service calls, database operations

### 4. Evaluate Test Quality

- **Assertion Quality**:
  - Are assertions specific and meaningful?
  - Do tests verify behavior against an independent expectation, not mirror implementation details?
  - Would a plausible regression in the expected outcome make the test fail?
  - Do end-to-end checks exercise the real workflow rather than mock away the behavior they claim to verify?
  - Do acceptance tests assert the promised final outcome and contractual effects (for example, the amount charged), not only an intermediate display, status, or screen?
  - Where a test substitutes an external system, does it state what remains unproven, and does the substitute avoid supplying the business result the test claims to verify?
  - Are error messages helpful for debugging?
- **Test Isolation**:
  - Do tests depend on execution order?
  - Are external services properly mocked?
  - Is test data properly set up and torn down?
- **AAA Pattern**:
  - Clear Arrange (setup) section
  - Single Act (execution)
  - Focused Assert (verification)
- **Test Names**:
  - Do names describe expected behavior?
  - Would someone understand the test without reading code?

### 5. Check Test Patterns

- **Framework Compliance**:
  - RSpec: Proper use of `describe`, `context`, `it`, `let`, `before`
  - Minitest: Proper use of `setup`, `test`, fixtures
  - Jest: Proper use of `describe`, `it`, `beforeEach`, mocks
- **Project Conventions**:
  - Do tests follow existing patterns in the codebase?
  - Are shared examples/contexts used where appropriate?
  - Is test data handled consistently (fixtures vs factories)?

### 6. Identify Missing Scenarios

Recommend a missing test only when it would detect a failure that existing tests miss. State the test level and that failure for each recommendation. Prefer extending an acceptance test over adding a unit test that duplicates cases the acceptance tests already exercise reliably and cheaply.

Look for common gaps:
- **Nil/Empty Handling**: What happens with nil, empty strings, empty arrays?
- **Error Cases**: Network failures, database errors, validation failures
- **Concurrency**: Race conditions, parallel execution
- **Edge Cases**: First/last items, exactly at limits, off-by-one
- **Security**: Input sanitization, authorization bypasses
- **Performance**: Large data sets, timeout scenarios

### 7. Check for Test Anti-Patterns

- Skipped/pending tests without justification
- Tests with `sleep` or hardcoded delays (flaky test smell)
- Tests with non-deterministic data (random values without seeds)
- Over-reliance on `allow_any_instance_of` or similar broad mocks

**Note:** This skill performs static analysis only. Test execution is handled separately to avoid redundant test runs.

**Red Flags to Watch For:**
- Tests that always pass (no real assertions)
- Tests that test Rails/framework behavior, not application code
- Overly complex test setup indicating design issues
- Mocking too much (testing mocks, not real behavior)
- No error case coverage
- Tests tightly coupled to implementation details
- Agent-written tests that copy the implementation's assumptions without an independent contract
- Many passing unit tests with no check that the user can complete the workflow
- Redundant tests that add maintenance work without detecting additional failures
- Mocks of the project's own forms, parameters, controllers, jobs, or services where their connections determine correctness
- Acceptance expectations changed during a refactor or rewrite to match the new implementation without an independently established requirement change. Locator and setup changes are acceptable when the behavioral assertions stay the same
- Implementation-coupled or temporary tests kept as protected assets. Recommend rewriting or removing them when acceptance or regression protection at the right level remains
- Missing database state verification
- **Frozen fixture assertions**: Exact collection comparisons (`assert_equal [a, b], scope` or `expect(scope).to eq([a, b])`) that break when unrelated fixtures are added. Recommend `assert_includes`/`expect(...).to include(...)` instead.

## Test Quality Review Report

Provide your findings in this structure:

### Test Coverage Summary
- **Implementation Files Reviewed**: [count]
- **Test Files Reviewed**: [count]
- **Coverage Gaps Identified**: [count]
- **Quality Issues Found**: [count]

### Coverage Matrix
| Workflow/Contract | Expected Outcome Source | Test Level | Failure Detected | Coverage Gaps |
|-------------------|-------------------------|------------|------------------|---------------|
| [workflow] | [specification or failure trace] | [e2e/integration/unit] | [regression] | [missing or unverified behavior] |

Record coverage as present, missing, or unknown. This static review cannot establish that tests passed; report execution evidence separately when supplied.

### Critical Coverage Gaps
[List features/methods with missing test coverage, prioritized by risk]

1. **[Feature/Method Name]** - [file:line]
   - Missing: [what scenarios are not tested]
   - Risk: [why this gap matters]
   - Recommended: [specific test to add]

### Test Quality Issues

**High Priority**:
- [Issue description with file:line reference]
- Recommendation: [how to fix]

**Medium Priority**:
- [Issue description]
- Recommendation: [how to fix]

**Low Priority**:
- [Issue description]
- Recommendation: [how to fix]

### Missing Edge Cases
[List specific edge case scenarios that should be tested]

1. [Scenario]: [failure it would detect] → [test level] → [file to add test]

### Pattern Violations
[Tests that don't follow project conventions or best practices]

### Recommendations
1. [Prioritized actionable recommendations]
2. [Include specific test examples where helpful]
3. [Reference project patterns to follow]

### Overall Assessment
- **Test Suite Health**: [Excellent/Good/Needs Work/Critical Gaps]
- **Confidence Level**: [High/Medium/Low] - Can we ship with current tests?
- **Priority Actions**: [Top 3 things to fix before merging]

**Note**: Use clear repository-relative file paths and line numbers. Focus on actionable feedback that improves test quality and coverage.