Should PR reviews check for code correctness?

Question

When doing PR reviews, should the reviewer be checking that the code is correct (e.g. the logic is correct, etc.) or should the only focus be higher level concepts (e.g. architectural/functional/etc.)?

I've been doing a lot of code correctness checking in PRs, mainly because we have been seeing some quality issues, and it really drains you, but also I keep catching lots of bugs. So, if I stop then more bugs at least in the short term will likely appear.

I'm not sure if I should be letting this stuff slip through as a matter of process and then if we are getting bugs the developer needs to improve or we need to improve our PR requirements e.g. testing etc. So rather make it a process problem or a hiring problem rather than patching up the real issue by scrutinizing code correctness in PRs.

I'm not sure what the best answer is here.

Answer 1

There is more than one way to shave a cat.

In an ideal organization which has a good testing process, and where only experienced developers work who regularly write unit tests, you can leave verification of correctness fully to the authors and dedicated testers. Here PRs can be restricted to verify if there are enough (and enough of the right) unit tests, if the responsibilities between components haven't been compromised, or if a certain change should be refactored.

However, not every organization works this way. If you want to achieve a certain level of quality, someone in your organization has to do double checks and tests. Moreover, if a dev is reading and understanding some PR, they may find potential issues in the code which are unlikely to be found by a tester who treats the code just as black-box. I think it would be inefficient to ignore such issues, sweep them under the rug and just hope that a tester or the author will find the same issue by chance as well.

Of course, when this case occurs, in some organizations the reviewer may be able to ask the author of the PR to write an additional unit test to proof their code works. Or the reviewer may be able to give the tester a hint what they shall test specifically. But in some organizations, the reviewer must be the one who brings proof to the author that the issue they saw in code is a real one (by presenting some test case, manual or automated).

So in short, this is not a black-and-white situation - at the end of the day, you have to work out with your team what works best for your organization, how much testing shall be done as part of the review process and who will take the responsibility for correctness in which case. But what surely not works is saying to them "testing is not part of the PR process" and just hope that someone else will do it.

Answer 2

If you attempt to double check code in PRs you are effectively writing the code yourself and will end up having no time for anything but PRs

Instead decide what the rules for passing a PR are and check that they have been followed. For example.

1. Are there automated tests
2. Do they pass
3. Does the functionality described by the test match the requirements. ie. have the requirements been misinterpreted/misunderstood/skipped.
4. Is the architectural design been followed. ie has a console app been written instead of an API

1 and 2 check that the person who wrote the code has checked that it worked so you dont have to.

3 checks that what they think "working" means is the same as what you think "working" means.

4 checks we are all heading in the same direction.

You may also catch some gotchas or bugs if you know the code base and problems that have come up before. But you shouldn't be spending too much time checking. That's what tests are for.

Answer 3

A real quick and dirty(?) way to determine if the logic/mechanics of the code is error prone, is to note the amount of comments the engineer puts into his work. I used to be a service manager for a technical company that dealt mainly with electro-mechanical devices, and as a result I would get numerous calls from field engineers wanting help in solving a particular problem. I had to answer the phone, as there was no alternative at the time. But along came email and things changed. At that point, I made the engineers email me, which meant that they would have to sit down and put their issue into words. That meant that they had to sit down and actually think about the problem and what they had done so far to solve it. The number of help requests dropped dramatically after that. Part of it was that some of them were just downright lazy and didn't want to bother typing, but, for the most part, it forced the engineers to actually sit down and think about it. Lack of comments in code is just being lazy and selfish. A lot of engineers don't care if their code works first time, they rely on code reviews to finish their job.