# E2e Test Review

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

- Skill: `metabase-metabase/e2e-test-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add metabase-metabase/e2e-test-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/metabase-metabase/e2e-test-review/raw
- Safety review: pending (external: skill-scanner PASS, skillspector PASS)
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: metabase (https://skillmd.com/u/metabase-metabase)
- Updated: 2026-09-10
- Page: https://skillmd.com/skills/metabase-metabase/e2e-test-review

---


# 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 budget, atomic against one DOM snapshot.
- [ ] `expect()` only used inside `cy.then` / `cy.wrap` callbacks
- [ ] `.should("not.exist")` vs `.should("not.be.visible")` used correctly for intent
- [ ] No JS conditionals on cy chains (`.then(el => if (...))`) — `.then()` runs once, `.should()` retries

### `cy.within` (always chained)

- [ ] **Every `cy.within(...)` is chained off a previous selector.** Standalone `cy.within(...)` has no scope and is wrong by construction — flag on sight.
- [ ] **No single-statement `within` callbacks** — if the callback has only one inner command, chain directly off the parent instead. Reserve `within` for two or more commands sharing the scope.
- [ ] **No named parameter on `within` callbacks** — `within((modal) => ...)` is redundant; the subject is inherited automatically by inner commands. If you actually need the jQuery subject, use `.then($el => ...)` instead.
- [ ] `.within()` callback is properly closed; assertions outside the callback don't accidentally rely on the within scope

### Logging and annotation

- [ ] Step annotations use `cy.log("...")`, not `// comments` (visible in command panel, screenshots, videos)
- [ ] `cy.log` lines aren't redundant paraphrases of the next command — they mark phases or non-obvious intent

### Performance

> **Look up actual CI duration before flagging slowness.** `e2e/support/timings.json` holds the latest CI run's per-spec wall time in ms (entries keyed as `../test/scenarios/...`). Read it instead of running the spec — running adds minutes; reading is instant. Compare the spec to its siblings in the same directory to ground the "this is expensive" claim. Per-`it()` granularity is not in this file.

- [ ] **Tiny tests** — multiple `it()` blocks for the same flow with the same `beforeEach` setup are a smell. Each `it()` costs 5–10s of Cypress runner overhead + the `beforeEach` cost; merge into a single flow when possible.
- [ ] **Tiny tests that should be unit tests** — a test that only asserts on element existence/visibility (no flow, no real backend interaction, no cross-screen traversal) belongs in a Jest + RTL unit test, not e2e. Common offenders: token-gated UI checks, "renders the help panel" tests.
- [ ] **Near-duplicate tests** — a new `it()` that shares 80–90% of its setup and steps with a sibling test should extend the existing test, not spawn a clone.
- [ ] **Multiple `cy.visit()` calls** — each is a cold app boot (multi-second). After the first visit, navigate via the UI (click a link, breadcrumb, sidebar item) instead of issuing a second `cy.visit()`.
- [ ] **Redundant with a cheaper-layer test** — when the spec references an issue (e.g. `metabase#12345`) in the test name or `describe`, grep the codebase for the **bare** `#NNNNN` (no `metabase` prefix — backend tests don't always include it). If a Jest spec or backend `_test.clj` already cites the same issue, the e2e test is already redundant; recommend deletion. Recipe:
      ```bash
      rg "#12345" -g '*.spec.{ts,tsx,js,jsx}' -g '!*.cy.spec.*' -g '*_test.clj' -g '*_test.cljc'
      ```
      The `-g '!*.cy.spec.*'` exclusion drops the e2e spec being reviewed (which will obviously match its own reference).

### Cypress framework anti-patterns

- [ ] No `forEach` over cy queries — use `cy.each()` if iteration is needed
- [ ] No mixing native Promises / `async`-`await` with cy chains
- [ ] No reliance on stale element references after re-render — re-query instead
- [ ] No `cy.contains("text")` (single-arg, top-level) — same scoping rule as `findByText`

## Pattern matching table

Quick scan for common issues. **(lint)** marks patterns ESLint already catches in the simple form — flag when you see them slip through.

| Pattern                                                      | Issue                                                                                                                          |
| ------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------ |
| `cy.wait(2000)`                                              | Numeric wait — use intercept alias or `.should("be.visible")`                                                                  |
| `cy.get(".css-1abc2d")`                                      | Generated CSS class — use a11y query / `findByText` / `findByTestId`                                                           |
| `cy.get("[data-testid='foo']")`                              | Always use `cy.findByTestId('foo')` instead                                                                                    |
| `cy.get("path[fill='#abc']")` in a spec                      | Ad-hoc chart selector — use `e2e-visual-tests-helpers` or add a new helper there                                               |
| `cy.get("li:nth-child(3)")`                                  | Positional in a CSS selector — anchor on text or role                                                                          |
| `.last()` / `.eq(-1)` without a prior length assertion       | Risk of off-by-one when collection size shifts **(lint)**                                                                      |
| `cy.visit("/dashboard/10")`                                  | Hardcoded numeric ID — capture from create response or import from `cypress_sample_instance_data`                              |
| `import { restore } from "e2e/support/helpers"`              | Direct helper import — use `const { H } = cy;` **(lint)**                                                                      |
| `cy.findByText("Save")` at top of `it()` / `beforeEach`      | Unscoped text selector — wrap in `cy.contains(selector, text)` or chain off a scoping query **(lint, BUT misses helper-wrapped)** |
| `function clickSave() { cy.findByText("Save").click() }`     | Helper-wrapped unscoped text — lint blind spot, scan helpers manually                                                          |
| Standalone `cy.within(() => ...)`                            | `cy.within` must be chained off an existing selector — wrong by construction                                                   |
| `cy.intercept` placed AFTER the triggering call              | Intercept must precede the action                                                                                              |
| `cy.get(...).then(el => { if (...) })`                       | JS conditional on cy chain — use `.should()` for retry semantics                                                               |
| `await cy.something()`                                       | Cypress chains aren't real Promises                                                                                            |
| `els.forEach(el => cy....)`                                  | Use `cy.each()` instead                                                                                                        |
| `{ timeout: 30000 }` on a single command                     | Likely papering over a race — find the root cause                                                                              |
| `const button = cy.findByRole(...)`                          | Assigning `cy.*` return value — `button` is a one-shot chainer, not a DOM element **(lint catches simple cases)**              |
| `const foo = () => cy.findByText("Foo")`                     | This is **fine** — the function form re-enqueues the query on each call                                                        |
| `.should("not.exist")` as the only assertion in a step       | Negative-only — passes by accident if page hasn't rendered. Anchor on a positive assertion first                               |
| `H.resetTestTable()` before `H.restore()`                    | `restore` must come first **(lint)**                                                                                           |
| Multiple `cy.visit()` in one `it()` block                    | Each is a cold boot — navigate via the UI between screens                                                                      |
| `it.only(` / `describe.only(`                                | Pre-commit hook should block — flag as slip                                                                                    |
| New `it()` block with same setup + 80–90% same steps as sibling | Likely a near-duplicate — extend the existing test instead                                                                  |
| `it("...", () => { cy.findBy*().should("be.visible") })` only | Static-UI-only test — strong signal it should be a Jest unit test, not e2e                                                    |
| `cy.log("Visit dashboard"); H.visitDashboard(id);`           | Redundant log — paraphrases the next command                                                                                   |
| `// Visit dashboard` followed by `H.visitDashboard(id);`     | Use `cy.log("...")` instead — visible in screenshots/videos                                                                    |

## What NOT to flag

- Don't flag the `.cy.spec.js` extension on existing files — both `.js` and `.ts` are acceptable; `.ts` is preferred only for **new** specs.
- Don't flag CSS attribute selectors (`path[fill='...']`, `[stroke-dasharray='...']`, `text[stroke-width='3']`, etc.) inside `e2e/support/helpers/e2e-visual-tests-helpers.js`. ECharts renders SVG with no `data-testid` and minimal a11y, and that file is the intentional exception. **Do** flag the same patterns when they appear in spec files or in new helpers — those should funnel through the existing helpers or extend that file.
- Don't flag `.first()` / `.eq(N)` / `:nth-child(N)` when the order is genuinely the assertion (e.g. testing sort order) or when a `.should("have.length", n)` immediately precedes them.
- Don't flag `cy.findByText(...)` / `cy.contains(...)` inside a helper body **if** the helper is documented or clearly intended to be called from inside an outer `within(...)` scope at the call site (the inherited within scope makes it safe at runtime). When in doubt, ask the author.
- Don't flag `.only` if you're doing a local-dev review (the user is intentionally focused). At PR / commit-time review, do flag it.
- Don't post "looks good" or congratulatory comments. Only post issues.
- Don't post comments for formatting issues that the linter handles (Prettier/ESLint).
- Don't flag stylistic preferences that don't materially affect flakiness, speed, or correctness.

## Feedback format

Number every issue sequentially starting from `Issue 1`. Format: `**Issue N: [Brief title]**`.

### Local review mode

```markdown
## Issues

**Issue 1: [Brief title]**
File:Line — succinct description
Suggested fix

**Issue 2: [Brief title]**
...
```

### PR review mode

Use the pending review workflow:

1. `mcp__github__create_pending_pull_request_review` — start a draft review.
2. `mcp__github__get_pull_request_diff` — get file paths and line numbers.
3. Identify ALL issues, number them sequentially.
4. `mcp__github__add_pull_request_review_comment_to_pending_review` — post each issue as a separate comment, in **parallel** within a single response.
5. `mcp__github__submit_pending_pull_request_review` with event `"COMMENT"` (not `REQUEST_CHANGES`), no body.

Each comment body starts with `**Issue N: [Brief title]**`.

## Honest e2e-vs-unit breakdown (always include)

Every report ends with this section. The numbered issues above are about whether the test is *well-written*; this section is about whether it should be an **e2e test at all**. They're orthogonal — a flawless spec can still be paying e2e prices for component-test value, and that's worth saying out loud.

The bar is simple: **an `it()` earns its place in e2e only if it exercises a layer you cannot fake without gutting the test's value.** In this codebase that means one or more of:

- a **real backend query against seeded data** (the assertion depends on the server actually filtering/sorting/bucketing rows, not on a mocked response);
- **real rendering you then interact with** — clicking a charted ECharts SVG element at computed coordinates, drag/hit-testing, drill-through;
- **routing across screens** — navigation, URL param round-trips that the backend then honors, browser back/forward;
- **cross-screen traversal** — list → detail → back, where the value is the seam between screens.

Conversely, a test belongs in **Jest + RTL** (the cheaper layer this repo already uses for component/unit coverage) when its real subject is frontend-only:

- **pure frontend logic** — a bucketing/formatting/derivation decision (e.g. "single-day range → bucket by hour") that has one right answer independent of the backend;
- **render assertions** — "the page mounts N titled cards and an SVG exists." A mocked dataset response + RTL verifies the titles; that an ECharts SVG renders is library behavior, not your logic;
- **state/wiring** — tab → `aria-selected` → active-state, or select → querystring mapping. The *URL gets the param* half is component-level; only the *backend honors the param* half needs e2e.

Write the breakdown in three honest buckets. Ground each call in **which layers the test actually touches**, not in how the test is named:

1. **Genuinely must-be-e2e (keep)** — load-bearing. Name the layer(s) only a browser + backend can verify together.
2. **Defensible-but-narrowable e2e** — justified as integration, but over-split or doing more boots than the coverage requires. Say what to merge.
3. **Should be unit / component tests** — where the real savings are. Name the cheaper layer and what it would assert (often *more precisely* than the e2e test does indirectly).

Then a one-paragraph **bottom line**: what to keep as e2e, what to push down, and — if you pulled CI timing from `e2e/support/timings.json` for the Performance check — roughly how much of that wall time is *earned* by the must-be-e2e set versus paid by the rest.

Rules of engagement so this stays honest, not performative:

- **Don't inflate the keep bucket to look balanced.** If most of the spec is component-test value paying e2e prices, say exactly that. If every test is load-bearing, say that too — a spec where all 12 tests must be e2e is a legitimate outcome, not a failure to find candidates.
- **Be specific.** Cite the `it()` titles and line ranges. "Several tests could be unit tests" is useless; "tests 1/17/18 assert only that N titled cards mount — RTL with a mocked dataset" is actionable.
- **This is advice to the author, not a blocking issue.** It's a recommendation, separate from the numbered issues. Don't renumber it into the issue list.
- **Stay rooted in what exists.** The cheaper layer here is Jest + RTL component/unit tests. Don't invent infrastructure the repo doesn't have, and don't recommend deleting backend-dependent coverage just because part of the test is frontend-only — split it, keep the integration half.

## Final check

1. Trim issues that won't materially help the author.
2. Verify sequential numbering with no gaps.
3. In PR mode, verify each issue was posted as a separate review comment.
4. Verify the honest e2e-vs-unit breakdown is present — it's required in every report, even when the answer is "all of these need to be e2e."