Writing Review Feedback That Lands

Learning Objectives

By the end of this lesson you will be able to:

• Write a review comment that gets acted on rather than argued with
• Signal severity so a reviewer knows what blocks and what doesn't
• Decide whether a finding belongs in the PR or in a bug report
• Review a diff as a tester, looking for what a developer reviewer won't

What a Tester Looks For

A developer reviewing a PR is mostly asking "is this code correct and maintainable?" You are asking something different, and the difference is the reason you were invited.

Missing negative cases. A new validation function with three happy-path tests and nothing for empty input, wrong type, or the boundary value.

Boundaries. Off-by-one is still the most common defect class in software. If the code says if (retries < 3), is three retries allowed or not, and does that match the ticket?

Error paths. A catch block that logs and continues. What does the user see? What does the caller receive? Is a failure now silent?

Changes with no test change. A source file modified with no corresponding test file touched. The single easiest high-value observation in code review.

Test quality, not just test presence. A test that asserts expect(result).toBeTruthy() on a function that returns an object proves almost nothing.

Configuration and defaults. A changed timeout, a new feature flag, an altered retry count. Which environments does it apply to, and is the default the safe one?

Anatomy of a Comment That Works

Four elements. Comments that get acted on almost always have all four.

1. What you observed. Specific, on the exact line.

2. Why it matters. The consequence, not just the fact.

3. What you did. Evidence, if you ran anything.

4. What you're asking for. A question or a concrete suggestion.

Compare:

❌ "This looks like it might break."

✅ "validateEmail returns true for an empty string here — line 34 checks email.includes('@') only after the null guard, so '' never reaches it. I checked out the branch and confirmed: validateEmail('') returns true. PAY-4102's AC says empty should be rejected. Should this get an explicit empty check, or is that handled upstream?"

The second is longer, and it will be fixed today. It names the line, explains the mechanism, provides evidence, references the acceptance criteria, and ends with a question that has an easy answer.

Signal Severity

Reviewers cannot read your mind about what blocks a merge. Say it. Many teams use conventional prefixes:

blocking: the expired-token path returns 200 with an empty body; the client treats that as success. Reproduced at d4e5f6a.

suggestion: this test would be clearer with the fixture inline rather than in beforeEach, but that's taste.

praise: thank you for adding the boundary tests for retries — that's exactly the case that bit us in PAY-3980.

That last one is not filler. Review that is only ever criticism trains people to dread it, and a tester who acknowledges good testing gets taken more seriously when they push back.

PR Comment or Bug Report?

The rule of thumb: if it is inside the diff, it belongs in the PR. If the PR merely made you notice something older, raise it separately — attaching it to the PR makes an unrelated change into that author's problem, which is both unfair and a good way to slow the PR down.

Tone

Review comments are read by a person who spent two days on this and is aware that everyone can see the thread.

• Comment on the code, not the author. "This function returns true for empty input" — not "you forgot to handle empty input".
• Ask rather than assert when you are unsure. "Is '' handled upstream?" costs nothing if you're wrong. "This is broken" costs credibility.
• Give evidence, not adjectives. A reproduction beats a strong opinion every time.
• Do not review the person's approach when you were asked about behaviour. Stay in your lane and your comments carry more weight in it.

You are not the gatekeeper. You are the person who ran it and can say what happened.

A Complete Review

What a tester's review of a PR reasonably looks like:

Tested at d4e5f6a. Checked out locally, npm ci && npm test — 47/47 green. Also merged origin/main in locally and re-ran: still green.

Manual checks: happy path, expired token, malformed token, empty email, 200-character email. Ukrainian and English locales.

blocking: (line 34) validateEmail('') returns true — empty email is accepted and the account is created with a null email. AC in PAY-4102 says it should be rejected. Reproduced locally.

question: (line 61) is SUBMIT_TIMEOUT = 5000 intended for all environments? Our CI runner is slower and 5s may make this flaky there.

suggestion: LoginView.test.js covers the new ID but not the old one — worth a test asserting the old login-btn is genuinely gone, so nothing quietly depends on it.

praise: the boundary tests on retries are exactly right.

One blocking item, one question, one suggestion, one acknowledgement, and a clear statement of what was actually run and at which commit. That is a review a developer can work through in ten minutes — and it is what this whole trail has been building towards.

Pro Tip: Post your review before the CI finishes if you have already tested locally. Being the first substantive comment on a PR is how a tester becomes someone whose review is waited for rather than worked around.

Key Takeaways

• Look for missing negative cases, boundaries, silent error paths, and code changed without tests
• A comment that lands has four parts: observation, consequence, evidence, request
• Prefix comments with blocking / question / suggestion / nitpick / praise so severity is unambiguous
• Findings inside the diff go in the PR; pre-existing problems go in a bug report, linked
• Comment on the code, ask rather than assert, and give a reproduction instead of an adjective
• State what you ran and at which commit — that is the evidence only you can provide