Hacker News new | ask | show | jobs
by rinchik 2517 days ago
IMHO, #3 - Usability Test should never be part of the code review, the author of the PR should make sure that changes do what they are supposed to do BEFORE opening a PR.

Developers should be responsible for their own QA. QA is not what code reviews are about.

There are always exceptions, of course. Use of the common sense should be highly encouraged, e.g. if proposed change is complex or reviewer want to visually confirm this change, feel free to checkout this branch, but it is NOT a requirement for a good review.

2 comments

I once worked in an environment that I liked a lot where after review, the reviewer would deploy the code out. I think this enforced some nice ideas like -- code reviews are point of collectivizing the code in the team (so now this is the teams code, and I'm deploying it, so I'm incentivized to not phone it in on the review). It also made sure that the reviewer understood the impacts of code changes as they verified/promoted through staging environment.
Thanks for the input. I plan on creating a version for authors too.

If it's possible in your organization or release strategy, self-deployment is good for the reasons you say, and it corroborates Larry Wall's virtue of hubris [0].

The PR author is like a salesman. You should be making the lives of your customers easier and incentivizing them to look at your product in order to move things forward. That being said, you should prioritize team profit and not your own (i.e. don't be a crooked salesman).

[0]: http://threevirtues.com/

Author here. Thanks for the feedback.

> Developers should be responsible for their own QA. QA is not what code reviews are about.

While I agree that each dev is responsible for their own QA and test should be run before opening a PR, two eyes are better than one. If checking the code out on local and testing doesn't take time and there is risk, then I'd say go for it. Catching changes earlier in a PR is less work overall than having to restart a large part of the process when QA catches them. Also see the link Smotko commented with [0], it also addresses your point.

> if proposed change is complex or reviewer want to visually confirm this change, feel free to checkout this branch

Yes, I specified that usability tests are usually reserved for PRs that introduce sufficient code changes and/or risk.

[0]: https://blog.codereview.chat/2019/06/27/code-reviews-and-you...