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

81–90 of 146 posts

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

#81

Post by striking »

wodenokoto wrote: post #34
c7DJTLrn wrote: post #32

No. 1. PRs should target development, which is an *unstable* branch, so no biggie if something breaks 2. CI should catch things like syntax errors, failing tests, or poor test coverage 3. I trust my colleagues to do their work properly and fix things if they've made a mistake

I don’t get your first point. If the pull request doesn’t run: - how can we even say it works? - who’s responsibility is it to fix it? - and when does it get fixed?

In order:

* Tests

* The engineer making the PR

* Before they're allowed to merge

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

#82

Post by mnutt »

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

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

#83

Post by Folcon »

x86a wrote: post #75
Folcon wrote: post #58

Earlier quoted context omitted.

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 =)...

"MR" so probably Gitlab

Ah didn't know that, less familiar with gitlab.

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

#84

Post by mjaniczek »

Mostly we depend on the CI actions (running automated tests and linters), but every PR also creates a feature branch on the testing/staging environments so anybody can run the app _as of that branch_ online. The URL schema is kind of like

https://app-staging.foo.com/feature/add-sorting/

compared to the original

https://app-staging.foo.com/

given you have created a PR pointing from branch named feature/add-sorting to master.

This also makes it easier for our QA engineers to test things before they get merged to master.

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

#85

Post by bri3d »

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 video and test automation, but for any kind of new functionality I strongly advocate pulling the code and actually playing with it.

Depending on your company structure, product managers can help with this functional review as well, although this requires working tooling and org structure in a way that doesn't exist at some companies.

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

#86

Post by nicoburns »

codecurve wrote: post #22
nicoburns wrote: post #17

I wish there was a better workflow for this. We have code across ~10 repos at work, and I'm usually working on something else. So checking out somebody's code for pull requests is pretty disruptive. We generally consider "that the code runs" not to be part of code review, and have a separate testing phase before codes goes into production. Which works, but it definitely could be a smoother process.

Tried git worktrees? https://git-scm.com/docs/git-worktree

I have played with them before, but I haven't properly tried them for this. I can see it solves half the problem (being able to open something twice). But there's still a lot left to be solved:

1. I need to check out ~8 branches

2. I then need to reinstall all dependencies for those branches.

3. I then need to run those branches locally on a different set of ports to my standard dev environment to avoid clashes (or shutdown my main one temporarily)

4. If it involves the mobile app I may need to wait some time for a clean build

It's a lot of administrative overhead compared to what I tend to default to in most cases which is just merge the branch after reading through it and then test in our staging environment once CI has run.

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

#89

Post by BossingAround »

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.

Post reply on HN