Review Tests

---
name: review-tests
description: Review a test suite or test plan for coverage, quality, correctness, and maintainability.
---

# Review Tests

Audits a test suite or test plan and reports findings across five categories: coverage, quality, correctness, maintainability, and missing scenarios.

## Prerequisites

- Apply the shared SDLC conventions in `skills/sdlc/references/shared.md`.
- If no argument is provided, locate the feature directory under `.sdlc/features/` whose frontmatter `issue` field references `$ISSUE_NUMBER`.
- `.sdlc/features/N-<slug>/tests.md`, or a test suite/test plan provided in context or as a file path
- `.sdlc/features/N-<slug>/requirements.md` (optional, improves coverage analysis)

## Steps

1. Read the tests or test plan from `.sdlc/features/N-<slug>/tests.md` if present, otherwise from context or as a file path.
2. Map tests to requirements or acceptance criteria if a spec is available.
3. Identify issues in each category below.
4. Report findings. Omit categories with no findings.
5. Write the findings to `.sdlc/features/N-<slug>/review-tests.md` with frontmatter `artifact: tests`, `verdict` (`approved` if there are no blocking findings, `changes-requested` if the author must address findings, `rejected` for a fundamental flaw), and `reviewed_at: <ISO date>`, and the findings as the body, per `skills/sdlc/references/shared.md`. Record any unresolved open questions in the findings body. For any question that carries meaningful risk to the implementation, also invoke `/create-assumption` to record it formally.

## Review Checklist

### Coverage
- Are all acceptance criteria covered by at least one test?
- Are non-functional requirements (performance, security) verified?
- Are error paths and failure scenarios tested?

### Quality
- Do test names clearly describe what is being tested and the expected outcome?
- Are tests verifying behavior rather than implementation details?
- Is each test independent (no shared mutable state between tests)?
- Is test setup minimal and focused?

### Correctness
- Do assertions actually verify the intended behavior?
- Are assertions specific enough (not just `assert result is not None`)?
- Are mocks and stubs used appropriately without over-mocking?

### Maintainability
- Is test code DRY where it benefits readability?
- Are magic values extracted into named constants or fixtures?
- Will tests break on valid refactors (brittle tests)?

### Missing Scenarios
- Are edge cases covered (empty inputs, boundary values, max/min)?
- Are concurrency or race conditions tested where applicable?
- Are security-relevant paths tested (unauthorized access, injection)?

## Output Format

```markdown
## Coverage

<Findings or "No issues found.">

## Quality

<Findings or "No issues found.">

## Correctness

<Findings or "No issues found.">

## Maintainability

<Findings or "No issues found.">

## Missing Scenarios

<Findings or "No issues found.">
```

## Outcome

If `$OUTCOME_YAML` is set, emit your verdict there per `skills/sdlc/references/shared.md`:

| Verdict | When |
|---|---|
| `approved` | No blocking findings; the subject passes review |
| `changes-requested` | Findings the author must address before it passes |
| `rejected` | Fundamental flaw requiring rework or stopping |

In the same emission, list the findings file under `artifacts:` (`.sdlc/features/N-<slug>/review-tests.md`).

## Example Usage

**Scenario 1: Missing error path**
Tests cover the happy path for user registration but nothing tests what happens when the email already exists.
Report under Missing Scenarios.

**Scenario 2: Brittle test**
A test asserts the exact SQL query string generated by an ORM.
This will break on any ORM upgrade without a behavior change. Report under Maintainability.

**Scenario 3: Weak assertion**
`assert response.status_code == 200` without checking the response body.
Report under Correctness.

## Next Step

Once the findings verdict is `approved`, continue with `/create-implementation`.