Reviewing E2E tests in pull requests: a practical guide

The last mile of automation: code review for E2E tests

By day 99 of your Playwright journey, you've likely built a solid test suite. But a test that passes locally can still introduce maintenance debt, flakiness, or false confidence in your CI pipeline. Reviewing E2E tests in pull requests means catching these issues before they compound. This practical guide walks you through the key checks that keep suites maintainable and reliable.

Why E2E test review differs from feature code review

E2E tests are deceptively simple to write but remarkably easy to write badly. A unit test might fail immediately if the assertion is wrong. An E2E test, by contrast, can pass reliably in CI while being fragile, slow, or dependent on brittle selectors. When you review E2E tests in pull requests, you're not just checking logic—you're enforcing patterns that prevent cascading failures across your suite. A single flaky test can block releases and erode team confidence in automation.

The stakes are also different. A bug in feature code affects users directly. A bug in E2E tests affects your ability to catch bugs in feature code. That inversion makes preventive review essential.

Check locators use roles and labels

The most common mistake in E2E tests is relying on CSS class names or arbitrary data attributes for selection. These break the moment a designer refactors the UI or a framework change rewrites the DOM. Strong locators are rooted in accessibility semantics—roles and labels that reflect what users actually see and interact with.

When reviewing a PR, flag any locator that looks like this:

// ❌ Fragile: relies on internal class names
await page.locator('.btn-primary-v2').click();
await page.locator('div.modal-wrapper > div:nth-child(2)').fill('email');

Instead, encourage the author toward role-based and label-based locators:

// ✅ Resilient: based on semantic HTML and accessibility attributes
await page.getByRole('button', { name: 'Submit' }).click();
await page.getByLabel('Email address').fill('user@example.com');
await page.getByRole('heading', { name: /welcome/i }).isVisible();

The difference is profound. Role-based locators stay valid even when CSS classes change. They also force the test author to think like a user: "Am I clicking a button that says 'Submit' or a div that looks clickable?" That perspective shift makes tests more meaningful.

In your review comment, ask: "Does this locator describe what the user sees, or does it depend on internal class names?" If a button or input lacks proper labels, that's often a signal that the feature itself has an accessibility gap worth fixing.

No fixed waits

Fixed waits (await page.waitForTimeout(2000)) are a code smell in E2E tests. They mask underlying issues: maybe the page is slow to load, maybe the element hasn't appeared yet, or maybe the test is just poorly structured. Fixed waits also inflate test duration without improving reliability. A suite with ten tests that each include a 2-second hardcoded wait becomes a 20-second bottleneck.

When you spot a fixed wait in review, ask why it's there. Usually the answer reveals the real problem:

// ❌ Hardcoded wait hides the issue
await page.waitForTimeout(2000);
await page.getByRole('button', { name: 'Save' }).click();

// ✅ Wait for the actual trigger
await page.getByRole('spinbutton', { name: 'Processing' }).isHidden();
await page.getByRole('button', { name: 'Save' }).click();

Or in another scenario:

// ❌ Sleeping before an action
await page.waitForTimeout(1000);
const confirmButton = await page.getByRole('button', { name: 'Confirm' });
await confirmButton.click();

// ✅ Wait for the element to be ready
await page.getByRole('button', { name: 'Confirm' }).click();
// Playwright automatically waits for actionability

Playwright has excellent built-in waiting mechanisms. By default, it waits for elements to be actionable (visible, enabled, not obscured) before interacting with them. Explicit waits like waitForSelector, waitForNavigation, or waitForLoadState are appropriate when you need to synchronize with actual page changes. Random sleeps are almost never appropriate.

In review, if a test includes waitForTimeout, consider it a red flag. Either the test needs restructuring, or the test setup needs to seed the right conditions upfront.

Data created per test

E2E tests that depend on shared, persistent test data become brittle and hard to parallelize. If test A expects a user account to already exist, and test B deletes it, tests fail in random order. If multiple tests run in parallel and all expect to find a specific product, they collide. The solution is straightforward: each test should create its own data, in isolation.

In review, look for tests that query hardcoded IDs or assume pre-existing records:

// ❌ Depends on pre-existing, shared state
test('user can view dashboard', async ({ page }) => {
  await page.goto('/dashboard/user-42');
  await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible();
});

// ✅ Creates data per test
test('user can view dashboard', async ({ page, context }) => {
  const user = await createTestUser(context);
  await page.goto(`/dashboard/${user.id}`);
  await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible();
});

The second approach has several advantages. It runs safely in parallel—each test gets its own user. It's self-documenting: the test setup shows exactly what state is needed. And it's resilient: if you delete the test user afterward, the test still passes the next run.

For API-based setup, use fixtures or helper functions:

// In your fixtures or helpers
async function createTestUser(context: BrowserContext) {
  const response = await context.request.post('/api/users', {
    data: {
      name: `Test User ${Date.now()}`,
      email: `test-${Date.now()}@example.com`,
    },
  });
  return response.json();
}

test('user can update profile', async ({ page }) => {
  const user = await createTestUser(page.context());
  await page.goto(`/profile/${user.id}`);
  await page.getByLabel('Full name').fill('Updated Name');
  await page.getByRole('button', { name: 'Save' }).click();
  await expect(page.getByText('Profile updated')).toBeVisible();
});

In your review, ask: "Will this test pass if run in isolation? Will it pass if run ten times in parallel?" If the answer is no, the test needs per-test data setup.

Assertions on user-visible outcomes

The purpose of an E2E test is to verify that users experience what they expect. That sounds obvious, but many tests verify implementation details instead: checking that a Redux store was updated, or that a specific API call was made, or that an internal counter incremented. These assertions make tests brittle because they're tightly coupled to architecture choices that change frequently.

When reviewing assertions, ask: "Could a user see or experience this outcome?" If not, consider whether the test is really testing what matters.

// ❌ Asserts on implementation
test('product added to cart', async ({ page }) => {
  const cartState = await page.evaluate(() => window.__state__.cart);
  expect(cartState.items.length).toBe(1);
});

// ✅ Asserts on user-visible outcome
test('product added to cart', async ({ page }) => {
  await page.getByRole('button', { name: 'Add to cart' }).click();
  await expect(page.getByRole('status')).toContainText('Added to cart');
  // Or: verify the item appears in the cart UI
  await page.getByRole('link', { name: 'View cart' }).click();
  await expect(page.getByRole('row', { name: /product name/i })).toBeVisible();
});

The second approach survives refactoring. You could swap Redux for Zustand, change the API endpoint, or redesign the cart UI—the test would still verify the user's perspective: "I clicked a button and the item appeared in my cart."

In review, if you see assertions on internals, suggest pivoting to user-visible outcomes. Ask the author: "How does a user know this worked?"

Common mistakes in E2E test review

• Overlooking selector brittleness: A test that uses CSS selectors still passes during review. The fragility only shows up weeks later when the designer refactors CSS. Make selector review part of every PR.
• Accepting "it works on my machine": A test that passes locally might fail in CI due to different timing, environment variables, or browser versions. Always ask: will this pass consistently in headless mode and in parallel?
• Ignoring test isolation: A test that passes in isolation might fail when run with others. Check test setup and teardown. Verify that tests don't depend on execution order.
• Allowing vague assertions: expect(response.ok).toBe(true) is not enough. What did the user do, and what should they see? Make assertions specific.
• Missing error cases: Happy-path tests are important, but E2E suites need to cover error states too. Did the validation message appear? Did the user get helpful feedback?

Key takeaways

• Locators should be rooted in accessibility semantics (roles and labels), not internal CSS class names, to survive UI refactors.
• Fixed waits mask real issues and inflate test duration. Use Playwright's built-in waiting mechanisms and explicit synchronization instead.
• Each test should create its own data in isolation to run safely in parallel and remain resilient to state changes.
• Assertions should verify what users see and experience, not implementation details like state objects or API call counts.
• E2E test review is preventive maintenance. Catching patterns early prevents cascading failures and keeps your suite a reliable source of truth.