4.2The QA Review Checklist

When reviewing another engineer's test PRs, evaluate each test against these criteria:

1. Determinism

Does the test produce the same result every time, regardless of when or where it runs?

Red flags:

• Depending on the current date or time (new Date())
• Relying on specific database state that other tests might modify
• Using Math.random() or other non-deterministic inputs without seeding
• Waiting for fixed durations (sleep(3000)) instead of dynamic conditions
• Depending on the order of elements in a set or unordered collection

// BAD: Depends on current time
test('show greeting', async () => {
  await page.goto('/');
  // This test fails after 6 PM
  await expect(page.locator('.greeting')).toHaveText('Good morning');
});

// GOOD: Mock the time
test('show morning greeting before noon', async () => {
  await page.clock.setFixedTime(new Date('2024-06-15T09:00:00'));
  await page.goto('/');
  await expect(page.locator('.greeting')).toHaveText('Good morning');
});

2. Independence

Can this test run in isolation, or does it depend on other tests running first?

Red flags:

• Test B assumes Test A created a user in the database
• Tests share a global variable that gets modified
• Test order matters (fails when run with --randomize-order)
• Cleanup happens in a "last" test rather than in afterEach/afterAll

// BAD: Depends on previous test
test('login with created user', async () => {
  // Assumes "create user" test ran first and created "testuser@example.com"
  await loginAs('testuser@example.com');
});

// GOOD: Each test creates its own data
test('login with valid credentials', async () => {
  const user = await createTestUser({ email: 'login-test@example.com' });
  await loginAs(user.email, user.password);
  await expect(page).toHaveURL('/dashboard');
  await deleteTestUser(user.id); // Cleanup
});

3. Meaningful Assertions

Are assertions specific enough to catch real bugs but not so brittle they break on irrelevant changes?

Red flags:

• Asserting on exact pixel positions or screenshot matches without tolerance
• Checking only that a page loaded (no specific content verified)
• Asserting on implementation details (internal class names, data attributes that may change)
• Missing assertions (the test performs actions but never verifies outcomes)

// BAD: Too brittle -- breaks when any text changes
await expect(page.locator('.cart')).toHaveText(
  'Your cart contains 3 items totaling $59.97 including tax'
);

// BAD: Too loose -- passes even when the feature is broken
await expect(page.locator('.cart')).toBeVisible();

// GOOD: Specific enough to catch bugs, stable enough to survive minor changes
await expect(page.locator('.cart-count')).toHaveText('3');
await expect(page.locator('.cart-total')).toContainText('$59.97');

4. Naming

Can you understand what the test verifies from its name alone?

Red flags:

• test1, test2, test_new
• Names that describe what the test does, not what it verifies
• No describe blocks to group related tests

// BAD: Meaningless names
test('test1', async () => { /* ... */ });
test('checkout works', async () => { /* ... */ });

// GOOD: Self-documenting names
describe('checkout', () => {
  test('displays order summary with correct item count and total', async () => { /* ... */ });
  test('shows validation error when credit card is expired', async () => { /* ... */ });
  test('redirects to confirmation page after successful payment', async () => { /* ... */ });
});

5. Cleanup

Does the test clean up after itself?

Red flags:

• Test creates users, orders, or files but never deletes them
• Browser state (cookies, localStorage) leaks between tests
• Temporary files accumulate and eventually cause disk space issues

// GOOD: Explicit cleanup
let testUser: User;

test.beforeEach(async () => {
  testUser = await createTestUser();
});

test.afterEach(async () => {
  await deleteTestUser(testUser.id);
});