ai-qa-review

Files (1)

• SKILL.md

SKILL.md (first lines)

<objective>
QA-focused code review that detects test smells, analyzes testability of application code, and identifies coverage gaps.

**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>

---

## Discovery Questions

1. **Review scope:** Reviewing test code for quality, application code for testability, or both? Each triggers different analysis patterns.
2. **Framework conventions:** What test framework (Jest, Vitest, Playwright, pytest)? Conventions differ -- `describe/it` nesting, fixture usage, assertion style.
3. **PR review or batch audit?** A PR review focuses on changed files. A batch audit scans the entire test 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:** Are there recurring problems -- 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. **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.

3. **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.

4. **Smells are symptoms, not verdicts.** A test smell indicates a potential problem. Context determines 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.

5. **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 explicit wait condition" is.

---

## Test Smell Buckets

Each smell is categorized by the dimension it affects. Each links to a specific review action.

### 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.

```typescript
// SMELL: Slow on fast machines, flaky on slow ones
it('should show notification after save', async () => {
  await page.click('#save');
  await page.waitForTimeout(3000);
  expect(await page.isVisible('.notification')).toBe(true);
});

// FIX: Wait for the specific condition
it('should show notification after save', async () => {
  await page.getByRole('button', { name: 'Save' }).click();
  await expect(page.getByRole('alert')).toBeVisible();
});
```

**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.

```typescript
// SMELL: Test B depends on Test A's side effects
describe('user management', () => {
  it('A: should create a user', async () => {
    await api.post('/users', { name: 'Alice' });
  });
  it('B: should list users', async () => {
    const users = await api.get('/users');
    expect(users).toContainEqual({ name: 'Alice' }); // Fails if A didn't run first
  });
});

// FIX: Each test sets up its own state
describe('user management', () => {
  it('should create a user', async () => {
    const response = await api.post('/users', { name: 'Alice' });
    expect(response.status).toBe(201);
  });
  it('should list users including recently created', async () => {
    await api.post('/users', { name: 'Bob' }); // Own setup
    const users = await api.get('/users');
    expect(users).toContainEqual({ name: 'Bob' });
  });
});
```

From JaiminVadadoriya/Distributed-Cloud-Storage---Sync-Platform, MIT.

More from JaiminVadadoriya/Distributed-Cloud-Storage---Sync-Platform