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

101–110 of 146 posts

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

#101

Post by verdverm »

icedchai wrote: post #59
verdverm wrote: post #54

Earlier quoted context omitted.

Or have a machine run the code, like a CI system?

Often, it is not that simple. Many code bases are completely lacking any integration or end-to-end tests, despite having tons of unit tests. Unit tests can pass, but the system may still "not work" due to failures to call the new code in the correct place. The original developer can miss this and it may not be obvious from the context of the PR that something not already in the PR needs to also change. "You don't kno…

It seems to me it would be worth the effort to get CI in place over having reviewers manually run the same thing. The labor is more expensive than the computers

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

#102

Post by julienfr112 »

stephen_cagle wrote: post #95
BossingAround wrote: post #89

Earlier quoted context omitted.

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.

I remember a few companies ago we had a system that would deploy every branch to a separate subdomain (same name as the PR) that you could access. It was fantastically useful for just seeing what the deployed code would look like. I think (for UI things at least) this is a very reasonable solution. Wish I could remember the github service that did this?

the neame is review app, it's on heroku since forever, also in gitlab ops today.

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

#103

Post by toast0 »

tester756 wrote: post #35
ricardolopes wrote: post #27

This should be something agreed within a team, so that review standards are consistent across team members. In my previous team, the reviewer was the main responsible for the code they were approving. They were expected to test locally and should actively hunt for potential issues, such as checking in the logs that the ORM was building correct SQL. In my current team, the developer is the main responsible for the cod…

>This should be something agreed within a team, so that review standards are consistent across team members. Why? it feels like individual preference >such as checking in the logs that the ORM was building correct SQL. Couldnt the person who creates PR copy/paste sample generated SQLs into PR?

A team standard helps contribute to clear responsibility.

If correct SQL is a priority (which seems reasonable), but it's not clear who should check it, it's likely to not be checked at least some of the time. Same with whatever other things have difuse responsibility.

There's still some room for individual preference. If it's author's repsonsibility to do X, they can ask for the reviewer to do it in the review request; or if it's the reviewer's responsibility, they can approve but say they didn't do X and are relying on the author to do it; or the author can assert they've done X in the request (perhaps it's difficult to do for this speciric request) and the reviewer can note they've relied on that assertion. But having a clear expectation strongly reduces the cases where author was relying on reviewer to check X and the reviewer was relying on the author, and X wasn't checked and the check would have found an issue before production.

Some things are a much bigger problem when found after production, and some things aren't; diffuse responsibility is ok for things that aren't a big deal, IMHO.

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

#104

Post by ricardolopes »

tester756 wrote: post #35
ricardolopes wrote: post #27

This should be something agreed within a team, so that review standards are consistent across team members. In my previous team, the reviewer was the main responsible for the code they were approving. They were expected to test locally and should actively hunt for potential issues, such as checking in the logs that the ORM was building correct SQL. In my current team, the developer is the main responsible for the cod…

>This should be something agreed within a team, so that review standards are consistent across team members. Why? it feels like individual preference >such as checking in the logs that the ORM was building correct SQL. Couldnt the person who creates PR copy/paste sample generated SQLs into PR?

As someone said in another reply, setting these team expectations is important so that the developer won't assume the reviewer will do the heavy testing, and the reviewer assumes the developer must have done it, leading to preventable production outages.

The developer can definitely do work like providing samples of generated SQL, UI screenshots and so on. Again, knowing who will do that deep due diligence is more important than the actual decision.

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

#105

Post by hatchnyc »

Hell no. Also if this is your expectation you’re absolutely insane if this isn’t automated…also this kind of sounds like QA to me. My presumption is the base functionality requirements are met, I’m looking for potential performance issues, code standards, sound architecture, etc.

I am always amazed at how many repetitive tasks folks want to load up developers with. I think there’s a tendency among developers-turned-managers to see their job as crafting the perfect “program” of processes for their developers to follow, instead just automating these tasks. Like they say, the cobbler’s children have no shoes.

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

#106

Post by peterhunt »

city41 wrote: post #29
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?

I sometimes find code reviews can create a "gap" in the center where neither person fully vets the change. The reviewer thinks "surely the author fully tested it, I'm just added assurance." And the author thinks "they approved the change so it must be good" and the end result can be code that is less reviewed overall. If someone pushes a change without anyone reviewing it, I sometimes find their fear of making a mist…

I think the fix for this is for every pull request to include a written test plan. Usually this is just a few bullets (and often is just "ran the test suite" if it's applicable). It's on the author to come up with a test plan and execute it. It's on the reviewer to review both the code and the test plan and ensure that the test plan is sufficient.

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

#107

Post by icedchai »

verdverm wrote: post #101
icedchai wrote: post #59

Earlier quoted context omitted.

Often, it is not that simple. Many code bases are completely lacking any integration or end-to-end tests, despite having tons of unit tests. Unit tests can pass, but the system may still "not work" due to failures to call the new code in the correct place. The original developer can miss this and it may not be obvious from the context of the PR that something not already in the PR needs to also change. "You don't kno…

It seems to me it would be worth the effort to get CI in place over having reviewers manually run the same thing. The labor is more expensive than the computers

Yes. Ideally we would write integration and e2e tests, and run them during CI. I personally favor those sorts of tests but they require a lot of effort.

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

#108

Post by bri3d »

quixoticaxolotl wrote: post #100
bri3d wrote: post #85

Earlier quoted context omitted.

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…

It sounds like what you want is a beta testing phase once a feature is released, not a per-PR play phase. I've had companies host release parties before, where everybody downloads the product and plays with it to identify glaring issues. It's a decent approach, but you'd get way better results having a dedicated testing team whose job is to test the software from a UX/UI/contract functionality if that's what you care…

I've used the release party approach, and it works, at a high cost. I've also worked with really good QA teams. The best ones don't really test contracts, they test the contract itself - that is, instead of testing that the UX behavior matches the "expectation", they pressure test that the "expectation" matches what a real user might do.

But even in these places, I still find value in having a second set of eyes pull down a PR and see if it makes sense. With good tooling it should only take a minute or two (and without good tooling, investing in this tooling usually helps a ton of other places too). This saves time in the ultra-expensive "30 sets of eyes pull down the product and use it" party phase, and frees up QA to be even more effective at pursuing edge cases.

Plus, again depending on the company, often fixes forward from mainline are kind of expensive, especially if you work in B2B / Enterprise where change management often works on a cadence. Fixing the feature on a branch can be a lot cheaper and easier than fixing it once it's in the wild.

Anyway, it all depends on the team and their process, and I'm far from a zealot about much of anything, but I do really encourage pulling down PRs and clicking through them as a matter of course, regardless of what additional mechanisms are in place.

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

#110

Post by koyote »

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 CI builds as well as the QA testers will test the actual functionality works as expected (obviously if I can't find the code that is supposed to fix/implement the PR item, I will question it).

Post reply on HN