Hacker News Reader

Comment by ricardolopes | Hacker News Reader

This should be something agreed within a team, so that review standards are consistent across team members.

In my previous team, the reviewer was the main responsible for the code they were approving. They were expected to test locally and should actively hunt for potential issues, such as checking in the logs that the ORM was building correct SQL.

In my current team, the developer is the main responsible for the code they're pushing, and is expected to do all the tests needed. Reviews are more about ensuring code standards, finding possible oversights, requirements mismatches, and so on.

No one strategy is right or wrong, as long as expectations are set and everyone knows their responsibilities and expectations.

View on HN

>This should be something agreed within a team, so that review standards are consistent across team members.

Why? it feels like individual preference

>such as checking in the logs that the ORM was building correct SQL.

Couldnt the person who creates PR copy/paste sample generated SQLs into PR?

A team standard helps contribute to clear responsibility.

If correct SQL is a priority (which seems reasonable), but it's not clear who should check it, it's likely to not be checked at least some of the time. Same with whatever other things have difuse responsibility.

There's still some room for individual preference. If it's author's repsonsibility to do X, they can ask for the reviewer to do it in the review request; or if it's the reviewer's responsibility, they can approve but say they didn't do X and are relying on the author to do it; or the author can assert they've done X in the request (perhaps it's difficult to do for this speciric request) and the reviewer can note they've relied on that assertion. But having a clear expectation strongly reduces the cases where author was relying on reviewer to check X and the reviewer was relying on the author, and X wasn't checked and the check would have found an issue before production.

Some things are a much bigger problem when found after production, and some things aren't; diffuse responsibility is ok for things that aren't a big deal, IMHO.

Code review isn't really something we do at an individual level because we feel like it, it's something the team, as an entity, does to ensure work is completely to a certain standard of quality. One's personal preference on reviews (or how to perform them) is less important than the team as a whole being comfortable with the process.

To your second point, sure, if that's what the team agrees is better.

As someone said in another reply, setting these team expectations is important so that the developer won't assume the reviewer will do the heavy testing, and the reviewer assumes the developer must have done it, leading to preventable production outages.

The developer can definitely do work like providing samples of generated SQL, UI screenshots and so on. Again, knowing who will do that deep due diligence is more important than the actual decision.

Reply on news.ycombinator.com