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.