/e2e-test-review

name: e2e-test-review
description: Review Cypress E2E spec files for Metabase conventions, common gotchas, and flakiness/performance issues. Use when reviewing pull requests or diffs containing Cypress spec files in e2e/test/scenarios/.
allowed-tools: Read, Grep, Bash, Glob

E2E Test Review Skill

@./../_shared/cypress-conventions.md

Review mode detection

Before starting, determine which mode to use:

1. **PR review mode** — if `mcp__github__create_pending_pull_request_review` is available, post issues as one cohesive pending review. 2. **Local review mode** — if not, output a numbered list in the conversation.

Review process

1. Detect mode. 2. Read the changed spec end-to-end first to understand intent. Don't review line-by-line cold. 3. **(Conditional)** If after reading the spec a specific assertion or selector is **ambiguous** — you can't tell if it's the right anchor, or whether the test actually verifies what its title claims — briefly grep the related component for the test ID / role / text. **Don't read components by default.** Only do this when there's a real signal of confusion in the spec. 4. Scan against the checklist + pattern table below. 5. Number all issues sequentially. Skip nits — only flag what's worth fixing. 6. **Always** finish with the honest e2e-vs-unit breakdown (see the section of that name below). It's part of every report, not an optional add-on.

When to hand off instead of review

If the spec references an issue (`metabase#NNNNN`) and the user wants to **fix** flakiness or assess whether the test still reproduces the original bug, that's outside the review skill's scope. Point them at the dedicated flake-fixing workflow, which knows to fetch the issue and the resolving PR's diff. The review skill stays focused on "is this test well-written and conformant" — it doesn't fetch external issue context by default.

Review checklist

The checklist mirrors the order of the conventions file. Items marked **(lint)** are also caught by ESLint — flag only when you see them slip through (helper-wrapped, lint-disabled, etc.).

File and naming

• [ ] Spec lives in `e2e/test/scenarios/<area>/`
• [ ] Extension is `.cy.spec.ts` (preferred) or `.cy.spec.js` — don't flag `.js` on existing files
• [ ] `describe` block names the area when relevant: `"area > sub-area > feature (#issue-number)"`
• [ ] No leftover `.only` or `.skip` (pre-commit hook should catch, but flag it if it slips through)

Helpers and constants

• [ ] Helpers accessed via `const { H } = cy;` — no direct imports from `e2e/support/helpers` **(lint)**
• [ ] Sample DB schema imported from `cypress_sample_database`
• [ ] Instance data IDs imported from `cypress_sample_instance_data`
• [ ] **No hardcoded numeric IDs anywhere** — including for entities the test creates itself. Capture from the create response or alias the intercept.
• [ ] Existing navigation helpers used (`H.openOrdersTable`, `H.visitDashboard(id)`, etc.) instead of raw `cy.visit()` chains

Selectors

• [ ] Prefers a11y queries (`findByRole`, `findByLabelText`) over `findByText`
• [ ] `findByText` only when an a11y query doesn't fit
• [ ] `findByTestId` for `data-testid` attributes (never raw `cy.get("[data-testid='...']")`)
• [ ] No CSS class names — especially generated ones from styled-components/Mantine (`.css-1abc2d`)
• [ ] No ad-hoc CSS attribute selectors in specs or new helpers (the visualization helpers in `e2e/support/helpers/e2e-visual-tests-helpers.js` are the **only** intentional exception — see "What NOT to flag")
• [ ] No XPath
• [ ] **Positional selectors** (`.eq()`, `.first()`, `.last()`, `:nth-child`) only when the order is the assertion itself, or guarded by a length assertion immediately before. **(lint catches `.last()` and `.eq(<negative)`; the rest is on the reviewer)**
• [ ] **Text selectors are scoped** — top-level `cy.findByText(...)` / `cy.contains(...)` in `it`/`before`/`beforeEach` is forbidden. Must be scoped via `cy.contains(selector, text)`, `cy.someQuery().findByText(...)`, or `someQuery().within(...)`. **(lint catches the top-level case ONLY — it does NOT catch helper-wrapped queries; manually scan helper bodies)**

Setup and isolation

• [ ] State setup uses `cy.request` / API helpers, not the UI
• [ ] `H.restore()` and sign-in are in `beforeEach`, not `before`
• [ ] Each `it()` is independently runnable — no `it()` depends on prior `it()` state
• [ ] When both appear, `H.restore()` precedes `H.resetTestTable()` **(lint)**

Waits and timing

• [ ] No numeric `cy.wait(ms)` — even small ones
• [ ] `cy.intercept()` defined BEFORE the action that triggers the request
• [ ] No `setTimeout`, `Cypress.Promise.delay`, or other manual sleeps
• [ ] No long custom timeouts (e.g. `{ timeout: 30000 }`) papering over a race
• [ ] DOM readiness uses `.should("be.visible")`, **not** `.should("exist")`. Reserve `exist` for hidden inputs / off-screen / portal-detached cases.

Never assign return values from `cy.*` commands

• [ ] No `const x = cy.someCommand(...)` — `x` is a one-shot chainer, not the resolved value **(lint catches simple cases)**
• [ ] If a query needs a name, it's wrapped in a function (`const foo = () => cy.findByText("Foo")`), **not** assigned to a `const`
• [ ] Resolved values accessed via `.then()` or aliased with `.as()` + `cy.get("@alias")`
• [ ] Aliases only used when there's distance between lookup and use (otherwise just chain)

Assertions

• [ ] Assertions target user-visible state (text, URL, aria) — not DOM structure
• [ ] **Negative assertions are paired with a positive one.** A standalone `should("not.exist")` / `should("not.be.visible")` passes by accident if the page hasn't rendered yet. Anchor on a positive signal first.
• [ ] **Multiple text checks on the same parent are collapsed into a `.should("contain", ...).and("contain", ...).and("not.contain", ...)` chain** rather than three separate `findByText().should(...)` queries. Single retry budg