Ask HN: Do you pull and run code as part of code review?

#1

Post by gary_chambers » Sun, Mar 06, 2022, 2:17 PM UTC

I've been having a debate about whether code should be checked out and run locally (or on a staging environment) as part of code review. Personally, I find it easier to review a change after I've run it locally, but perhaps that's because I'm not very good at reading code.

I'm interested to hear what other people think. Do you run code as part of code review?

Re: Ask HN: Do you pull and run code as part of code review?

#2

Post by daviddever23box » Sun, Mar 06, 2022, 2:21 PM UTC

Yes, but only in tandem with actually reading the code (not just the changes).

Re: Ask HN: Do you pull and run code as part of code review?

#3

Post by detaro » Sun, Mar 06, 2022, 2:22 PM UTC

No hard-and-fast rule, depends if I feel I need to play through options or not, what others are involved, ...

Re: Ask HN: Do you pull and run code as part of code review?

#4

Post by bryanrasmussen » Sun, Mar 06, 2022, 2:24 PM UTC

it depends on application size, if you have dedicated testers etc.

currently at a place with extremely large code base and with dedicated testers, I don't run the code.

Sometimes even with dedicated testers etc. I will run the code if I think something might be problematic and I want to double check. I tend to only comment on code though if I think it could really be improved, if you have named a variable something I wouldn't I probably won't care.

Re: Ask HN: Do you pull and run code as part of code review?

#5

Post by version_five » Sun, Mar 06, 2022, 2:28 PM UTC

For smaller PRs we usually review without running locally unless there is something specific to check. And then we do separate release reviews where we install and run it locally as part or testing. This is for ML projects though, we are not doing any kind of continuous integration or continuous releases, and it's generally small repositories. It must really depend on the nature of the project and how the reviews fit in.

Re: Ask HN: Do you pull and run code as part of code review?

#6

Post by tharne » Sun, Mar 06, 2022, 2:29 PM UTC

Yes.

I didn't used to, but that was a very big mistake.

Just wait until something you signed off on doesn't run and folks ask, "But didn't you review this code and say that everything 'looked good'? It doesn't even run!"

It's embarrassing to say the least.

If you're doing a code review, run the darn code. What would you think of a mechanic who didn't make sure the car was able to turn on after claiming it was fixed?

Re: Ask HN: Do you pull and run code as part of code review?

#7

Post by auslegung » Sun, Mar 06, 2022, 2:31 PM UTC

So far everyone is saying, “it depends”, and that’s my experience as well. Depending on the complexity of the change, how impactful it will be in production, what the test suite is like, etc. I may or may not pull and run it.

Re: Ask HN: Do you pull and run code as part of code review?

#8

Post by [deleted] » Sun, Mar 06, 2022, 2:43 PM UTC

[deleted]

Re: Ask HN: Do you pull and run code as part of code review?

#9

Post by keyanp » Sun, Mar 06, 2022, 2:55 PM UTC

No almost never. That’s the job of the CI build. If the code doesn’t even run then it should be caught by the tests. If there are no tests that would catch it then they must be written before passing code review.

Re: Ask HN: Do you pull and run code as part of code review?

#10

Post by jhnct » Sun, Mar 06, 2022, 2:58 PM UTC

Yes, UI tests are very hard to maintain on Android so I always pull and run the code to check the actual behavior. As said by others compilation and linting are done by the CI.