ai-qa-review — Skillopedia

<objective QA-focused code review that detects test smells, analyzes testability of application code, and identifies coverage gaps. A test that asserts 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 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 jum…

| tr '\\n' ',')\"`) so a full-suite mutation run (slow) only happens in scheduled audits.\n\nSurviving mutants double as a test-generation feedback loop: feed them back to the agent and ask it to write the assertion that kills each one.\n","content_type":"text/markdown; charset=utf-8","language":"markdown","size":2318,"content_sha256":"161d9cad8061d31892ea42ced42da7ecd03110089cc0ba27f10d46f312c9a155"},{"filename":"references/smell-examples.md","content":"# Test Smell Examples (SMELL → FIX)\n\nRunnable before/after code for the test smells catalogued in `SKILL.md`. The smell descriptions, detection cues, and review actions live in the SKILL; this file holds the full code so the SKILL loads lean.\n\n## Reliability Smells\n\n### Sleep-Based Waiting\n\n```typescript\n// SMELL: Slow on fast machines, flaky on slow ones\nit('should show notification after save', async () => {\n await page.click('#save');\n await page.waitForTimeout(3000);\n expect(await page.isVisible('.notification')).toBe(true);\n});\n\n// FIX: Wait for the specific condition\nit('should show notification after save', async () => {\n await page.getByRole('button', { name: 'Save' }).click();\n await expect(page.getByRole('alert')).toBeVisible();\n});\n```\n\n### Order Dependency\n\n```typescript\n// SMELL: Test B depends on Test A's side effects\ndescribe('user management', () => {\n it('A: should create a user', async () => {\n await api.post('/users', { name: 'Alice' });\n });\n it('B: should list users', async () => {\n const users = await api.get('/users');\n expect(users).toContainEqual({ name: 'Alice' }); // Fails if A didn't run first\n });\n});\n\n// FIX: Each test sets up its own state\ndescribe('user management', () => {\n it('should create a user', async () => {\n const response = await api.post('/users', { name: 'Alice' });\n expect(response.status).toBe(201);\n });\n it('should list users including recently created', async () => {\n await api.post('/users', { name: 'Bob' }); // Own setup\n const users = await api.get('/users');\n expect(users).toContainEqual({ name: 'Bob' });\n });\n});\n```\n\n### External Service Coupling\n\n```typescript\n// SMELL: Fails when Stripe is down, rate-limited, or returns different data\nit('should process payment', async () => {\n const result = await stripe.charges.create({ amount: 2000, currency: 'usd' });\n expect(result.status).toBe('succeeded');\n});\n\n// FIX: Mock the boundary\nit('should process payment', async () => {\n const mockStripe = { charges: { create: vi.fn().mockResolvedValue({ status: 'succeeded' }) } };\n const service = new PaymentService(mockStripe);\n const result = await service.charge(2000, 'usd');\n expect(result.status).toBe('succeeded');\n});\n```\n\n## Diagnostic Smells\n\n### Weak Assertion Messages\n\n```typescript\n// SMELL: Failure message: \"Expected false to be true\" -- useless\nit('should validate the form', () => {\n expect(isValid(form)).toBe(true);\n});\n\n// FIX: Use specific assertions that produce clear failure messages\nit('should accept form with valid email and non-empty name', () => {\n const result = validate(form);\n expect(result.isValid).toBe(true);\n expect(result.errors).toEqual([]);\n // Failure: \"Expected errors to equal [] but received [{ field: 'email', message: 'invalid format' }]\"\n});\n```\n\n### Multiple Failure Causes Per Test\n\n```typescript\n// SMELL: If this fails, is it the creation, the update, or the deletion?\nit('should handle user lifecycle', async () => {\n const user = await service.create({ name: 'Alice' });\n expect(user.id).toBeDefined();\n\n await service.update(user.id, { name: 'Bob' });\n const updated = await service.get(user.id);\n expect(updated.name).toBe('Bob');\n\n await service.delete(user.id);\n await expect(service.get(user.id)).rejects.toThrow(NotFoundError);\n});\n\n// FIX: One behavior per test\nit('should create user with generated id', async () => {\n const user = await service.create({ name: 'Alice' });\n expect(user.id).toBeDefined();\n});\n\nit('should update user name', async () => {\n const user = await service.create({ name: 'Alice' });\n await service.update(user.id, { name: 'Bob' });\n expect((await service.get(user.id)).name).toBe('Bob');\n});\n\nit('should delete user so they cannot be retrieved', async () => {\n const user = await service.create({ name: 'Alice' });\n await service.delete(user.id);\n await expect(service.get(user.id)).rejects.toThrow(NotFoundError);\n});\n```\n\n## Design Smells\n\n### Conditional Test Logic\n\n```typescript\n// SMELL: Test logic that can take different paths is itself untested\nit('should handle all user roles', () => {\n for (const role of ['admin', 'user', 'guest']) {\n const result = getPermissions(role);\n if (role === 'admin') {\n expect(result).toContain('delete');\n } else if (role === 'user') {\n expect(result).toContain('read');\n expect(result).not.toContain('delete');\n } else {\n expect(result).toEqual(['read']);\n }\n }\n});\n\n// FIX: Use parameterized tests (test.each / it.each)\nit.each([\n ['admin', ['read', 'write', 'delete']],\n ['user', ['read', 'write']],\n ['guest', ['read']],\n])('role \"%s\" should have permissions %j', (role, expected) => {\n expect(getPermissions(role)).toEqual(expected);\n});\n```\n\n### Giant Fixtures\n\n```typescript\n// SMELL: Every test pays the setup cost for data it doesn't use\nbeforeEach(async () => {\n await createUser(pool, { id: 'u1', role: 'admin' });\n await createUser(pool, { id: 'u2', role: 'user' });\n await createUser(pool, { id: 'u3', role: 'guest' });\n await createProduct(pool, { id: 'p1', stock: 100 });\n await createProduct(pool, { id: 'p2', stock: 0 });\n await createOrder(pool, { id: 'o1', userId: 'u2' });\n await createOrder(pool, { id: 'o2', userId: 'u2' });\n // ... 15 more objects\n});\n\n// FIX: Each test creates only what it needs\nit('should prevent guest from deleting products', async () => {\n const guest = await createUser(pool, { role: 'guest' });\n const product = await createProduct(pool, { stock: 50 });\n await expect(productService.delete(product.id, guest.id)).rejects.toThrow(ForbiddenError);\n});\n```\n\n### Over-Mocking\n\n```typescript\n// SMELL: Mocking the thing you are testing\nit('should format price', () => {\n const mockFormatter = vi.fn().mockReturnValue('$29.99');\n const result = mockFormatter(29.99);\n expect(mockFormatter).toHaveBeenCalledWith(29.99);\n expect(result).toBe('$29.99');\n // This test verifies nothing about the real formatPrice function\n});\n\n// FIX: Only mock external boundaries (network, DB, filesystem, time)\nit('should format price with currency symbol', () => {\n expect(formatPrice(29.99, 'USD')).toBe('$29.99');\n expect(formatPrice(29.99, 'EUR')).toBe('29,99 EUR');\n});\n```\n\n## Coverage Smells\n\n### Happy Path Only\n\n```typescript\n// SMELL: What happens with invalid input? Empty input? Null? Boundary values?\ndescribe('calculateDiscount', () => {\n it('should apply 10% discount', () => {\n expect(calculateDiscount(100, 0.1)).toBe(90);\n });\n it('should apply 20% discount', () => {\n expect(calculateDiscount(200, 0.2)).toBe(160);\n });\n});\n\n// FIX: Add boundary, negative, and error cases\ndescribe('calculateDiscount', () => {\n it('should apply percentage discount', () => {\n expect(calculateDiscount(100, 0.1)).toBe(90);\n });\n it('should handle zero discount', () => {\n expect(calculateDiscount(100, 0)).toBe(100);\n });\n it('should handle 100% discount', () => {\n expect(calculateDiscount(100, 1.0)).toBe(0);\n });\n it('should reject negative discount', () => {\n expect(() => calculateDiscount(100, -0.1)).toThrow('Discount must be between 0 and 1');\n });\n it('should reject discount over 100%', () => {\n expect(() => calculateDiscount(100, 1.5)).toThrow('Discount must be between 0 and 1');\n });\n it('should handle zero price', () => {\n expect(calculateDiscount(0, 0.1)).toBe(0);\n });\n});\n```\n","content_type":"text/markdown; charset=utf-8","language":"markdown","size":7389,"content_sha256":"bbb31ba40f81ac2ce53ff1a533abf91b065cc6344c3e32e49577cc6ec41cffeb"},{"filename":"references/testability-refactors.md","content":"# Testability Refactors\n\nBefore/after code for the testability problems described under \"Testability Analysis\" in `SKILL.md`. The flags and what-to-look-for cues live in the SKILL; the refactor code lives here.\n\n## Dependency Injection\n\nFlag classes that instantiate dependencies directly (`new PostgresDatabase()`, `new StripeClient()` inside methods). Suggest constructor injection so tests can substitute mocks/fakes.\n\n```typescript\n// HARD TO TEST // TESTABLE\nclass OrderService { class OrderService {\n async create(data: OrderInput) { constructor(\n const db = new PostgresDB(); private readonly db: Database,\n const email = new SendGrid(); private readonly email: EmailClient,\n } ) {}\n} }\n```\n\n## Side Effect Isolation\n\nFlag functions that mix pure calculation with I/O (email, logging, analytics). Extract the calculation as a pure function, then call it from the side-effectful orchestrator. The pure half becomes trivially unit-testable.\n\n```typescript\n// HARD TO TEST: calculation and side effects fused\nasync function checkout(cart: Cart, userId: string) {\n let total = 0;\n for (const item of cart.items) total += item.price * item.qty;\n if (cart.coupon) total *= 1 - cart.coupon.rate;\n await db.orders.insert({ userId, total }); // I/O\n await email.send(userId, `You paid ${total}`); // I/O\n return total;\n}\n\n// TESTABLE: pure calculation extracted\nfunction calcTotal(cart: Cart): number {\n const subtotal = cart.items.reduce((s, i) => s + i.price * i.qty, 0);\n return cart.coupon ? subtotal * (1 - cart.coupon.rate) : subtotal;\n}\n\nasync function checkout(cart: Cart, userId: string) {\n const total = calcTotal(cart); // unit-test this directly\n await db.orders.insert({ userId, total });\n await email.send(userId, `You paid ${total}`);\n return total;\n}\n```\n\n`calcTotal` now tests with plain inputs and outputs — no DB, no email, no boundary values hidden behind I/O.\n\n## Pure Function Extraction\n\nLook for validation, transformation, and business rules buried inside request handlers. If logic is inline in `app.post('/api/orders', ...)`, it cannot be unit-tested without spinning up an HTTP server. Extract it as a standalone function.\n\n```typescript\n// HARD TO TEST: rule logic trapped inside the route handler\napp.post('/api/orders', async (req, res) => {\n if (!req.body.items?.length) return res.status(400).json({ error: 'empty' });\n const weight = req.body.items.reduce((w, i) => w + i.weight * i.qty, 0);\n const shipping = weight > 20 ? 15 : weight > 5 ? 8 : 4; // business rule\n res.json({ shipping });\n});\n\n// TESTABLE: rule is a pure function, handler is a thin adapter\nexport function shippingFor(items: Item[]): number {\n const weight = items.reduce((w, i) => w + i.weight * i.qty, 0);\n return weight > 20 ? 15 : weight > 5 ? 8 : 4;\n}\n\napp.post('/api/orders', async (req, res) => {\n if (!req.body.items?.length) return res.status(400).json({ error: 'empty' });\n res.json({ shipping: shippingFor(req.body.items) });\n});\n```\n\n`shippingFor` is now testable with `it.each` across the 5/20 boundaries — no Express, no supertest.\n\n## Interface Segregation\n\nFlag classes that depend on broad interfaces (entire `PrismaClient`) when they only use 2-3 methods. Define a narrow interface with only the methods actually used, making test doubles trivial to implement.\n\n```typescript\n// HARD TO TEST: must mock all of PrismaClient to fake one query\nclass UserLookup {\n constructor(private prisma: PrismaClient) {}\n byEmail(email: string) { return this.prisma.user.findUnique({ where: { email } }); }\n}\n\n// TESTABLE: narrow port, one-line fake\ninterface UserReader {\n findUnique(args: { where: { email: string } }): Promise\u003cUser | null>;\n}\nclass UserLookup {\n constructor(private users: UserReader) {}\n byEmail(email: string) { return this.users.findUnique({ where: { email } }); }\n}\n// test: new UserLookup({ findUnique: async () => ({ id: '1', email }) })\n```\n","content_type":"text/markdown; charset=utf-8","language":"markdown","size":4095,"content_sha256":"531ea6e918ad1c60b43c73c96f07677e97051f5d85afbd22f679d90443847cc5"}],"content_json":{"type":"doc","content":[{"type":"paragraph","content":[{"text":"\u003cobjective> QA-focused code review that detects test smells, analyzes testability of application code, and identifies coverage gaps. A test that asserts ","type":"text"},{"text":"toBe(true)","type":"text","marks":[{"type":"code_inline"}]},{"text":" 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.","type":"text"}]},{"type":"paragraph","content":[{"text":"Before starting:","type":"text","marks":[{"type":"strong"}]},{"text":" Check for ","type":"text"},{"text":".agents/qa-project-context.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" in the project root. It contains test framework conventions, naming patterns, and project-specific quality standards that calibrate review feedback. \u003c/objective>","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Quick Route","type":"text"}]},{"type":"paragraph","content":[{"text":"Three distinct entry paths. Pick the row, then jump to the named section.","type":"text"}]},{"type":"table","attrs":{"layout":null},"content":[{"type":"tr","content":[{"type":"th","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Situation","type":"text"}]}]},{"type":"th","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Path","type":"text"}]}]},{"type":"th","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Jump to","type":"text"}]}]}]},{"type":"tr","content":[{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"PR with changed test files","type":"text"}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Run the changed files, score them, check the diff against the PR checklist","type":"text"}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Verification","type":"text","marks":[{"type":"strong"}]},{"text":" → ","type":"text"},{"text":"PR Review Checklist","type":"text","marks":[{"type":"strong"}]}]}]}]},{"type":"tr","content":[{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Whole suite needs a health pass","type":"text"}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Quantify, sample, find the 3-5 systemic smells, propose lint/mutation gates","type":"text"}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Batch Audit Process","type":"text","marks":[{"type":"strong"}]}]}]}]},{"type":"tr","content":[{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Application code, \"why is this hard to test?\"","type":"text"}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Flag DI / side-effect / pure-function / interface problems with before/after","type":"text"}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Testability Analysis","type":"text","marks":[{"type":"strong"}]}]}]}]}]},{"type":"paragraph","content":[{"text":"All three share the same smell vocabulary (the six buckets below) and the same Verification commands.","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Discovery Questions","type":"text"}]},{"type":"paragraph","content":[{"text":"First, read ","type":"text"},{"text":".agents/qa-project-context.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" if present and skip any question it already answers.","type":"text"}]},{"type":"ordered_list","attrs":{"order":1,"listStyle":"number"},"content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Review scope:","type":"text","marks":[{"type":"strong"}]},{"text":" Reviewing test code for quality, application code for testability, or both? Each triggers a different Quick Route path.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Framework conventions:","type":"text","marks":[{"type":"strong"}]},{"text":" What test framework (Jest, Vitest, Playwright, pytest)? Conventions differ — ","type":"text"},{"text":"describe/it","type":"text","marks":[{"type":"code_inline"}]},{"text":" nesting, fixture usage, assertion style — and the Verification commands are per-framework.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"PR review or batch audit?","type":"text","marks":[{"type":"strong"}]},{"text":" A PR review runs and scores only the changed files. A batch audit scans the entire suite for systemic patterns.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Existing quality standards:","type":"text","marks":[{"type":"strong"}]},{"text":" Does the team have documented test conventions? Check for ","type":"text"},{"text":".eslintrc","type":"text","marks":[{"type":"code_inline"}]},{"text":" test rules, ","type":"text"},{"text":"CONTRIBUTING.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" test guidelines, or a test style guide.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Known pain points:","type":"text","marks":[{"type":"strong"}]},{"text":" Recurring flaky tests, slow suites, unclear failures? These prioritize which smells to focus on first.","type":"text"}]}]}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Core Principles","type":"text"}]},{"type":"ordered_list","attrs":{"order":1,"listStyle":"number"},"content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Test code is production code.","type":"text","marks":[{"type":"strong"}]},{"text":" Apply the same quality standards: readability, maintainability, single responsibility. Test code that is hard to read is hard to trust.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Review what is asserted, not just what is executed.","type":"text","marks":[{"type":"strong"}]},{"text":" Coverage proves a line ran; it says nothing about whether a wrong value would be caught. A 95%-coverage suite of ","type":"text"},{"text":"toBeTruthy","type":"text","marks":[{"type":"code_inline"}]},{"text":" assertions catches almost nothing. Mutation score (see ","type":"text"},{"text":"Verification","type":"text","marks":[{"type":"strong"}]},{"text":") measures the thing coverage can't.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Testability review prevents test debt.","type":"text","marks":[{"type":"strong"}]},{"text":" 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.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Codify patterns, not just knowledge.","type":"text","marks":[{"type":"strong"}]},{"text":" Turn recurring review feedback into lint rules, custom ESLint plugins, or shared fixtures. Reviews that repeat the same feedback indicate missing automation.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Smells are symptoms, not verdicts.","type":"text","marks":[{"type":"strong"}]},{"text":" 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.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Actionable feedback only.","type":"text","marks":[{"type":"strong"}]},{"text":" 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.","type":"text"}]}]}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Test Smell Buckets","type":"text"}]},{"type":"paragraph","content":[{"text":"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 ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" — keep the inline pointers prominent; the before/after code is the load-bearing part.","type":"text"}]},{"type":"heading","attrs":{"level":3},"content":[{"text":"Readability Smells","type":"text"}]},{"type":"paragraph","content":[{"text":"Problems that make tests hard to understand at a glance.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Obscure Setup","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" 30+ lines of object construction with irrelevant fields drowning the test intent. The reader cannot tell which fields matter for the assertion.","type":"text"}]},{"type":"paragraph","content":[{"text":"Fix:","type":"text","marks":[{"type":"strong"}]},{"text":" Extract to factories. Only test-relevant data should appear in the test body: ","type":"text"},{"text":"buildOrder({ items: [buildItem({ weight: 2.5, quantity: 2 })] })","type":"text","marks":[{"type":"code_inline"}]},{"text":" instead of constructing full user/product/order objects inline.","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request factory extraction.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Mystery Guest","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" ","type":"text"},{"text":"loadFixture('report.json')","type":"text","marks":[{"type":"code_inline"}]},{"text":" — the test depends on external data the reader cannot see. They must open another file to understand the assertion.","type":"text"}]},{"type":"paragraph","content":[{"text":"Fix:","type":"text","marks":[{"type":"strong"}]},{"text":" Inline the test-relevant data or use descriptively named fixtures. The reader should understand the test without opening other files.","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request inline data or descriptive fixture names.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Duplicate Assertions","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" Multiple tests assert the same behavior with varying specificity (","type":"text"},{"text":"toBe('Alice')","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"toHaveProperty('name')","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"toBeDefined()","type":"text","marks":[{"type":"code_inline"}]},{"text":"). Three tests, one behavior.","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request consolidation. Keep the most specific assertion. Redundant tests increase maintenance cost without increasing confidence.","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":3},"content":[{"text":"Reliability Smells","type":"text"}]},{"type":"paragraph","content":[{"text":"Problems that cause tests to fail intermittently or in unexpected environments.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Sleep-Based Waiting","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" ","type":"text"},{"text":"setTimeout","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"sleep()","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"waitForTimeout()","type":"text","marks":[{"type":"code_inline"}]},{"text":" used for synchronization. See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (replace ","type":"text"},{"text":"waitForTimeout","type":"text","marks":[{"type":"code_inline"}]},{"text":" with an explicit ","type":"text"},{"text":"toBeVisible","type":"text","marks":[{"type":"code_inline"}]},{"text":" wait).","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Reject. Sleep-based waiting is never acceptable. Require explicit wait conditions.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Order Dependency","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" Tests pass when run together but fail in isolation or different order. See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (each test creating its own preconditions).","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request data isolation. Each test must create its own preconditions.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"External Service Coupling","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" Tests call real external APIs (payment gateways, email providers, third-party services). See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (mocking the service boundary).","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request mock or fake at the service boundary. External calls belong in integration/contract tests, not unit tests.","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":3},"content":[{"text":"Diagnostic Smells","type":"text"}]},{"type":"paragraph","content":[{"text":"Problems that make test failures hard to understand and debug.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Weak Assertion Messages","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" Assertion fails with no context about what was expected or why. See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (replacing ","type":"text"},{"text":"toBe(true)","type":"text","marks":[{"type":"code_inline"}]},{"text":" with specific assertions like ","type":"text"},{"text":"expect(result.errors).toEqual([])","type":"text","marks":[{"type":"code_inline"}]},{"text":" that surface the offending value).","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request stronger assertions with diagnostic value. The failure message should explain the problem without reading the test source.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Multiple Failure Causes Per Test","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" A single test covers multiple independent behaviors. When it fails, you do not know which behavior broke. See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (splitting a lifecycle test into one-behavior-per-test).","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request test splitting. Each test should have one reason to fail.","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":3},"content":[{"text":"Design Smells","type":"text"}]},{"type":"paragraph","content":[{"text":"Problems in test architecture that increase maintenance cost.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Conditional Test Logic","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" ","type":"text"},{"text":"if/else","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"switch","type":"text","marks":[{"type":"code_inline"}]},{"text":", ternaries, or ","type":"text"},{"text":"for","type":"text","marks":[{"type":"code_inline"}]},{"text":" loops inside test bodies. Branching logic in a test is itself untested — you cannot tell which cases actually ran. See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (converting a branching loop into ","type":"text"},{"text":"it.each","type":"text","marks":[{"type":"code_inline"}]},{"text":").","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request parameterized tests (","type":"text"},{"text":"it.each","type":"text","marks":[{"type":"code_inline"}]},{"text":" / ","type":"text"},{"text":"test.each","type":"text","marks":[{"type":"code_inline"}]},{"text":"). Conditional logic in tests hides which cases are actually verified.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Giant Fixtures","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" A ","type":"text"},{"text":"beforeEach","type":"text","marks":[{"type":"code_inline"}]},{"text":" or fixture that sets up 20+ objects for every test, even though each test uses 2-3 of them. See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (replacing a monolithic ","type":"text"},{"text":"beforeEach","type":"text","marks":[{"type":"code_inline"}]},{"text":" with per-test inline setup).","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request inline setup. Move shared setup to factories, not monolithic ","type":"text"},{"text":"beforeEach","type":"text","marks":[{"type":"code_inline"}]},{"text":" blocks.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Over-Mocking","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" Every collaborator is mocked, including simple value objects and pure functions. See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (dropping a mock of the very function under test).","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request removal of unnecessary mocks. Mock boundaries, not internals.","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":3},"content":[{"text":"AI-Generated Test Smells","type":"text"}]},{"type":"paragraph","content":[{"text":"When the test code came from a coding agent (Claude Code, Codex, Cursor, Copilot), the smell taxonomy is the same — but a few signature failures recur often enough to deserve their own pass.","type":"text"}]},{"type":"table","attrs":{"layout":null},"content":[{"type":"tr","content":[{"type":"th","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Smell","type":"text"}]}]},{"type":"th","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Detection","type":"text"}]}]}]},{"type":"tr","content":[{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Hallucinated locator","type":"text","marks":[{"type":"strong"}]}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Run the test against a real page once. If the locator never matches, the LLM invented a ","type":"text"},{"text":"data-testid","type":"text","marks":[{"type":"code_inline"}]},{"text":" that doesn't exist.","type":"text"}]}]}]},{"type":"tr","content":[{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Fabricated import","type":"text","marks":[{"type":"strong"}]}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Static-check every imported symbol — does the file or package actually export it? LLMs invent plausible APIs (","type":"text"},{"text":"@testing-library/something-that-doesnt-exist","type":"text","marks":[{"type":"code_inline"}]},{"text":").","type":"text"}]}]}]},{"type":"tr","content":[{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Generic test data","type":"text","marks":[{"type":"strong"}]}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"example.com","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"[email protected]","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"Lorem ipsum","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"John Doe","type":"text","marks":[{"type":"code_inline"}]},{"text":" — boilerplate the agent generated because it had no project-specific factory. Replace with the project's data factory.","type":"text"}]}]}]},{"type":"tr","content":[{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Closed AI loop","type":"text","marks":[{"type":"strong"}]}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Both implementation ","type":"text"},{"text":"and","type":"text","marks":[{"type":"em"}]},{"text":" tests authored by the same agent in the same session. The tests just describe what the agent produced; they don't constrain it. Pair the agent's tests with at least one human-authored boundary test, or use TDD (test-first) per ","type":"text"},{"text":"shift-left-testing","type":"text","marks":[{"type":"code_inline"}]},{"text":". A low mutation score (see ","type":"text"},{"text":"Verification","type":"text","marks":[{"type":"strong"}]},{"text":") is the objective tell.","type":"text"}]}]}]},{"type":"tr","content":[{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Project-convention drift","type":"text","marks":[{"type":"strong"}]}]}]},{"type":"td","attrs":{"colspan":1,"rowspan":1,"colwidth":null,"alignment":""},"content":[{"type":"paragraph","content":[{"text":"Page Object, fixture, naming, or assertion style different from the rest of the suite. AI-generated code rarely matches local conventions out of the box.","type":"text"}]}]}]}]},{"type":"paragraph","content":[{"text":"For first-time test generation patterns and the Step-7 review checklist, cross-link ","type":"text"},{"text":"ai-test-generation","type":"text","marks":[{"type":"code_inline"}]},{"text":". For AI-system ","type":"text"},{"text":"eval suites","type":"text","marks":[{"type":"em"}]},{"text":" (the equivalent of ESLint for prompts), wire each tool's CLI runner — ","type":"text"},{"text":"promptfoo eval","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"deepeval test run","type":"text","marks":[{"type":"code_inline"}]},{"text":" (Apache 2.0), Ragas ","type":"text"},{"text":"ragas evaluate","type":"text","marks":[{"type":"code_inline"}]},{"text":" / experiments (Apache 2.0) — as quality gates parallel to your test runner.","type":"text"}]},{"type":"blockquote","content":[{"type":"paragraph","content":[{"text":"Promptfoo ownership note:","type":"text","marks":[{"type":"strong"}]},{"text":" Promptfoo was acquired by OpenAI (announced 9 Mar 2026). The core stays MIT-licensed, open source, and model-agnostic; red-team capabilities are being folded into OpenAI Frontier. ","type":"text"},{"text":"promptfoo eval","type":"text","marks":[{"type":"code_inline"}]},{"text":" is still the correct quality-gate command — just expect the vendor to be OpenAI going forward.","type":"text"}]}]},{"type":"heading","attrs":{"level":3},"content":[{"text":"Coverage Smells","type":"text"}]},{"type":"paragraph","content":[{"text":"Problems that leave gaps in what is verified.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Happy Path Only","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" Every test provides valid input and expects success. No error paths tested. See ","type":"text"},{"text":"references/smell-examples.md","type":"text","marks":[{"type":"code_inline"}]},{"text":" for the SMELL/FIX pair (adding zero, max, negative, and boundary cases to a discount calculator).","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request missing scenarios. Use the BOUNDARY framework: Boundary values, Null/empty, Duplicates, Ordering, Range limits.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Missing Boundary Cases","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" Tests for \"normal\" values (5 items) but not for 0, 1, max, or max+1. Use ","type":"text"},{"text":"it.each","type":"text","marks":[{"type":"code_inline"}]},{"text":" to cover boundaries explicitly: empty collection, single item, exact page size, one over, large set.","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" Request boundary tests. Every numeric parameter, string length, and collection size has boundaries to test.","type":"text"}]},{"type":"heading","attrs":{"level":4},"content":[{"text":"Missing Error/Negative Cases","type":"text"}]},{"type":"paragraph","content":[{"text":"What it looks like:","type":"text","marks":[{"type":"strong"}]},{"text":" No tests for what happens when things go wrong — network failures, invalid input, permission denied, concurrent modification.","type":"text"}]},{"type":"paragraph","content":[{"text":"Review action:","type":"text","marks":[{"type":"strong"}]},{"text":" For each happy path test, ask: \"What is the corresponding failure mode?\" Request tests for the failure.","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Testability Analysis","type":"text"}]},{"type":"paragraph","content":[{"text":"When reviewing application code, assess whether it is structured for testability. Each subsection has a hard-to-test vs. testable before/after in ","type":"text"},{"text":"references/testability-refactors.md","type":"text","marks":[{"type":"code_inline"}]},{"text":".","type":"text"}]},{"type":"heading","attrs":{"level":3},"content":[{"text":"Dependency Injection","type":"text"}]},{"type":"paragraph","content":[{"text":"Flag classes that instantiate dependencies directly (","type":"text"},{"text":"new PostgresDatabase()","type":"text","marks":[{"type":"code_inline"}]},{"text":", ","type":"text"},{"text":"new StripeClient()","type":"text","marks":[{"type":"code_inline"}]},{"text":" inside methods). Suggest constructor injection so tests can substitute mocks/fakes. See ","type":"text"},{"text":"references/testability-refactors.md","type":"text","marks":[{"type":"code_inline"}]},{"text":".","type":"text"}]},{"type":"heading","attrs":{"level":3},"content":[{"text":"Side Effect Isolation","type":"text"}]},{"type":"paragraph","content":[{"text":"Flag functions that mix pure calculation with I/O (email, logging, analytics). Extract the calculation as a pure function, then call it from the side-effectful orchestrator. See the ","type":"text"},{"text":"calcTotal","type":"text","marks":[{"type":"code_inline"}]},{"text":" before/after in ","type":"text"},{"text":"references/testability-refactors.md","type":"text","marks":[{"type":"code_inline"}]},{"text":".","type":"text"}]},{"type":"heading","attrs":{"level":3},"content":[{"text":"Pure Function Extraction","type":"text"}]},{"type":"paragraph","content":[{"text":"Look for validation, transformation, and business rules buried inside request handlers. If logic is inline in ","type":"text"},{"text":"app.post('/api/orders', ...)","type":"text","marks":[{"type":"code_inline"}]},{"text":", it cannot be unit-tested without spinning up an HTTP server. Extract it as a standalone function. See the ","type":"text"},{"text":"shippingFor","type":"text","marks":[{"type":"code_inline"}]},{"text":" before/after in ","type":"text"},{"text":"references/testability-refactors.md","type":"text","marks":[{"type":"code_inline"}]},{"text":".","type":"text"}]},{"type":"heading","attrs":{"level":3},"content":[{"text":"Interface Segregation","type":"text"}]},{"type":"paragraph","content":[{"text":"Flag classes that depend on broad interfaces (entire ","type":"text"},{"text":"PrismaClient","type":"text","marks":[{"type":"code_inline"}]},{"text":") when they only use 2-3 methods. Suggest a narrow interface with only the methods actually used, making test doubles trivial to implement. See ","type":"text"},{"text":"references/testability-refactors.md","type":"text","marks":[{"type":"code_inline"}]},{"text":".","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Review Workflow","type":"text"}]},{"type":"heading","attrs":{"level":3},"content":[{"text":"PR Review Checklist","type":"text"}]},{"type":"paragraph","content":[{"text":"For each test file in a PR, check systematically:","type":"text"}]},{"type":"code_block","attrs":{"wrap":false,"language":"markdown"},"content":[{"text":"## Test Quality Review\n\n### Readability\n- [ ] Can I understand what each test verifies in under 10 seconds?\n- [ ] Is setup minimal and test-relevant?\n- [ ] Are test names descriptive: \"should [behavior] when [condition]\"?\n\n### Reliability\n- [ ] No sleep/waitForTimeout/setTimeout for synchronization?\n- [ ] No shared mutable state between tests?\n- [ ] No dependency on test execution order?\n- [ ] No calls to real external services?\n\n### Diagnostic Value\n- [ ] Will failures produce messages that identify the problem?\n- [ ] Does each test have one reason to fail?\n- [ ] Are assertions specific (not toBeTruthy/toBeDefined)?\n\n### Design\n- [ ] No conditional logic (if/else/switch/for) in test bodies?\n- [ ] Fixtures/setup proportional to what each test needs?\n- [ ] Mocking limited to external boundaries?\n- [ ] Parameterized tests (it.each) used for data-driven scenarios?\n\n### AI-Generated (if applicable)\n- [ ] Locators verified against a real page (no hallucinated data-testid)?\n- [ ] Imports resolve (no fabricated APIs)?\n- [ ] Project data factory used, not [email protected] / John Doe?\n\n### Coverage\n- [ ] Happy path AND error/negative paths tested?\n- [ ] Boundary values tested (0, 1, max, max+1)?\n- [ ] Edge cases: empty, null, duplicate, concurrent?","type":"text"}]},{"type":"heading","attrs":{"level":3},"content":[{"text":"Batch Audit Process","type":"text"}]},{"type":"paragraph","content":[{"text":"For a full test suite audit:","type":"text"}]},{"type":"ordered_list","attrs":{"order":1,"listStyle":"number"},"content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Quantify:","type":"text","marks":[{"type":"strong"}]},{"text":" Count tests by type (unit/integration/E2E), framework, and directory.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Sample:","type":"text","marks":[{"type":"strong"}]},{"text":" Review 10-20% of test files, prioritizing the largest and most recently changed.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Pattern:","type":"text","marks":[{"type":"strong"}]},{"text":" Identify the 3-5 most common smells across the sample.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Prioritize:","type":"text","marks":[{"type":"strong"}]},{"text":" Rank by impact: reliability smells > diagnostic smells > design smells > readability smells.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Automate:","type":"text","marks":[{"type":"strong"}]},{"text":" For each common smell, determine if an ESLint rule or a mutation-score gate can catch it automatically.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Report:","type":"text","marks":[{"type":"strong"}]},{"text":" Document findings (one row per file reviewed) with specific examples, suggested fixes, severity (high/medium/low), and estimated effort.","type":"text"}]}]}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Prompt Templates","type":"text"}]},{"type":"paragraph","content":[{"text":"Three prompt patterns for AI-assisted review:","type":"text"}]},{"type":"ordered_list","attrs":{"order":1,"listStyle":"number"},"content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Review test quality:","type":"text","marks":[{"type":"strong"}]},{"text":" \"Check this test file for readability, reliability, diagnostic, design, AI-generated, and coverage smells. For each issue: name the smell, cite the line, explain why it matters, and provide a fix.\"","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Identify coverage gaps:","type":"text","marks":[{"type":"strong"}]},{"text":" \"Given this application code and its existing tests, identify missing scenarios across happy path, error handling, boundaries, edge cases, concurrency, and security. Prioritize as P0/P1/P2.\"","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Testability improvements:","type":"text","marks":[{"type":"strong"}]},{"text":" \"Review this application code for hard-coded dependencies, mixed side effects, extractable pure functions, and overly broad interfaces. Show current vs. refactored code.\"","type":"text"}]}]}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Anti-Patterns","type":"text"}]},{"type":"ordered_list","attrs":{"order":1,"listStyle":"number"},"content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Reviewing test code with production-code standards only.","type":"text","marks":[{"type":"strong"}]},{"text":" Test code has additional quality dimensions (reliability, diagnostics, coverage) that production-code linters do not check. Apply the six smell buckets, not just \"clean code\" principles.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Flagging every smell without context.","type":"text","marks":[{"type":"strong"}]},{"text":" A 50-line test for a complex state machine is not obscure setup — it is necessary complexity. Evaluate smells against the behavior being tested.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Suggesting mocks for everything.","type":"text","marks":[{"type":"strong"}]},{"text":" Over-mocking is itself a smell. Do not recommend mocking pure functions, value objects, or fast in-process collaborators. Mock boundaries: network, database, filesystem, clock.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Focusing on coverage percentage over coverage quality.","type":"text","marks":[{"type":"strong"}]},{"text":" 95% line coverage with only happy-path tests is worse than 75% coverage that includes error paths and boundaries. Review what is asserted, not just what is executed — and back it with mutation score.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Reviewing without running.","type":"text","marks":[{"type":"strong"}]},{"text":" Static analysis misses runtime issues. Run the suite (see ","type":"text"},{"text":"Verification","type":"text","marks":[{"type":"strong"}]},{"text":"), check for flakiness with repeated runs, and check execution time. A passing suite that takes 20 minutes has a performance smell.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"One-time review without follow-up.","type":"text","marks":[{"type":"strong"}]},{"text":" Test quality degrades over time. Establish a recurring review cadence or automated quality gates (lint rule, mutation threshold) that catch regression.","type":"text"}]}]}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Verification","type":"text"}]},{"type":"paragraph","content":[{"text":"Run before you start commenting — these are the runtime checks the skill insists on, made concrete. Scope them to the PR's changed test files in PR review; run the whole suite in a batch audit.","type":"text"}]},{"type":"ordered_list","attrs":{"order":1,"listStyle":"number"},"content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Run the suite once.","type":"text","marks":[{"type":"strong"}]},{"text":" Confirm it is green before reviewing — never review red or skipped tests as if they pass.","type":"text"}]},{"type":"bullet_list","content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Vitest: ","type":"text"},{"text":"npx vitest run \u003cfiles>","type":"text","marks":[{"type":"code_inline"}]},{"text":" · Jest: ","type":"text"},{"text":"npx jest \u003cfiles>","type":"text","marks":[{"type":"code_inline"}]},{"text":" · Playwright: ","type":"text"},{"text":"npx playwright test \u003cfiles>","type":"text","marks":[{"type":"code_inline"}]},{"text":" · pytest: ","type":"text"},{"text":"pytest \u003cfiles>","type":"text","marks":[{"type":"code_inline"}]}]}]}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Repeat to surface flakiness.","type":"text","marks":[{"type":"strong"}]},{"text":" A test that passes once and fails on the next run is the reliability smell, not a fluke.","type":"text"}]},{"type":"bullet_list","content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Vitest: ","type":"text"},{"text":"npx vitest run --retry=0 \u003cfiles>","type":"text","marks":[{"type":"code_inline"}]},{"text":" looped, or ","type":"text"},{"text":"for i in 1 2 3; do npx vitest run \u003cfiles> || break; done","type":"text","marks":[{"type":"code_inline"}]}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Jest: ","type":"text"},{"text":"npx jest \u003cfiles> && npx jest \u003cfiles> && npx jest \u003cfiles>","type":"text","marks":[{"type":"code_inline"}]},{"text":" (or ","type":"text"},{"text":"jest-circus","type":"text","marks":[{"type":"code_inline"}]},{"text":" repeat)","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Playwright: ","type":"text"},{"text":"npx playwright test --repeat-each=3 \u003cfiles>","type":"text","marks":[{"type":"code_inline"}]}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"pytest: ","type":"text"},{"text":"pytest --count=3 \u003cfiles>","type":"text","marks":[{"type":"code_inline"}]},{"text":" (pytest-repeat) or ","type":"text"},{"text":"pytest -p no:randomly","type":"text","marks":[{"type":"code_inline"}]},{"text":" vs random order to catch order dependency","type":"text"}]}]}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Capture per-test timing.","type":"text","marks":[{"type":"strong"}]},{"text":" A 20-minute suite has a performance smell.","type":"text"}]},{"type":"bullet_list","content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Vitest: ","type":"text"},{"text":"--reporter=verbose","type":"text","marks":[{"type":"code_inline"}]},{"text":" (prints per-test duration) · Jest: ","type":"text"},{"text":"--verbose","type":"text","marks":[{"type":"code_inline"}]},{"text":" · Playwright: ","type":"text"},{"text":"--reporter=list","type":"text","marks":[{"type":"code_inline"}]},{"text":" · pytest: ","type":"text"},{"text":"--durations=10","type":"text","marks":[{"type":"code_inline"}]}]}]}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Mutation score — the objective backstop.","type":"text","marks":[{"type":"strong"}]},{"text":" For the qualitative smells you can't eyeball at scale (Closed AI loop, weak assertions), run a mutation runner and read the score. AI-generated tests under ~60% mutation score are not constraining the implementation. Full tooling, thresholds, and config in ","type":"text"},{"text":"references/mutation-testing.md","type":"text","marks":[{"type":"code_inline"}]},{"text":".","type":"text"}]},{"type":"bullet_list","content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"JS/TS: ","type":"text"},{"text":"npx stryker run","type":"text","marks":[{"type":"code_inline"}]},{"text":" · Java: PIT ","type":"text"},{"text":"mvn ... mutationCoverage","type":"text","marks":[{"type":"code_inline"}]},{"text":" · Rust: ","type":"text"},{"text":"cargo mutants","type":"text","marks":[{"type":"code_inline"}]},{"text":" · Python: ","type":"text"},{"text":"mutmut run","type":"text","marks":[{"type":"code_inline"}]}]}]}]}]}]},{"type":"paragraph","content":[{"text":"Output of steps 1-3 plus the mutation score is the evidence behind every \"reliability\"/\"diagnostic\" finding in the report.","type":"text"}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Done When","type":"text"}]},{"type":"bullet_list","content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"The findings artifact exists with one row per reviewed file, each row carrying a severity rating (high/medium/low). (file exists, row count == files reviewed)","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Every smell dimension that applies to the reviewed code is covered: readability, reliability, diagnostic, design, AI-generated, coverage. (six headings present in the report; \"N/A\" allowed, blank not)","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Each high-severity finding has an actionable remediation step (what + why + fix), not just a description.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"Verification ran: the suite executed at least 3x green (no flaky failures) and per-test timing captured; for AI-generated suites, a mutation score is recorded.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"At least one recurring smell was converted to an automated gate — an ESLint rule, a CI check, or a mutation-score threshold committed — or the report explicitly states none was warranted.","type":"text"}]}]}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Reference Files (in ","type":"text"},{"text":"references/","type":"text","marks":[{"type":"code_inline"}]},{"text":")","type":"text"}]},{"type":"bullet_list","content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"smell-examples.md","type":"text","marks":[{"type":"strong"}]},{"text":" — Full SMELL/FIX code for every catalogued smell across reliability, diagnostic, design, and coverage dimensions.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"testability-refactors.md","type":"text","marks":[{"type":"strong"}]},{"text":" — Hard-to-test vs. testable before/after for all four testability subsections: dependency injection, side effect isolation, pure function extraction, interface segregation.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"mutation-testing.md","type":"text","marks":[{"type":"strong"}]},{"text":" — Tooling (Stryker/PIT/cargo-mutants/mutmut), thresholds, Stryker config, and the AI-loop / weak-assertion gate.","type":"text"}]}]}]},{"type":"hr","attrs":{"markup":"---"}},{"type":"heading","attrs":{"level":2},"content":[{"text":"Related Skills","type":"text"}]},{"type":"bullet_list","content":[{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"unit-testing","type":"text","marks":[{"type":"strong"}]},{"text":" — Framework-specific patterns (Jest, Vitest, pytest) that inform what \"good\" looks like for each framework.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"shift-left-testing","type":"text","marks":[{"type":"strong"}]},{"text":" — Pre-commit hooks and IDE integration that catch test smells before review.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"coverage-analysis","type":"text","marks":[{"type":"strong"}]},{"text":" — Interpreting coverage reports to find meaningful gaps, not just percentage targets.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"ai-test-generation","type":"text","marks":[{"type":"strong"}]},{"text":" — Generated tests need review too. Apply these smell checks to AI-generated test code.","type":"text"}]}]},{"type":"list_item","content":[{"type":"paragraph","content":[{"text":"test-reliability","type":"text","marks":[{"type":"strong"}]},{"text":" — Reliability smells (sleep-based waits, order dependency) overlap with flaky test patterns.","type":"text"}]}]}]}]},"metadata":{"date":"2026-11-02","name":"ai-qa-review","author":"@skillopedia","source":{"stars":239,"repo_name":"qa-skills","origin_url":"https://github.com/petrkindlmann/qa-skills/blob/HEAD/skills/ai-qa-review/SKILL.md","repo_owner":"petrkindlmann","body_sha256":"07a40a415272e346b6120ec867054bced297276029e725a793efb80180af7782","cluster_key":"f09f6b4b601994cd53d0dd26a7675fc8bde704b9b170c00ea9a758fb97fe709a","clean_bundle":{"format":"clean-skill-bundle-v1","source":"petrkindlmann/qa-skills/skills/ai-qa-review/SKILL.md","text_bytes":13802,"source_commit":"62434253c2592f27b494609c27f224939c86346d","skills_sh_rank":10872,"attachment_count":3,"text_attachments":3,"attachment_storage":"skill_attachments","binary_attachments":0},"cluster_size":1,"skill_md_path":"skills/ai-qa-review/SKILL.md","import_metadata":{"date":"2026-11-02","author":"@skillopedia","version":"v1","category":"testing-qa","category_label":"Testing"},"exact_dupes_collapsed_into_this":0},"license":"MIT","version":"v1","category":"testing-qa","metadata":{"author":"kindlmann","version":"2.0","category":"ai-qa"},"import_tag":"skills-sh-top20k-2026-11","description":"Review EXISTING test code for quality, smells, and testability issues. Detects test smells across six dimensions — readability, reliability, diagnostic value, design, AI-generated, and coverage — analyzes testability of application code, and backs the qualitative smells with mutation testing. Use when: \"review my tests,\" \"test quality audit,\" \"test smells,\" \"testability analysis,\" \"are these tests any good.\" Not for: generating new tests — use `ai-test-generation`. Not for: testing AI features in your product — use `ai-system-testing`. Related: unit-testing, shift-left-testing, coverage-analysis, ai-test-generation."}},"renderedAt":1793671889648}

Important: agents should read /llm.txt, /llms.txt, or /.well-known/skills.json to discover the public Skillopedia API.