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

91–100 of 146 posts

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

#91

Post by newaccount74 »

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 had a coworker who seemed to just open PRs as a part of the development workflow. As soon as the feature kind of works, they open a PR, wait for someone to complain about problems, fix them, ask for another review, get more complaints about bugs, rinse and repeat. If the reviewer didn't pay attention, they would always merge broken code.

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

#92

Post by ChrisLTD »

alkonaut wrote: post #45

Only very rarely. Because we have a dedicated QA team (normally an antipattern but for complex enough software you some times end up building software you literally can’t run which is - yes - weird and difficult).

As long as you’re not using your QA team as an excuse to avoid writing automated tests, I don’t see why having a QA team would be an anti-pattern.

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

#95

Post by stephen_cagle »

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.

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?

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

#96

Post by heygarrison »

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?

Vercel and Netlify do PR reviews these days. Was it in the JS space?

I think render.com is going to I introduce this soon too.

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

#97

Post by svilen_dobrev »

once in a while, a new feature (or fix) can mess up with another new feature (or fix). And running those branches separately would be ok by themselves. But when both merge, then stuff goes wrong.

So it might be useful to run the thing before AND after merging, but that is very very tedious..

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

#98

Post by antifa »

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…

I'd expect tests to have assertions that all the expected 401/403/404/422 responses actually happen and have the correct response messages.

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

#99

Post by klohto »

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?

There is already Vercel, Netlify, etc. that support this feature.

You can also easily do your own version with Cloudfront and S3 or just a custom nginx config.

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

#100

Post by quixoticaxolotl »

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…

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 about.

Post reply on HN