Item — HN Reader

> It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes.

Actually, that's not true at all. Code review is not a replacement for automated builds and testing. That's what Continuous Integration is for. Let humans concentrate on what they do best (understanding and comment on code) and don't lose their time and patience with things that can be automated.

Projects often enforce that by running CI on code submitted for review and it cannot be accepted as long as a CI fails. It also reduces breakages for other devs of the builds of the code base.

1 comments

> Code review is not a replacement for automated builds and testing.

I agree, and never said it was. But the fact is most projects don't have complete test coverage, or even replicate the real world environment of actually using the program. In the absolute best case scenario where they do, I agree with you; automated tests and CI should find any functional issues. But if your software is meant to be used by a human, then at some point it should also be tested by one. Preferably before your actual users test it for you.

It depends on the project, of course, so if all you're working on is a library or API consumed by machines, then by all means, rely entirely on CI if you can. Same goes if your project is a small piece of a much larger machinery, and it's difficult to test locally, then it makes sense to rely more on CI in that case as well.

But you can't expect humans to actually interpret all the nuance and complexity of a non-trivial code change from just staring at patches. At least, I'm not that smart. So I typically pull the changes locally, go through the relevant files, try to manipulate inputs and see what the effects are (i.e. manual fuzzing), compare the behavior before and after the changes, and, finally, since I already have the code locally, build and run the program as a last quick check that things actually work. Sure, this is time-consuming sometimes, but it's been helpful for finding issues or just understanding the code more than if I wouldn't do this.

I expect the submitter to have done that. The most annoyed I get in code reviews when it is clear that the tests are green but the code is totally broken and the submitter has never bothered to test it. And at my old job there were things that to properly do a manual QA would have taken all day because it wasn't possible to test on a laptop (e.g. wrong O/S and no ability to easily spin up an AIX image to start with). Really feel like this needs to not be a job of the reviewer, but needs to be on the submitter, but a lot of junior devs or open source contributors tend to skip that step and treat green tests as the definition of done, rather than working software.