# Review Test Quality

> Audit AHK test suite for drifted copies, dead tests, and design issues

- Skill: `majiayu000/review-test-quality-2` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx skillmds add majiayu000/review-test-quality-2`
- Raw SKILL.md: https://api.skillmd.com/api/skills/majiayu000/review-test-quality-2/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- Author: majiayu000 (https://skillmd.com/u/majiayu000)
- Updated: 2026-09-09
- Page: https://skillmd.com/skills/majiayu000/review-test-quality-2

---

Enter planning mode. Audit the AHK test suite for quality issues. Use maximum parallelism — spawn explore agents for independent test files.

**Scope**: AHK tests only — unit tests, GUI tests, live tests, lifecycle tests in `tests/`. NOT static analysis checks, NOT query tools, NOT test coverage (this is about quality of existing tests, not finding gaps).

## Priority Issue — Production Code Copies

This is the highest-priority defect. Past reviews have found tests that copy-paste production logic into the test file instead of `#Include`-ing the production file. These tests always pass because they test the copy, not production. When production changes, the copy doesn't update — the test is useless.

**Detection pattern**: A test file that defines a function with the same name as a production function, but does NOT `#Include` the production file containing it. The test's "function" is a frozen copy.

For each test file, check:
1. What production files does it `#Include`?
2. Does it define any functions that shadow production functions? (Compare against `query_function_visibility.ps1` to find the real definition)
3. Is the test calling real production code, or its own copy?

A test that defines **mocks** (GUI rendering, IPC sends, DWM calls) is fine — that's intentional shadowing per `testing.md`. The problem is when **business logic** is copied instead of included.

## Other Quality Issues

### Dead tests
Tests that can never fail regardless of production behavior:
- Assertions that test hardcoded values against themselves
- Tests that set up state and assert on the setup, not on production function output
- Tests where the "expected" value is computed by the same logic being tested

### Duplicative tests
Multiple tests verifying the exact same behavior:
- Same function called with same inputs and same assertions, in different test files
- A unit test and a live test that test identical logic (the unit test is sufficient for pure logic; the live test should test integration aspects the unit test can't)

### Broken assertions
Tests that appear to assert but don't actually fail on wrong values:
- Missing `TestErrors++` after a failed comparison
- Conditions that log `FAIL` but don't increment the error counter
- Assertions comparing wrong variables (e.g., comparing input to input instead of input to output)

### Stale tests
Tests for features or code paths that no longer exist:
- Tests for removed functions (function deleted from production, test still "passes" because it tests its own copy or mock)
- Tests for old behavior that was intentionally changed

## Constraints

The test suite is designed for **automated agentic execution** — no user interaction. Any suggested modifications must follow existing patterns from `test_utils.ahk` and respect the parallel, timing-critical design. See `testing.md` for the full pattern guide.

## Validation

After explore agents report back, **validate every finding yourself**. Test quality issues are subtle — what looks like a copy may actually be an intentional mock, and what looks dead may test an edge case.

For each candidate:

1. **Cite evidence**: "I verified by reading `test_file.ahk` lines X–Y and `production_file.ahk` lines A–B" — show both sides.
2. **Trace the test path**: Does the test call the real production function, or a local copy? Use `query_function_visibility.ps1` to confirm where the function is defined.
3. **Counter-argument**: "What would make this test valuable despite appearances?" — Is it testing a subtle edge case? Is the "copy" actually an intentional mock? Would removing it lose coverage of an important path?
4. **Observed vs inferred**: Did you confirm the test calls a copy (by reading both files), or infer it from the absence of an `#Include`?

## Plan Format

**Section 1 — Production code copies** (highest priority):

| Test File | Lines | Function Copied | Production Source | Fix |
|-----------|-------|----------------|------------------|-----|
| `test_foo.ahk` | 10–25 | `CalculateScore()` | `src/shared/scoring.ahk:42` | Replace copy with `#Include`, add mock for GUI dependency |

**Section 2 — Dead/broken tests**:

| Test File | Lines | Issue | Evidence | Fix |
|-----------|-------|-------|----------|-----|
| `test_bar.ahk` | 88–95 | Asserts setup value, not production output | Expected value is hardcoded `3`, same as setup | Rewrite to call production function and assert result |

**Section 3 — Duplicative tests**:

| Test A | Test B | Overlap | Recommendation |
|--------|--------|---------|---------------|
| `test_unit_x.ahk:30` | `test_live_y.ahk:55` | Both test `ParseConfig()` with same inputs | Keep unit test, refocus live test on integration behavior |

**Section 4 — Stale tests** (testing removed features):

| Test File | Lines | What It Tests | Status | Fix |
|-----------|-------|--------------|--------|-----|
| `test_old.ahk` | 1–50 | `LegacyFormat()` — removed in v0.7 | Always passes (tests own copy) | Delete |

Order by impact: production copies first (actively hiding bugs), then dead tests, then duplicative, then stale.

Ignore any existing plans — create a fresh one.