> 1. Preliminary Checks
+1 for this, but I would also add that you need to add as much formatting/linting into this step as possible. You shouldn't be pointing out formatting issues in your code review.
> 2. Understanding
This is super important! Pull requests that do not explain their purpose in the description should not be reviewed.
The PR description is also really important when git bisect leads you to the PR when debugging a new bug.
> 3. Usability Test
A lot of debate regarding this, but I believe this is extremely important if not the most important part of your code review. For details read this section[0]
> 4. Code Review
A few good tips in the article, I'd just add that you should be polite and try not to waste people's time.
[0] https://blog.codereview.chat/2019/06/27/code-reviews-and-you...
> you need to add as much formatting/linting into [step 1] as possible
Yes, project formatting is a whole other topic but this assumes the project already has the team's linting/formatting rules baked into the build. As a reviewer, we are pointing out failing builds here, whatever they may be. If there are deeper issues/disagreements with the build tasks, that would be a separate issue.
Thanks for the link by the way.
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.
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).
> 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...
This helps with understanding for the review, and also if it's referenced again in the future (eg, someone is coming back to fix a bug that's later found to be introduced by the change). It takes only a minute or two extra to do this, while saving the reviewer a lot of time (checking out and running) and/or mental gymnastics (reasoning about what CSS/html changes will actually look like).
Obvious example of where this is useful is for a UI change (with before+after and/or annotated screenshots), but I've also found this useful for timing bugs (eg logs), and data-driven code (eg where output depends heavily on data).
