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

121–130 of 146 posts

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

#121

Post by actuallyalys »

While I think the author has the most responsibility to test changes, it's too easy to commit only some of the changes or otherwise make a change that's dependent on your development environment. Running tests and building code on CI prevents some, but not all of these mistakes.

Risk also needs to be taken into account. It's not necessary to spin up the application locally for tweaking error message wording or a small CSS change with no real responsiveness or accessibility implications. But for most fixes and features, I think at least a quick check is worth it. And if you're mentoring an intern or very junior developer, you should probably check every change, at least for the first few months.

Finally, the rest of your testing makes a big difference. If you have a formal QAer who tests your application before release or a community of people running nightlies, it's less urgent.

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

#122

Post by paradite »

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.

It's interesting that you brought up the issue of back button. It is indeed an area where bugs frequently occur.

I haven't found a good solution except manually asking in every MR that I sense a potential issue. Maybe it is a good idea to have it in MR template as a checklist item.

Another problem with back button is that the expected behaviour is usually missing in the design or requirements to begin with, requiring a lot of back-and-forth on what's actually expected (especially for SPA).

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

#123

Post by paradite »

mnutt wrote: post #82
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…

You can even automate this with something like Percy that takes screenshots and does visual diffs at selected points in the test suite. You just have to be careful about including random/date-based data in your tests or you’ll get more diffs than expected. Also not quite a substitute for someone calling out “this was a major UX change”.

Interesting to hear that there are automation solutions around this. In your experience / expertise, how much work can be automated by such tools? 80%? 99%?

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

#124

Post by paradite »

Folcon wrote: post #58
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…

This is great, do you do this in github? Or elsewhere? Just curious if you had a template to hand that myself and others could use =)...

The PR/MR template itself is pretty generic. You can make one yourself quickly by referring to GitHub markdown table documentation: https://docs.github.com/en/get-started/writing-on-github/wor...

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

#125

Post by monster2control »

I almost never run the code locally unless I see something questionable and can’t reason about what it will do because I’m unfamiliar with something in it. For that, I tend to use a REPL for just the snippet of questionable code.

However, if the code is a large enough change, and it needs testing anyways, I will run it.

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

#126

Post by paradite »

bobobob420 wrote: post #115
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.

Requiring screen recording isn't about lack of trust or lack of competence. Rather it's a way of eliminating information asymmetry between the author and the reviewer:

- The author might have checked that the code works fine (A) or haven't done it (B).

- The reviewer might decide to run the code locally (1) or not (2).

Combination A1 results in duplicate effort. Combinations A2 and B1 are perfect. Combination B2 results in potential bugs.

The MR standard merely eliminating the combinatorial possibilities by sharing information between the author and the reviewer automatically. The end result is that both parties know A2 is the process that the other person follows.

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

#128

Post by crnkofe »

I tend not to run stuff locally due to the complexity of setting up whatever is being tested. This goes especially for bug fixes and 3rd party integrations.

I do however check out code locally. Online code review only shows what's been changed and I often find subtle issues in the code that wasn't changed. In short I'm trying to ask myself: What's missing in implementation?

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

#129

Post by flippinburgers »

bobobob420 wrote: post #115
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.

Consider yourself lucky to be on a team where people are competent at testing the features they create.

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

#130

Post by bradwood »

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.

Code review != Testing.

A lot of people conflate these IMHO. Code review should not really be about whether it "works" or not. That's what tests are for.

Code reviews are about checking for code complexity, good use of abstractions, readability, etc.

Post reply on HN