Ask HN: Do you pull and run code as part of code review? • Hacker News Board

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

#61

Post by makapuf » Sun, Mar 06, 2022, 5:36 PM UTC

azornathogron wrote: post #19

tharne wrote: post #6

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?

Making the code correct is a shared responsibility between reviewer and author; if the code has a bug or doesn't even run that means both people missed the problem. Unless the author is very new/junior (still at a stage where they need close guidance on individual changes) then I would be more annoyed or concerned by an author who hasn't run their change at all than a reviewer who hasn't run it and misses some proble…

Letting the author run it could lead to "it ran on my pc" surprises.

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

#62

Post by tdstein » Sun, Mar 06, 2022, 5:37 PM UTC

Only if I don’t understand something and need to validate an assumption.

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

#63

Post by eyelidlessness » Sun, Mar 06, 2022, 5:39 PM UTC

It depends on:

- what’s in the change; if I’m reasonably sure I understand it, and that it won’t have side effects, and that it’s the right thing to do, I may skip running it

- how familiar I am with the area of the codebase; even if the above is satisfied, I’ll pull it and explore around the change to look for potential side effects I’ve missed, or other approaches that might be better for maintenance… and generally to improve my familiarity

- how confident I am in the other contributor’s (and reviewers’) thoroughness; I generally scrutinize even trivial changes if I don’t think others have, because surprising things sometimes reveal themselves

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

#64

Post by xtracto » Sun, Mar 06, 2022, 5:39 PM UTC

Where i work ehen a PR is created, a copy of the environment is spinned with the changes automatically by the CI peocess. Mainly so that QA can test the PR.

So people doing code review can poke at the resulting running program if they like. But generally code review is used for the "how" instead of the "does it work?"

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

#65

Post by gsej » Sun, Mar 06, 2022, 5:51 PM UTC

I used to pull the code and run it for each PR, as I believed it just wasn't possible to read code adequately. Then I worked with one guy who clearly could read code very well - he found quite subtle issues just by reading it, and having seen it was possible, I put in the effort and learned to do it too. So in my case, my reason for always pulling and running the code was, as you suggest, that I simply wasn't very good at reading code. I still won't hesitate to pull and run code if I need to, but I'm pretty suspicious of code I can't read these days, and usually request it be made clearer.

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

#66

Post by xen0 » Sun, Mar 06, 2022, 5:53 PM UTC

Most of the time I would expect the code to be already exercised by automated tests.

Sometimes, if it adds a new feature or something 'interesting', I've checked it out locally to see what the user-facing behaviour is, but this is very rare.

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

#67

Post by fuzzy2 » Sun, Mar 06, 2022, 5:53 PM UTC

Sometimes. It was worth it every time, too. "I thought this was working"–well, it wasn't.

Most of the time I pull the code to easily navigate it in context. The diff viewer will only get you so far.

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

#68

Post by rm_-rf_slash » Sun, Mar 06, 2022, 5:55 PM UTC

Almost always.

Better I find out about edge cases that weren’t considered or unit tested than our customers.

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

#69

Post by berkes » Sun, Mar 06, 2022, 5:55 PM UTC

Pooge wrote: post #21

No. That's the job of unit tests, integration tests and your CI/CD pipeline or alternatively the test environment (dev, staging, quality control, you name it...). Moreover, the person having written the code is supposed to have checked that it runs and meets business requirements - although the latter should be ultimately checked by your Product Owner or client.

I do test manually when the author does not practice TDD. (Or if there aren't tests.)

Additional benefit is that this shows to devs and management what a time saver TDD and testing in general is. It shows immediately that lacking tests cost valuable, senior, time, rather than save time by not writing tests.

It also is simply needed: when there are no tests, we'll need to manually verify and check for regressions.

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

#70

Post by sorobahn » Sun, Mar 06, 2022, 5:56 PM UTC

I think this depends on the scale and type of the project, but it's good to get in the habit of codifying tests and setting up CI ASAP. It's basically scaling you pulling down code and running it. Now with containers, there isn't much of a reason to not have automated build checks and testing. Even when you have changes in a distributed environment, things like docker-compose make it much easy to create mocked CI environments with dbs, caches, etc. Also, CI helps me prioritize PRs when I have many to review. Being able glance at github to see which ones are mergeable immediately vs which ones may have other dependencies causing CI to break is great.

If you're working on some legacy codebase and don't have these luxuries, I totally get running it locally first. I am lucky to work with people who I do trust deeply to not waste others' precious time by testing first so there is probably also a human element for me.