Test Review

Autotest review (TypeScript + Playwright)

Check just-written or modified test code against best practices and produce a prioritized list of findings with fixes. Rule source — the official Playwright documentation (best-practices, locators, test-assertions) and TypeScript + the specific project's conventions.

> Source of truth for the project — its root CLAUDE.md (if present) and the style of neighboring code. This skill is the review phase: it complements project rules, it does not replace them. Expanded ❌ before → ✅ after examples and source links for each rule — in [references/rules-catalog.md](references/rules-catalog.md).

---

When to apply and operating mode

• Default mode — diagnostics. Read the code, run static analysis, produce a report. Do not edit files until the user explicitly asks to "apply / fix". Then — iteratively, one change at a time with a run in between (no big-bang rewrite of a working test).
• Scope — only new/changed code, not the whole suite. Default — uncommitted changes (git status + git diff). If the user specified a file/directory — review those.
• Any test type: UI E2E, API, UI+API, visual regression, mobile, mocks, plus Page Objects, fixtures, constants.
• Do not drift into autonomous actions beyond the review (browser reproduction, running the whole suite, edits) without confirmation — the default task is "read and assess".

Evidence discipline (no hallucinations)

• Every finding — from an actually read line (file:line) or from observed output of typecheck / lint / a run. Do not invent violations "by analogy" and do not reference lines you have not seen.
• A rule is checkable by a tool (tsc, ESLint, a run) → run the tool first, then report its output, not "probably present".
• Not sure it is a defect rather than a deliberate project decision → mark it "questionable", do not assert. Cross-check against the project's CLAUDE.md and neighboring code: some "anti-patterns" may be intentional (legacy helpers, non-standard markup, deliberate rule exceptions). File names/endpoints/selectors from memory and past context are background — re-verify against live code.
• Found no violations in a category — say so: "clean", do not invent a finding for volume.

Process

1. Scope. Determine the files under review: git status --short + git diff --name-only (include untracked), or the paths provided. For each spec, find the related Page Objects, constants, fixtures.
2. Context. Read the changed files + related POM/constants/fixtures. Read a neighboring spec in the same directory as a style reference. Cross-check the directory/suite rules (smoke / regress / api etc.) against the project's CLAUDE.md, if present.
3. Static analysis (mandatory — cheap and evidence-based):

• Typecheck: tsc --noEmit (or the project's typecheck script from package.json). Any type error in new code = 🔴 Blocker.
• ESLint: find the project config and read which rules are actually enabled (especially from eslint-plugin-playwright) — do not assume from memory. Lint output is the source of truth.
• What lint actually catches — a two-level check. (1) No eslint-plugin-playwright at all → floating promises, manual asserts and networkidle are invisible; suggest enabling recommended. (2) Recommended is on → missing-playwright-await, prefer-web-first-assertions, no-networkidle are already errors and get caught, but no-wait-for-timeout, no-force-option and expect-expect are only warn there (verified against v2.10.5) — without --max-warnings 0 these warnings never fail CI. Suggest raising them to error. @typescript-eslint/no-floating-promises needs type-aware linting — rarely enabled. Whatever lint still misses — verify manually (A/C/H).
• If needed — a formatting check (Prettier), if configured in the project.

1. Checklist. Go through categories A–J below + K (your project's rules). For each violation — severity + file:line + fix. Deeper per rule — [references/rules-catalog.md](references/rules-catalog.md).
2. Stability verification (only if the user asks to confirm the test works and a sandbox is available): run only this test in the project's native parallelism (NOT --workers=1):

``bash npx playwright test --grep "" --project="" --retries=0 --repeat-each=5 `` Pass-on-retry or a floating result = flake = 🟠 Major; fix the cause (races/hydration/waits), do not hide it behind retries.

1. Report — in the format from the "Report format" section.

Severity

| Label | Meaning | Typical examples | |---|---|---| | 🔴 Blocker | Test is broken, non-deterministic, or masks a bug. Do not merge. | Typecheck error; missing await (floating promise); waitForTimeout/in-page setTimeout pause; pass only on retry; { force: true } / dispatchEvent / direct setter bypassing the real UI; test without asserts; test.only; conditional expect that may never execute. | | 🟠 Major | Brittleness or flake on content/environment change; violation of a key project rule. | CSS/XPath chains instead of role/label; instant count()/isVisible()/allTextContents() as a gate; exact prices/texts/dates instead of regex; project convention violation (importing base @playwright/test where the project requires a custom fixture; a mandatory project check skipped — e.g. the network error monitor); waitForLoadState('networkidle'); a test depends on another test's state. | | 🟡 Minor | Style/readability/maintainability; no impact on stability. | No test.step for business steps; inline comments instead of self-documentation; .nth() where .filter() fits; a working but non-priority locator; unused import/constant. | | ⚪ Nit | Cosmetics. | Naming, import order, formatting (if not caught by Prettier). |

---

Review checklist

Each item — what to look for; the violation's severity in parentheses. Expanded examples and proofs — in the catalog.

A. Determinism, waits, asynchrony

• [ ] No page.waitForTimeout(ms) and no setTimeout/sleep inside page.evaluate (grep for both). Wait for state, not time. (🔴)
• [ ] No floating promises: every expect, test.step, action (click/fill/goto), waitFor* — under await/return/void. A missing await = silent flake. (🔴)
• [ ] Network — the promise → action → await pattern: const p = page.waitForResponse(...); await click(); await p. Declaring after the action = race. (🔴)
• [ ] No waitForLoadState('networkidle'). Navigation — goto(url, { waitUntil: 'domcontentloaded' }), without a duplicate waitForLoadState right after. (🟠)
• [ ] Derived/compound checks (several related conditions, measuring a collection right after it appears) — in await expect(async () => {...}).toPass({ timeout }), not a chain of awaits. (🟠)
• [ ] SSR hydration accounted for: SSR frameworks (Nuxt/Next etc.) can remount content after hydration → an instant count()/allTextContents() right after appearance hits the emptiness window. Measure via toPass. (🟠)

B. Locators

• [ ] Priority: getByRole({ name }) → getByText → getByLabel → getByPlaceholder → getByAltText → getByTitle → getByTestId → CSS (last resort) → XPath (almost never). (🟠 for CSS/XPath without a reason)
• [ ] No brittle CSS chains tied to DOM structure (div > div > span, .episode-actions-later). They break on rebranding. (🟠)
• [ ] Strict mode: the locator resolves to a single element; narrow via { name } / .filter({ hasText }) / .filter({ has }), not .nth(). .nth() — only with justification. (🟡)
• [ ] Floating elements (dropdowns, toasts, modals, portal content, iframes) are located globally from page, not from a section. (🟠)
• [ ] A long getByText('whole sentence') is not used as an anchor — brittle to copy edits; take a stable fragment/role. (🟡)

C. Assertions

• [ ] Only web-first (auto-retrying): toBeVisible/toHaveText/toHaveCount/toHaveValue/toBeChecked/toHaveAttribute/toHaveURL. No expect(await loc.isVisible()).toBe(true) and expect(await loc.count()).toBe(n) — they do not retry. (🔴/🟠)
• [ ] Every test verifies something (no test that only clicks without an expect). (🔴)
• [ ] A block of independent checks for one section — via expect.soft, to collect all failures at once. (🟡)
• [ ] No asserts on exact prices/numbers/dates/dynamic content — regex or a range. (🟠)
• [ ] A known open bug — test.fail() (with the single bug assert), not test.fixme(); the working behavior — as a separate regular test. (🟡)

D. Isolation and independence

• [ ] Tests are independent: state is NOT passed between tests. let x at describe level, reinitialized in beforeEach, is a common valid pattern; the violation is a test reading another test's result. Must pass in isolation and in any order. (🔴 if isolation is broken)
• [ ] describe.configure({ mode: 'serial' }) — only for a real dependency, not "just in case". (🟠)
• [ ] Setup/teardown — in beforeEach/fixtures, no copy-paste; created entities (API) are deleted. (🟠)
• [ ] The test does not depend on external sites and third-party widgets — test only what you control; for the external part — mock/verify the request was made. (🟠)

E. TypeScript and lint

• [ ] Typecheck green for new code (tsc --noEmit, strict). (🔴)
• [ ] any in POM / fixtures / utils — undesirable, type it (Locator/Page/Route/APIResponse). Cross-check the project config: any may be deliberately allowed in specs (e.g. for mock data) — do not flag it there. @ts-ignore — only with a reason/ticket. (🟠 for POM/utils)
• [ ] POM fields — readonly Locator; fixtures typed (base.extend); type the API response body explicitly if asserts rely on it. (🟡)
• [ ] A missing await is often NOT caught by lint (check the config: is no-floating-promises / missing-playwright-await there; valid-expect covers only part) → re-read by eye, see A2. (🔴)
• [ ] Match the module system (ESM vs CJS) and import style (relative vs aliases) against the project's actual code — follow the existing style, do not impose your own. Remove unused imports/variables/constants/POM methods. (🟡)

F. Network and mocks

• [ ] Mocks (page.route) — only for edge cases (5xx, empty response, timeout, offline). The positive happy path — against the real API. (🟠)
• [ ] Contract verification where it is the point of the test: waitForResponse (status + body) / waitForRequest + postDataJSON(). For forms — inspect the payload for [object Object], empty/non-serialized fields, not just "button is enabled". (🟠)
• [ ] Routes are set up before the triggering action; scope — test/fixture, not globally on the suite. (🟠)
• [ ] An external host that may not respond (e.g. an external account portal, a payment gateway): do not verify the transition "in depth" — the oracle is the initiated navigation request + route.abort(), otherwise a pending navigation hangs teardown. (🟠)

G. Structure, readability, hygiene

• [ ] Logical steps are wrapped in test.step('Imperative name', …) (visible in Allure/HTML/trace). return — outside the callback. Do not split into a step per action. (🟡)
• [ ] No inline comments in tests — self-documentation (meaningful names, semantic locators, steps). Context — in the reporter description/annotation (e.g. allure.description), if the project uses them. (🟡)
• [ ] Parameterize same-shaped cases via for...of outside test.describe, not test copies. (🟡)
• [ ] No test.only, commented-out tests, temporary files/drafts, debug console.log/page.pause(). (🔴 for test.only/page.pause, otherwise 🟡)
• [ ] Test/step names are meaningful; ID/tag format (@allure.id:N, TC key etc.) — as in the neighboring tests in the file. (⚪)

H. Bug masking and flake

• [ ] No synthetic bypasses of the real UX: { force: true }, dispatchEvent, direct React/Vue setter, manual scroll instead of auto-actionability — unless justified by a controlled input (e.g. custom display:none inputs — verify in the browser). The fix must catch a regression if the feature breaks, not hide it. (🔴)
• [ ] retries/mode: serial/an increased timeout are not used as a "cure" for flake. Quarantine is acceptable only temporarily, with a ticket link. (🟠)
• [ ] try/catch does not swallow action/assert failures (auto-waiting is built in; .catch() hides the bug). (🟠)
• [ ] A pre-release test (written before the feature ships) fails honestly, not hidden behind skip/a flag. (🟠)

I. Special cases by test type

• [ ] API: verify status AND body; request identifiers — a fresh randomUUID() from the built-in crypto per request (do not pull in the uuid package if the project does not have it); rate limit accounted for; cleanup of created entities. (🟠)
• [ ] iframe: frameLocator; content is located inside the frame. New tab: context.waitForEvent('page'). Download: waitForEvent('download') + filename check. Upload: setInputFiles. Time: page.clock. Geolocation/permissions: grantPermissions/setGeolocation. (🟠 for manual workarounds)
• [ ] visual: toHaveScreenshot with animations:'disabled' and mask on dynamic content; baselines — on the CI platform (a macOS baseline against Linux CI = guaranteed diff). Only if the test case requires a baseline. (🟠)

J. Intent conformance (the oracle actually verifies what is claimed)

• [ ] The test verifies what the name/description promises, not a surrogate. "Form validation" → inspect the real payload, not just "button is enabled". "Load more" → actual loading of more items and verification, not just the click. (🟠)
• [ ] Content binding is structural (presence, non-emptiness, count > 0, regex), so the test survives copy/price changes — especially for regression tests after a fix. (🟠)
• [ ] The oracle matches the environment's limitation: where the UI does not distinguish 404/5xx (one stub for both) — the check is network-level, not "saw the error text". (🟠)

K. Your project's rules (template — fill in for your repository)

> A mature test repository always has conventions no universal checklist will catch. Record them here or in the project's CLAUDE.md — then the review will catch their violations. Typical categories with examples:

• [ ] Custom fixtures: where to import test from the custom fixture (./fixtures/custom-test) instead of @playwright/test, and which mandatory checks it provides (e.g. a network error monitor invoked at the end of the test / in afterEach). (🟠)
• [ ] Directory patterns: how the rules of the smoke / regress / api suites differ — composition vs fixtures for POM, reporter annotations, testMatch/testIgnore, where to add new tests. (🟠)
• [ ] Environments: the test and POM are verified on all target environments, not just one (the DOM on the test environment may differ from prod); known environment quirks are recorded as a list. (🟠)
• [ ] Skipped hygiene: the test does not add permanent skips to standard runs; environment-specific exclusion goes through config (testIgnore/testMatch), runtime test.skip — only for dynamic conditions (feature flag, known bug with a ticket). Run outcome: passed = ok, failed = problem, skipped = requires an explanation. (🟡)
• [ ] Dependencies/config: do not bump the @playwright/test version and do not add dependencies without checking against CI (Docker image, lock file); do not touch playwright.config.ts without necessity. (🔴 if touched without being asked)

---

Report format

## Review: 

**Static analysis:** typecheck ✅/❌ · lint ✅/❌ · (run: N/N pass, --repeat-each=5)

### 🔴 Blocker (N)
1. `path/to/spec.ts:42` — .
   Why: .
   Fix:
   ```ts

…

## Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

- **Author:** [akovalion](https://github.com/akovalion)
- **Source:** [akovalion/paranoid-qa](https://github.com/akovalion/paranoid-qa)
- **License:** MIT

Install and usage instructions live in the source repository linked above.