E2E Test Scenario Quality Review

Systematic checklist for reviewing E2E spec files AND Page Object Model (POM) files. Framework-agnostic principles; code examples show Playwright, Cypress, and Puppeteer where they differ.

Review Checklist

Run each check against every non-skipped test and every changed POM file.

Important: test.skip() with a reason comment or reason string is intentional — do NOT flag or remove these. Only flag mid-test conditional skips that hide failures (see #6).

Tier 1 — High-Impact Bugs (always check)

1. Name-Assertion Alignment

Symptom: Test name promises something the assertions don’t verify.

// BAD — name says "status" but only checks visibility
test('should display paragraph status', () => {
  await expect(status).toBeVisible();  // no status content check
});

Rule: Every noun in the test name must have a corresponding assertion. Add it or rename.

2. Missing Then

Symptom: Test acts but doesn’t verify the final expected state.

// BAD — toggles but doesn't verify the dismissed state
test('should cancel edit on Escape', () => {
  await input.click();
  await page.keyboard.press('Escape');
  await expect(text).toBeVisible();
  // input still hidden?
});

Rule: For toggle/cancel/close actions, verify both the restored state AND the dismissed state.

3. Error Swallowing

Symptom (spec): try/catch wrapping assertions — test passes on error.

Symptom (POM): .catch(() => {}) or .catch(() => false) on awaited operations — caller never sees the failure.

// BAD spec — silent pass
try { await expect(header).toBeVisible(); }
catch { console.log('skipped'); }

// BAD POM — caller thinks execution succeeded
await runningIndicator.waitFor({ state: 'detached' }).catch(() => {});

Rule (spec): Never wrap assertions in try/catch. Use test.skip() in beforeEach if the test can’t run.

Rule (POM): Remove .catch(() => {}) / .catch(() => false) from wait/assertion methods. If the operation can legitimately fail, the caller should decide how to handle it. Only keep catch for UI stabilization like editor.click({ force: true }).catch(() => textArea.focus()).

4. Always-Passing Assertions

Symptom: Assertion that can never fail.

// BAD — count >= 0 is always true
expect(count).toBeGreaterThanOrEqual(0);

Rule: Search for toBeGreaterThanOrEqual(0), toBeTruthy() on always-truthy strings, || chains that accept defaults as valid.

5. Boolean Trap Assertions

Symptom (spec): expect(bool).toBe(true) — failure message is just “expected false to be true”.

Symptom (POM): Method returns Promise<boolean> instead of exposing an element handle — forces spec into boolean trap.

// BAD — boolean return forces spec into trap
async isEditorVisible(index = 0): Promise<boolean> {
  return await paragraph.locator('code-editor').isVisible();
}
expect(await page.isEditorVisible(0)).toBe(true);

Rule (spec): Use the framework’s built-in assertion instead of extracting a boolean first:

• Playwright: await expect(locator).toBeVisible()
• Cypress: cy.get(selector).should('be.visible')
• Puppeteer: await page.waitForSelector(selector, { visible: true })

Rule (POM): Expose the element handle (Locator / selector string) instead of returning Promise<boolean>. Let specs use framework assertions directly.

6. Conditional Bypass (Silent Pass / Hidden Skip)

Symptom: expect() inside if block, or mid-test test.skip() — test silently passes when feature is broken.

// BAD — if spinner never appears, assertion never runs
if (await spinner.isVisible()) {
  await expect(spinner).toBeHidden({ timeout: 5000 });
}

Rule: Every test path must contain at least one expect(). Move environment checks to beforeEach or declaration-level test.skip().

7. Raw DOM Queries (Bypassing Framework API)

Symptom: Test drops into raw document.querySelector* / document.getElementById via evaluate() when the framework’s element lookup API could do the same job.

// BAD — no auto-wait, returns stale boolean
const has = await page.evaluate((i) => {
  return !!document.querySelectorAll('.para')[i]?.querySelector('.result');
}, 0);
expect(has).toBe(true);

Why it matters: No auto-waiting, no retry, boolean trap, framework error messages lost.

Rule: Use the framework’s element API instead of raw DOM:

• Playwright: page.locator() + web-first assertions
• Cypress: cy.get() / cy.find() — avoid cy.window().then(win => win.document.querySelector(...))
• Puppeteer: page.$() / page.waitForSelector() — avoid page.evaluate(() => document.querySelector(...))

Only use evaluate/waitForFunction when the framework API can’t express the condition (getComputedStyle, cross-element DOM relationships). In POM, add a comment explaining why.

Tier 2 — Quality Improvements (check when time permits)

8. Render-Only Tests (Low E2E Value)

Symptom: Test only calls toBeVisible() with no interaction or content assertion.

Rule: Add at least one of: content assertion (not.toBeEmpty(), toContainText()), count assertion (toHaveCount(n)), or sibling element assertion.

9. Duplicate Scenarios (DRY)

Symptom: Two tests share >70% of their steps with minor variations.

Rule (within file): Merge tests that differ only in setup or a single assertion. Use the richer verification set from both.

Rule (cross-file): After reviewing all files in scope, cross-check tests with similar names across different spec files. If test A in feature-settings.spec.ts is a subset of test B in feature-form-validation.spec.ts, delete A and strengthen B.

10. Misleading Test Names (KISS)

Symptom: Name implies UI interaction but test uses API/REST, or name implies feature X but tests feature Y.

Rule: If the test uses REST API, reload, or indirect methods, the name must make that explicit.

11. Over-Broad Assertions (KISS)

Symptom: Assertion too loose to catch regressions.

// BAD — any string containing '%' passes
expect(content.includes('%')).toBe(true);

Rule: Prefer exact matches or explicit value lists over .includes() or loose regex when valid values are known and small.

12. Hard-coded Timeouts

Symptom: waitForTimeout() or magic timeout numbers scattered across tests and POM.

// BAD — arbitrary sleep
await page.waitForTimeout(2000);

// BAD — magic number, no explanation
await element.waitFor({ state: 'visible', timeout: 30000 });

Rule: Never use explicit sleep (waitForTimeout / cy.wait(ms)) — rely on framework auto-wait or retry mechanisms. For custom timeouts, extract named constants with comments explaining why the default isn’t sufficient.

13. Flaky Selectors

Symptom: Positional selectors or unstable text that breaks across environments.

// BAD — breaks if DOM order changes
await expect(items.nth(2)).toContainText('Settings');

Rule: Prefer data-testid, role-based, or attribute-based selectors over nth() or raw text. If nth() is unavoidable, add a comment. For text selectors, use regex with i flag or hasText filter.

14. YAGNI in Page Objects

Symptom: POM has locators/methods never referenced by any spec.

Procedure:

1. List all public members of each changed POM file
2. Grep each member across all test files and other POMs
3. Classify: USED / INTERNAL-ONLY (private) / UNUSED (delete)

Common patterns: Convenience wrappers (clickEdit() when specs use editButton.click()), getter methods (getCount() when specs use toHaveCount()), state checkers (isEditMode() when specs assert on elements directly), pre-built “just in case” locators.

Rule: Delete unused members. Make internal-only members private. Shared utils must be used by 2+ specs.

Output:

| File | Member | Used In | Status |
|------|--------|---------|--------|
| page.ts | addLinks | (none) | DELETE |
| page.ts | searchDialog | internal only | PRIVATE |

Output Format

Present findings grouped by severity:

## [HIGH/MEDIUM/LOW] Task N: [filename] — [issue type]

### N-1. `[test name or POM method]`
- **Issue:** [description]
- **Fix:** [name change / assertion addition / merge / deletion]
- **Code:**
  ```typescript
  // concrete code to add or change

**Severity guide:**
- **HIGH:** Error swallowing, always-passing, conditional bypass, raw DOM queries — tests that silently pass when broken
- **MEDIUM:** Boolean traps, missing Then, duplicates — tests that work but give poor diagnostics or waste CI time
- **LOW:** Render-only, naming, YAGNI, timeouts — quality/maintenance improvements

## Quick Reference

| # | Check | Tier | Detection Signal |
|---|-------|------|-----------------|
| 1 | Name-Assertion | T1 | Noun in name with no matching `expect()` |
| 2 | Missing Then | T1 | Action without final state verification |
| 3 | Error Swallowing | T1 | `try/catch` in spec, `.catch(() => {})` in POM |
| 4 | Always-Passing | T1 | `>=0`, truthy on non-empty, `\|\|` defaults |
| 5 | Boolean Trap | T1 | `expect(bool).toBe(true)`, POM returns boolean |
| 6 | Conditional Bypass | T1 | `expect()` inside `if`, mid-test `test.skip()` |
| 7 | Raw DOM Queries | T1 | `document.querySelector` in `evaluate` bypassing framework API |
| 8 | Render-Only | T2 | Only `toBeVisible()`, no content/count |
| 9 | Duplicate | T2 | >70% shared steps, cross-file overlap |
| 10 | Misleading Name | T2 | API/reload in "should [UI verb]" test |
| 11 | Over-Broad | T2 | `.includes()` where enum values known |
| 12 | Hard-coded Timeout | T2 | `waitForTimeout()`, magic numbers |
| 13 | Flaky Selectors | T2 | `nth()` without comment, raw text matching |
| 14 | YAGNI in POM | T2 | Public member not referenced in any spec |