ai-qa-review

Files (4)

• SKILL.md
• mutation-testing.md · references/
• smell-examples.md · references/
• testability-refactors.md · references/

SKILL.md (first lines)

<objective>
QA-focused code review that detects test smells, analyzes testability of application code, and identifies coverage gaps. A test that asserts `toBe(true)` and a 95%-coverage suite that only feeds happy-path input both look green and both hide bugs — this skill names the smell, cites the line, and converts "looks fine" into a mutation-score gate.

**Before starting:** Check for `.agents/qa-project-context.md` in the project root. It contains test framework conventions, naming patterns, and project-specific quality standards that calibrate review feedback.
</objective>

---

## Quick Route

Three distinct entry paths. Pick the row, then jump to the named section.

| Situation | Path | Jump to |
|-----------|------|---------|
| PR with changed test files | Run the changed files, score them, check the diff against the PR checklist | **Verification** → **PR Review Checklist** |
| Whole suite needs a health pass | Quantify, sample, find the 3-5 systemic smells, propose lint/mutation gates | **Batch Audit Process** |
| Application code, "why is this hard to test?" | Flag DI / side-effect / pure-function / interface problems with before/after | **Testability Analysis** |

All three share the same smell vocabulary (the six buckets below) and the same Verification commands.

---

## Discovery Questions

First, read `.agents/qa-project-context.md` if present and skip any question it already answers.

1. **Review scope:** Reviewing test code for quality, application code for testability, or both? Each triggers a different Quick Route path.
2. **Framework conventions:** What test framework (Jest, Vitest, Playwright, pytest)? Conventions differ — `describe/it` nesting, fixture usage, assertion style — and the Verification commands are per-framework.
3. **PR review or batch audit?** A PR review runs and scores only the changed files. A batch audit scans the entire suite for systemic patterns.
4. **Existing quality standards:** Does the team have documented test conventions? Check for `.eslintrc` test rules, `CONTRIBUTING.md` test guidelines, or a test style guide.
5. **Known pain points:** Recurring flaky tests, slow suites, unclear failures? These prioritize which smells to focus on first.

---

## Core Principles

1. **Test code is production code.** Apply the same quality standards: readability, maintainability, single responsibility. Test code that is hard to read is hard to trust.

2. **Review what is asserted, not just what is executed.** Coverage proves a line ran; it says nothing about whether a wrong value would be caught. A 95%-coverage suite of `toBeTruthy` assertions catches almost nothing. Mutation score (see **Verification**) measures the thing coverage can't.

3. **Testability review prevents test debt.** Reviewing application code for testability catches design problems before they force awkward test workarounds. If code is hard to test, it is usually hard to maintain.

4. **Codify patterns, not just knowledge.** Turn recurring review feedback into lint rules, custom ESLint plugins, or shared fixtures. Reviews that repeat the same feedback indicate missing automation.

5. **Smells are symptoms, not verdicts.** A test smell indicates a potential problem; context decides whether it is actually harmful. A long test for a complex workflow may be appropriate. A mock-heavy test for a boundary may be correct.

6. **Actionable feedback only.** Every review comment must include what is wrong, why it matters, and how to fix it. "This test is bad" is not actionable. "This test uses sleep-based waiting which causes flakiness — replace with an explicit wait condition" is.

---

## Test Smell Buckets

Six dimensions. Each smell is categorized by the dimension it affects and links to a specific review action. Full SMELL/FIX code for every catalogued smell lives in `references/smell-examples.md` — keep the inline pointers prominent; the before/after code is the load-bearing part.

### Readability Smells

Problems that make tests hard to understand at a glance.

#### Obscure Setup

**What it looks like:** 30+ lines of object construction with irrelevant fields drowning the test intent. The reader cannot tell which fields matter for the assertion.

**Fix:** Extract to factories. Only test-relevant data should appear in the test body: `buildOrder({ items: [buildItem({ weight: 2.5, quantity: 2 })] })` instead of constructing full user/product/order objects inline.

**Review action:** Request factory extraction.

#### Mystery Guest

**What it looks like:** `loadFixture('report.json')` — the test depends on external data the reader cannot see. They must open another file to understand the assertion.

**Fix:** Inline the test-relevant data or use descriptively named fixtures. The reader should understand the test without opening other files.

**Review action:** Request inline data or descriptive fixture names.

#### Duplicate Assertions

**What it looks like:** Multiple tests assert the same behavior with varying specificity (`toBe('Alice')`, `toHaveProperty('name')`, `toBeDefined()`). Three tests, one behavior.

**Review action:** Request consolidation. Keep the most specific assertion. Redundant tests increase maintenance cost without increasing confidence.

---

### Reliability Smells

Problems that cause tests to fail intermittently or in unexpected environments.

#### Sleep-Based Waiting

**What it looks like:** `setTimeout`, `sleep()`, `waitForTimeout()` used for synchronization. See `references/smell-examples.md` for the SMELL/FIX pair (replace `waitForTimeout` with an explicit `toBeVisible` wait).

**Review action:** Reject. Sleep-based waiting is never acceptable. Require explicit wait conditions.

#### Order Dependency

**What it looks like:** Tests pass when run together but fail in isolation or different order. See `references/smell-examples.md` for the SMELL/FIX pair (each test creating its own preconditions).

**Review action:** Request data isolation. Each test must create its own preconditions.

#### External Service Coupling

**What it looks like:** Tests call real external APIs (payment gateways, email providers, third-party services). See `references/smell-examples.md` for the SMELL/FIX pair (mocking the service boundary).

**Review action:** Request mock or fake at the service boundary. External calls belong in integration/contract tests, not unit tests.

---

### Diagnostic Smells

Problems that make test failures hard to understand and debug.

#### Weak Assertion Messages

**What it looks like:** Assertion fails with no context about what was expected or why. See `references/smell-examples.md` for the SMELL/FIX pair (replacing `toBe(true)` with specific assertions like `expect(result.errors).toEqual([])` that surface the offending value).

**Review action:** Request stronger assertions with diagnostic value. The failure message should explain the problem without reading the test source.

#### Multiple Failure Causes Per Test

**What it looks like:** A single test covers multiple independent behaviors. When it fails, you do not know which behavior broke. See `references/smell-examples.md` for the SMELL/FIX pair (splitting a lifecycle test into one-behavior-per-test).

From ExelAlfanso/puppets, MIT.

More from ExelAlfanso/puppets