e2e-selector-quality-critic

git diff --name-only origin/${{ github.base_ref }}...HEAD \
  | xargs grep -l -E "(@playwright/test|cypress|webdriverio|selenium-webdriver|cy\.|browser\.|page\.)" 2>/dev/null

// Original (line 12)
await page.locator('.button-primary.submit-button').click();

// Look at surrounding HTML / similar tests for context.
// If the button has a visible label "Submit" or aria-label="Submit":
await page.getByRole('button', { name: 'Submit' }).click();

// If the button has data-testid="submit-button":
await page.getByTestId('submit-button').click();

// Original
await page.locator('xpath=//div[@class="cart"]//button[1]').click();

// Recommendation:
await page.getByRole('button', { name: /add to cart/i }).first().click();
// OR if there are multiple visible buttons, narrow:
await page.locator('section', { hasText: 'Cart' }).getByRole('button').first().click();

// Original
await page.locator('li:nth-child(2)').click();

// Recommendation:
await page.getByRole('listitem').filter({ hasText: 'Premium plan' }).click();

## E2E selector quality critic — `<PR>`

**E2E test files reviewed:** N
**Selectors walked:** M
**Findings:**

| Class              | Count | Action |
|--------------------|------:|--------|
| `accessibility`    |   142 | (no action) |
| `semantic-ok`      |    34 | (no action) |
| `testid`           |    18 | (no action — acceptable last-resort) |
| `css-class`        |    24 | Replace with role / label query. |
| `nth-position`     |     8 | Replace with role + filter. |
| `xpath`            |     3 | Replace; xpath is most brittle. |
| `id` (auto-gen)    |     5 | Replace with role / label or `data-testid`. |
| Non-web-first asserts |  12 | Replace with `await expect(...).toBeVisible()`. |

### Per-finding detail

#### `tests/checkout.spec.ts:42` — CSS class

**Original:** `await page.locator('.button-primary').click();`

**Recommendation:** `await page.getByRole('button', { name: 'Submit' }).click();`

The button has visible text "Submit" per the production HTML.
Per [pw-best-practices][pwb], CSS classes are brittle. Per
[tl-queries][tl] §priority 1, `getByRole` is the preferred query.

#### `tests/cart.spec.ts:18` — XPath

**Original:** `cy.get('xpath=//div[@class="cart"]//button[1]').click();`

**Recommendation:** `cy.findByRole('button', { name: /add to cart/i }).click();`

XPath is the most brittle selector class per [pw-best-practices][pwb].
The button has visible text "Add to Cart"; use the accessible-name
query.

#### `tests/profile.spec.ts:7` — Non-web-first assertion

**Original:**
```typescript
expect(page.locator('.toast').isVisible()).toBe(true);

await expect(page.locator('.toast')).toBeVisible();

## Refuse-to-proceed rules

The agent **refuses** to:

- Auto-rewrite selectors. The replacement requires production-HTML
  context the agent doesn't always have.
- Flag `getByTestId` usage when no accessibility-friendly
  alternative exists in the production code (e.g. an icon-only
  button without `aria-label`). Recommend the production-side fix
  too, not just the test rewrite.
- Operate on unit / integration tests. Strictly E2E test files
  (Step 1).

## Anti-patterns

| Anti-pattern                                                          | Why it fails                                                              | Fix |
|-----------------------------------------------------------------------|---------------------------------------------------------------------------|-----|
| Treating `getByTestId` as always wrong                                | It's the documented last-resort per [tl-queries][tl]; not always wrong.  | Flag only when accessibility selectors would work (Step 2). |
| Recommending `.first()` / `.nth(0)` chains                             | Same brittleness as `:nth-child(0)` - index-based selection.             | Recommend filtering (`.filter({ hasText: ... })`) instead. |
| Flagging Selenium tests with CSS selectors when Selenium is the team's deliberate choice | Selenium predates accessibility-first locators; the team may be locked in. | Note Selenium's age but recommend the migration to its modern accessibility-aware extensions. |
| Auto-rewriting xpath without DOM context                               | Risk of incorrect replacement.                                            | Recommend with confidence flag: `(verified)` if production HTML supports the recommendation; `(needs review)` otherwise. |
| Treating `await page.waitForTimeout(...)` as "non-web-first only"      | Sometimes wait is the right call (animation completion).                  | Flag generic waits but accept commented-justified waits (`// wait for animation`). |

## Limitations

- **Replacement context.** Without seeing the production HTML, the
  agent's recommendations are best-guess based on visible test
  context (e.g. variable names, surrounding assertions).
- **Per-framework selector adapters.** New E2E frameworks (Storybook
  test-runner, Maestro, Detox) need adapter updates.
- **No accessibility-tree query.** The agent doesn't actually run
  the production code to check whether `getByRole` would find the
  element; it recommends based on heuristics. Verify the
  recommendation by running the test.
- **Cypress without cypress-testing-library** has no `findByRole`
  primitive; the recommendation is to install the library first.

## Hand-off targets

- **WCAG keyboard / focus-trap / contrast checks (production-side
  accessibility, not test-side selectors)** → see the
  `qa-accessibility` plugin.
- **WCAG compliance reporting** → see
  [`wcag-compliance-reporter`](../../qa-accessibility/skills/wcag-compliance-reporter/SKILL.md).
- **Test-code structure / naming** → [`test-code-critic`](test-code-critic.md).
- **Assertion specificity** → [`assertion-quality-reviewer`](assertion-quality-reviewer.md).
- **Mocking patterns** → [`mocking-anti-pattern-detector`](mocking-anti-pattern-detector.md).

## References

- [pw-best-practices][pwb] - Playwright best practices: prefer
  `getByRole`; CSS classes + XPath brittle; web-first assertions;
  "automated tests should verify that the application code works
  for the end users, and avoid relying on implementation details."
- [tl-queries][tl] - Testing Library query priority: `getByRole`
  (priority 1), labels / text / placeholder (priorities 2-3),
  testid (last resort because "the user cannot see (or hear) these").
- [`test-code-conventions`](../skills/test-code-conventions/SKILL.md)
  §8 + §9 - selector priority + web-first assertion conventions.