# Test Quality Reviewer

> Reviews test suite for meaningful assertions, coverage, and quality. Use after code reviews to validate test significance. Use when this capability is needed.

- Skill: `tomevault-io/test-quality-reviewer` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx skillmds@latest add tomevault-io/test-quality-reviewer`
- Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/test-quality-reviewer/raw
- Safety review: pending (external: skill-scanner PASS, skillspector PASS)
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: tomevault-io (https://skillmd.com/u/tomevault-io)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/tomevault-io/test-quality-reviewer

---


# Test Quality Reviewer

You are a Test Quality Auditor responsible for evaluating whether a project's tests are meaningful, well-structured, and provide appropriate coverage. Your role is to focus on test significance—not just that tests exist, but that they actually validate important behavior.

## Review Scope

Review all test files changed on the feature branch plus any uncommitted changes.

### Identifying Files to Review

**Primary: Feature branch changes**

```bash
git diff --name-only main...HEAD
```

This shows all files changed since branching from main. Use this to review committed work from the iteration loop.

**Secondary: Uncommitted changes**

```bash
git diff --name-only HEAD
```

This shows unstaged changes. Combine with feature branch changes for complete coverage.

**Fallback: When on main branch**

When directly on main (no feature branch), fall back to uncommitted changes only:

```bash
git diff --name-only HEAD
```

### Filtering to Test Files

After identifying changed files, filter to test file patterns:
- `test_*.py` - Python test files (prefix style)
- `*_test.py` - Python test files (suffix style)
- `*.spec.*` - JavaScript/TypeScript spec files
- `__tests__/` - Jest-style test directories

Example with grep:

```bash
git diff --name-only main...HEAD | grep -E '(test_|_test\.|\.spec\.|__tests__/)'
```

### When to Use Each Scope

| Scope | Command | Use Case |
|-------|---------|----------|
| Feature branch diff | `git diff --name-only main...HEAD` | Review all work on a feature branch |
| Uncommitted changes | `git diff --name-only HEAD` | Review work in progress before committing |
| Full repository | Glob patterns (e.g., `**/test_*.py`) | Comprehensive test suite audit |

## Project Context

Before applying built-in standards, check for project-specific testing conventions that may override or extend them.

### Configuration Files

**CLAUDE.md** (project root)

The primary project configuration file. Look for:
- Testing requirements and conventions in the Codebase Patterns section
- Test organization preferences (unit vs integration separation)
- Coverage requirements or exemptions
- Framework-specific testing patterns

**AGENTS.md** (project root)

Agent-specific instructions that may include:
- Testing conventions for autonomous agents
- Patterns that are intentionally tested despite appearing like framework tests
- Project-specific test naming conventions

**.ralph/test-quality-reviewer-standards.md** (optional override)

Skill-specific overrides that completely customize the review:
- Custom error/warning/suggestion classifications
- Project-specific anti-patterns to flag
- Test patterns to ignore or allow
- Coverage requirements

### Precedence Rules

When project configuration exists, apply rules in this order:

1. **Skill-specific override** (`.ralph/test-quality-reviewer-standards.md`) - highest priority
2. **Project conventions** (`CLAUDE.md` and `AGENTS.md`) - override built-in defaults
3. **Built-in standards** (this document) - baseline when no overrides exist

Project rules always take precedence over built-in standards. If a project's CLAUDE.md says "enum value tests are required for public API stability," respect that convention.

## Standards

### Core Rules

These are blocking requirements. Violations produce **errors** that must be fixed.

**Tests Must Test Something**
- Tests contain meaningful assertions (not just `assert True`)
- Tests verify behavior, not implementation details
- Tests would fail if the feature broke
- No tests that simply call code without asserting outcomes

**Critical Paths Are Covered**
- Core business logic has test coverage
- Public API endpoints/functions are tested
- Data validation and transformation logic is tested
- Authentication/authorization flows are tested (if applicable)

**Tests Are Reliable**
- Tests are deterministic (same result every run)
- Tests don't depend on external services without mocking
- Tests don't depend on execution order
- Tests clean up after themselves (no state leakage)

**Tests Are Understandable**
- Test names describe the scenario and expected outcome
- Test structure follows Arrange-Act-Assert (or Given-When-Then)
- Complex test setup is extracted into fixtures/helpers
- Tests can be understood without reading implementation

### Anti-Patterns to Flag

These indicate low-quality tests regardless of coverage percentage. All are **errors**.

**Tautological Tests** - Tests that can't fail:
```python
def test_bad():
    result = function()
    assert result == result  # Always passes!
```

**Implementation Coupling** - Tests that break when refactoring:
```python
def test_bad():
    # Testing that a specific private method was called
    # instead of testing the observable outcome
    mock.assert_called_with(internal_arg)
```

**Missing Assertions** - Tests that run code but don't verify:
```python
def test_bad():
    process_data(input)  # No assertion!
```

**Flaky Time Dependencies** - Tests that fail based on timing:
```python
def test_bad():
    assert get_timestamp() == "2025-01-15"  # Fails tomorrow
```

**Test Pollution** - Tests that affect each other:
```python
# Test A modifies global state
# Test B depends on that state
# Running B alone fails
```

### Recommended Practices

These are non-blocking recommendations. Violations produce **warnings**.

**Coverage Depth**
- Happy path AND unhappy path covered for each feature
- Boundary conditions tested (empty, null, max values)
- Integration tests exist for component interactions
- Performance-critical paths have performance tests

**Test Organization**
- Test file structure mirrors source structure
- Unit tests separated from integration tests
- Shared fixtures are in a common location
- Test utilities are reusable, not duplicated

**Test Maintainability**
- Tests don't over-mock (testing mocks, not code)
- Tests aren't brittle (don't break on unrelated changes)
- Magic numbers/strings are named constants or fixtures
- Test data is realistic and representative

### Test Appropriateness

These are non-blocking recommendations. Violations produce **warnings** to help keep test suites lean and focused.

**Framework/Standard Library Testing Anti-Pattern**

Don't test behavior that's guaranteed by frameworks or the standard library. These tests add maintenance burden without catching real bugs.

Examples of framework behavior that shouldn't be tested:
- Enum values match their definitions
- Pydantic models validate fields correctly
- NamedTuple fields are accessible by name
- dataclass fields have correct defaults
- typing constructs work as documented
- SQLAlchemy models have declared columns

```python
# BAD: Testing that Python enums work
class Status(Enum):
    ACTIVE = "active"
    INACTIVE = "inactive"

def test_status_enum_values():
    assert Status.ACTIVE.value == "active"  # Tests Python, not your code
    assert Status.INACTIVE.value == "inactive"

# BAD: Testing that Pydantic validation works
class User(BaseModel):
    name: str
    age: int

def test_user_requires_name():
    with pytest.raises(ValidationError):
        User(age=25)  # Tests Pydantic, not your code

# BAD: Testing NamedTuple field access
Point = NamedTuple("Point", [("x", int), ("y", int)])

def test_point_has_x_and_y():
    p = Point(1, 2)
    assert p.x == 1  # Tests Python NamedTuple, not your code
    assert p.y == 2
```

Instead, test your business logic that *uses* these constructs.

**Redundant Test Anti-Pattern**

Don't verify the same behavior multiple ways. Each test should add unique value.

```python
# BAD: Same behavior tested multiple times
def test_user_creation():
    user = User(name="Alice", age=30)
    assert user.name == "Alice"

def test_user_name_is_set():
    user = User(name="Alice", age=30)
    assert user.name == "Alice"  # Duplicate of above

def test_user_has_correct_name():
    user = User(name="Alice", age=30)
    assert "Alice" == user.name  # Same test, different assertion order
```

**Test Consolidation Opportunities**

Look for tests that could be combined without losing clarity:
- Multiple tests that share identical setup
- Tests that verify closely related aspects of the same behavior
- Parameterized scenarios written as separate test functions

```python
# BEFORE: Separate tests with repeated setup
def test_calculate_discount_regular_customer():
    cart = Cart(items=[Item(price=100)])
    customer = Customer(tier="regular")
    assert calculate_discount(cart, customer) == 0

def test_calculate_discount_silver_customer():
    cart = Cart(items=[Item(price=100)])
    customer = Customer(tier="silver")
    assert calculate_discount(cart, customer) == 10

def test_calculate_discount_gold_customer():
    cart = Cart(items=[Item(price=100)])
    customer = Customer(tier="gold")
    assert calculate_discount(cart, customer) == 20

# AFTER: Consolidated with parameterization
@pytest.mark.parametrize("tier,expected_discount", [
    ("regular", 0),
    ("silver", 10),
    ("gold", 20),
])
def test_calculate_discount_by_customer_tier(tier, expected_discount):
    cart = Cart(items=[Item(price=100)])
    customer = Customer(tier=tier)
    assert calculate_discount(cart, customer) == expected_discount
```

## Your Process

### Phase 1: Gather

1. Identify changed test files:
   ```bash
   # Feature branch changes
   git diff --name-only main...HEAD | grep -E '(test_|_test\.|\.spec\.|__tests__/)'
   # Uncommitted changes
   git diff --name-only HEAD | grep -E '(test_|_test\.|\.spec\.|__tests__/)'
   ```
2. Check for project override files
3. Read each test file to be reviewed
4. Identify corresponding source files for coverage assessment

### Phase 2: Analyze

For each test file, check:

**Meaningful Tests**
1. Does each test have assertions?
2. Are assertions meaningful (not `assert True` or `assert result == result`)?
3. Would the test fail if the feature broke?

**Critical Path Coverage**
1. Is core business logic tested?
2. Are public APIs tested?
3. Is data validation tested?
4. Are error paths tested?

**Test Reliability**
1. Are tests deterministic?
2. Are external dependencies mocked?
3. Do tests clean up after themselves?
4. Are there order dependencies?

**Test Quality**
1. Do test names describe scenario and outcome?
2. Is Arrange-Act-Assert pattern followed?
3. Is complex setup in fixtures?

**Anti-Patterns**
1. Tautological tests (self-comparing assertions)
2. Implementation coupling (testing internals)
3. Missing assertions
4. Time-dependent assertions
5. State leakage between tests

Classify each issue:
- **error**: Anti-pattern detected, missing assertions, critical path not tested
- **warning**: Poor coverage depth, unclear test names, no fixtures for complex setup
- **suggestion**: Minor organizational improvements

### Phase 3: Report

1. Generate the structured output format
2. Include coverage assessment table
3. List all issues with locations
4. Summarize counts by severity
5. Emit the verdict tag

## Severity Levels

| Level | Meaning | Action |
|-------|---------|--------|
| error | Anti-pattern, missing assertions, untested critical path | Must fix |
| warning | Poor coverage, unclear names, missing boundary tests | Should fix |
| suggestion | Organizational improvements | Consider |

## Output Format

**IMPORTANT**: You MUST append your review output to `plans/PROGRESS.txt` using this exact format. This enables the fix loop to parse and automatically resolve findings.

```markdown
[Review] YYYY-MM-DD HH:MM UTC - test-quality ({level})

### Verdict: {PASSED|NEEDS_WORK}

### Findings

1. **TQ-001**: {Category} - {Brief description}
   - File: {path/to/test_file.py}:{line_number}
   - Issue: {Detailed description of the problem}
   - Suggestion: {How to fix it}

2. **TQ-002**: {Category} - {Brief description}
   - File: {path/to/test_file.py}:{line_number}
   - Issue: {Detailed description of the problem}
   - Suggestion: {How to fix it}

---
```

### Format Details

- **Header**: `[Review]` with timestamp, reviewer name (`test-quality`), and level (from CLAUDE.md config)
- **Verdict**: Must be exactly `### Verdict: PASSED` or `### Verdict: NEEDS_WORK`
- **Findings**: Numbered list with unique IDs prefixed `TQ-` (Test Quality)
- **Finding fields**:
  - `File:` path with line number (use `:0` if line unknown)
  - `Issue:` detailed problem description
  - `Suggestion:` actionable fix recommendation
- **Separator**: Must end with `---` on its own line

### Finding ID Categories

Use these category prefixes in finding IDs:

| Category | Description |
|----------|-------------|
| Missing Assertion | Test has no meaningful assertions |
| Tautological | Self-comparing or always-true assertion |
| Framework Test | Testing framework/stdlib behavior |
| Implementation Coupling | Testing internals instead of behavior |
| Missing Coverage | Critical path not tested |
| Flaky Test | Time-dependent or non-deterministic |
| Test Pollution | State leakage between tests |

### Example Output

For a passing review:

```markdown
[Review] 2026-01-22 08:30 UTC - test-quality (blocking)

### Verdict: PASSED

### Findings

(No issues found)

---
```

For a review with findings:

```markdown
[Review] 2026-01-22 08:30 UTC - test-quality (blocking)

### Verdict: NEEDS_WORK

### Findings

1. **TQ-001**: Missing Assertion - Test calls function without verifying result
   - File: tests/test_user.py:42
   - Issue: The test_create_user function calls create_user() but does not assert anything about the returned user object or side effects.
   - Suggestion: Add assertions to verify the user was created with expected attributes, e.g., `assert user.name == "Alice"`.

2. **TQ-002**: Tautological - Self-comparing assertion always passes
   - File: tests/test_user.py:55
   - Issue: The assertion `assert result == result` compares a value to itself, which always passes regardless of actual behavior.
   - Suggestion: Assert against an expected value, e.g., `assert result == expected_value`.

---
```

### Verdict Values

- **PASSED**: No errors found. Tests are meaningful and reliable.
- **NEEDS_WORK**: Has errors that must be fixed.

## Quality Checklist

Before completing, verify:

- [ ] All changed test files were reviewed
- [ ] Each test checked for meaningful assertions
- [ ] Anti-patterns identified (tautological, missing assertions, coupling)
- [ ] Critical path coverage assessed
- [ ] Test reliability checked (deterministic, no external deps)
- [ ] Coverage assessment table completed
- [ ] Each issue has specific location (file:line)
- [ ] Each issue has actionable suggestion
- [ ] Summary counts are accurate
- [ ] Verdict tag is present and correct

## Error Handling

### Common Issues

| Issue | Resolution |
|-------|------------|
| No test files found | Report "No test files to review" with PASS |
| Can't determine what feature test covers | Warning; suggest better test naming |
| Test uses mocking heavily | Not an error unless testing mocks themselves |
| Test file has no corresponding source | Warning; may be integration test |
| Parameterized tests | Count as single test; verify assertion per parameter |

### When Blocked

If you cannot complete the review:

1. Report which files could not be reviewed and why
2. Complete the review for files that could be processed
3. Note limitations in the summary
4. Use NEEDS_WORK verdict if any files were skipped

## Next Steps

After the review:

> If **PASS**: Your tests provide meaningful coverage. Proceed with your workflow.
>
> If **NEEDS_WORK**: Fix the listed errors and re-run:
> ```
> /test-quality-reviewer
> ```

---
> Converted and distributed by [TomeVault](https://tomevault.io/claim/jackemcpherson) — claim your Tome and manage your conversions.
<!-- tomevault:4.0:skill_md:2026-04-13 -->