Code Review for QA

Reviewing Test Code: A Different Lens

Reviewing test code is different from reviewing application code. Application code reviews focus on correctness, performance, and maintainability. Test code reviews focus on determinism, isolation, clarity, and meaningful assertions. A test that passes today but fails randomly tomorrow is worse than no test at all.

The 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);
});

Giving Constructive Feedback

The goal of code review is to improve the code, not to demonstrate your superiority. Frame feedback as questions or suggestions, not commands.

Bad Feedback

"This is wrong."
"Why would you do it this way?"
"This will never work."

Better Feedback

"Have you considered using a data-testid selector here? In my experience,
CSS class selectors tend to break when the design team updates styles."

"I think this assertion might be flaky because it depends on animation
timing. What if we wait for the element to be stable first?"

"Nice approach! One thought: could we extract this setup into a fixture
so other tests can reuse it? I see a similar pattern in checkout.spec.ts."

Feedback Calibration

Approve with minor comments rather than blocking on style preferences. Reserve "request changes" for issues that would cause real problems (flakiness, missing assertions, security concerns).

What to Look For in Non-Test PRs (As a QA Engineer)

QA engineers should also review application code PRs through a testability lens:

• Is the change testable? Are there hooks (data-testid attributes, API contracts) that make testing straightforward?
• Are there new error states? New code paths mean new test cases are needed.
• Does it change existing behavior? Existing tests may need updating.
• Are there database migrations? Schema changes can break existing test data.
• Is there adequate error handling? Missing error handling creates untestable failure modes.

// In a review, you might comment:
// "This new endpoint doesn't return a meaningful error for invalid input.
// Could we return a 400 with a JSON body? That would make it much easier
// to write specific assertions in our API tests."

Review Workflow Tips

Use Suggested Changes

GitHub's "suggest changes" feature lets you propose specific code edits that the author can accept with one click:

// Instead of fixed wait, use a dynamic condition
await page.waitForSelector('.results', { state: 'visible' });

Batch Your Comments

Read the entire PR before leaving comments. Your comment on line 10 might be answered by code on line 200. Batch all comments and submit them together.

Re-Review Quickly

When the author addresses your feedback, re-review promptly. A review cycle that takes 3 days each round slows everyone down.

Review Your Own PRs First

Before requesting review, read your own diff as if you were the reviewer. You will often catch:

• Debugging code left in (console.log, test.only)
• Missing test cases for edge scenarios
• Unclear variable names
• Accidental file changes (editor config, lockfile churn)

Hands-On Exercise

1. Pick a recent test PR in your repository and review it using the five-point checklist (determinism, independence, assertions, naming, cleanup)
2. Practice writing feedback for each severity level (blocking, suggestion, nit, question, praise)
3. Review an application code PR through the testability lens. What new tests would be needed?
4. Set up a review rotation where QA engineers cross-review each other's test PRs weekly
5. Create a team review checklist specific to your project's test patterns and post it in your team wiki