Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

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

news.ycombinator.com

Post reply on HN
Print view View original

111–120 of 146 posts

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

#111

Post by nameisname »

koyote wrote: post #110

I only do it if I suspect that the code will not work. Sometimes I can think of edge cases that I suspect are not covered; so I check out the code, add the edge case as a unit test, and if it fails I mention it in the code review for the other dev to add and fix. Other than that, no. I am reviewing the coding standard, architecture, and algorithms used. It's up to the dev to ensure that the code works and runs. The C…

Wouldn't it be better to ask in the MR if they could just write a test to cover that case? It prevents wasting your time writing a test case that's going to be thrown out and prevents the other engineer from being blind sided by someone else committing a test case to their branch. I'm really not sold that the approach you're taking is of more value than just asking the MR author to add the test themselves.

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

#112

Post by muzani »

maximus-decimus wrote: post #12
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?

To be fair, it's more like "what would you think a mechanic who doesn't verify that the mechanic who fixed the car didn't check it ran." If you can't trust the person who created the PR to have tried to compile the code... What are they even doing.

In most of the situations where the code doesn't run, the person creating the PR didn't commit the code needed to make it run, but have it offline. I've bitched at someone who approved a PR in about about 10 minutes, but there was a merge conflict where the ">>>>>>>" would have been apparent for anyone who actually even glanced at the code.

I'd say much of the value of a code review on senior devs is making sure that it's just all properly committed and nothing breaking.

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

#113

Post by rapfaria »

Every person saying "no" is probably part of a full, competent team, that has automated tests for everything, QA folks - that truly understant what they are doing and what are the business requirements, etc, etc.

If you are missing stuff like this, than yes. I'm surprised at the amount of time developers deliver something that doesn't even boot

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

#115

Post by bobobob420 »

paradite wrote: post #25

I'm a senior frontend engineer doing a lot of code reviews and my team loves my MR (PR) submission standard: - Require the author to attach screenshots of the feature in a markdown table format showing the before and after comparison. This is enforced using a MR template. - Require the author to attach screen recording for complex user interactions. Overall effect is that it saves everyone's time. - The author has to…

The fact that people think this is an excellent idea.....screen recording the proof that the feature works wtf what is the alternative you are getting? People are submitting features that don't work? Just fire them or teach them your set of standards. If my boss asked me to do this I would laugh and find a new job. I truly feel sorry for you but also please stop ruining our industry with this nonsense.

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

#116

Post by bobobob420 »

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?

Just make the developer responsible for code they write. It is not hard or complex. You shouln't have to build code from every branch, compile, run, and test. That is not what a code review is...

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

#117

Post by bobobob420 »

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.

makes me sad to see this so far down.

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

#118

Post by manibatra »

Yes we started doing this around 6 months ago. We started using [Github PR templates](https://docs.github.com/en/communities/using-templates-to-en...) to prompt for instructions on how to run and test the PR. We are seeing following benefits :

- We are finding deficiencies in our testing/development methods. The more people have to test/run code the more we are able to find bottlenecks and eliminate them.

- Knowledge sharing. We found that team members had knowledge/tricks in their heads which are being shared more.

- The overall reliability of our systems(measured via SLOs) has greatly increased.

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

#119

Post by ryan_lane »

BossingAround wrote: post #89
paradite wrote: post #25

I'm a senior frontend engineer doing a lot of code reviews and my team loves my MR (PR) submission standard: - Require the author to attach screenshots of the feature in a markdown table format showing the before and after comparison. This is enforced using a MR template. - Require the author to attach screen recording for complex user interactions. Overall effect is that it saves everyone's time. - The author has to…

But you still have to test the actual functionality, don't you? What if the author makes a gif of clicking a button, but doesn't record if the browser back button still works? I'd think that for frontend, you'd have CI/CD pipeline that deploys the code into a staging server, where I can test it myself.

Isn't this what automated tests are for? Manually testing every change is a huge burden, requires most of the automation necessary for automated testing (from the infrastructure point of view), and actually discourages people from writing automated tests.

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

#120

Post by ryan_lane »

bri3d wrote: post #85
xen0 wrote: post #66

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.

Automated tests are very poor at capturing the nuance of user interaction, and I find that they frequently are not exhaustive or watching the video shows only a "happy path" and doesn't expose functional deficiencies in a feature. They show that a feature works, but not that it works correctly or especially not that it works _well_. For straightforward regressions and minor tweaks I am usually satisfied to see a vide…

You're describing what QA's function normally is.
Post reply on HN