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

[gary_chambers]

I've been having a debate about whether code should be checked out and run locally (or on a staging environment) as part of code review. Personally, I find it easier to review a change after I've run it locally, but perhaps that's because I'm not very good at reading code.

I'm interested to hear what other people think. Do you run code as part of code review?

[paradite]

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 verify that the feature works anyway, so taking screenshots / recordings doesn't take much addtional efforts.

- The reviewer can focus on code itself instead of checking whether the code works.

- Easy for both parties to spot design and UX issues/regressions visually.

[BossingAround]

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.

[mnutt]

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

[dinkleberg]

This is an excellent idea for a submission standard, will have to store this in the back of my mind.

[slaymaker1907]

Word is actually really great for this stuff. You get a portable file and can add notes to the screenshots. There is even a screenshot button built into Word!

For recordings, if you use Windows you can bring up the game bar with Win+G. It has a very easy to use screen capture tool meant for gaming, but perfectly fit for small software demos. It even captures sound which is sometimes useful.

[bobobob420]

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.

[Folcon]

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

[lazypenguin]

I’ve found this works extremely well for visual changes and can’t believe when it’s not the default behavior!

[tharne]

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?

[city41]

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 mistake causes them to scrutinize the change more than if it had been reviewed. Not always of course, depends on the context and people involved.

[azornathogron]

Making the code correct is a shared responsibility between reviewer and author; if the code has a bug or doesn't even run that means both people missed the problem. Unless the author is very new/junior (still at a stage where they need close guidance on individual changes) then I would be more annoyed or concerned by an author who hasn't run their change at all than a reviewer who hasn't run it and misses some problem. But I guess it depends on the conventions of the organisation exactly what the balance of responsibility is.

As a reviewer (in the places I've worked so far) for me it depends what type of change it is. E.g., for a frontend change I might need to run it to be able to actually try it out and check what it looks like, whereas for a backend change I might rely more on reviewing the tests and if the tests look good then I probably wouldn't bother running locally to poke at the thing myself. Of course there are also just general issues of how big and how complicated the change is.

Anyway, wherever possible mechanical stuff like "does it build and run" should be covered by a CI system or other automation, not by spending reviewer time on it.

[maximus-decimus]

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.

[bobobob420]

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

[bluGill]

My job isn't to make your code work, it is to find those little things that don't seem to be bad that are. I have tools for obvious things.

Which is to say 'it doesn't work' is nitpicking that reviewers shouldn't be wasting their time checking.

[verdverm]

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

[Pooge]

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.

[berkes]

I do test manually when the author does not practice TDD. (Or if there aren't tests.)

Additional benefit is that this shows to devs and management what a time saver TDD and testing in general is. It shows immediately that lacking tests cost valuable, senior, time, rather than save time by not writing tests.

It also is simply needed: when there are no tests, we'll need to manually verify and check for regressions.

[bobobob420]

makes me sad to see this so far down.

[ricardolopes]

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 code they're pushing, and is expected to do all the tests needed. Reviews are more about ensuring code standards, finding possible oversights, requirements mismatches, and so on.

No one strategy is right or wrong, as long as expectations are set and everyone knows their responsibilities and expectations.

[tester756]

>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?

[xen0]

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.

[bri3d]

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.

[imetatroll]

If you want to approach the highest quality code your team can manage, you want:

- the author to test and have a succinct PR description that includes before and after screenshots/video as appropriate

- the reviewer to test the code locally

The reviewer test is particularly important for front-end code which is where automated tests are more likely to be poorly written or absent.

It goes without saying that there should be targeted automated tests, but I find that in practice the front-end is where this tends to fall short.

[alexdowad]

Historically, I have rarely tried running code under review, but I have a coworker who routinely does this and by doing so, he has caught my mistakes a couple of times.

I should probably learn from him.

[eyelidlessness]

It depends on:

- what’s in the change; if I’m reasonably sure I understand it, and that it won’t have side effects, and that it’s the right thing to do, I may skip running it

- how familiar I am with the area of the codebase; even if the above is satisfied, I’ll pull it and explore around the change to look for potential side effects I’ve missed, or other approaches that might be better for maintenance… and generally to improve my familiarity

- how confident I am in the other contributor’s (and reviewers’) thoroughness; I generally scrutinize even trivial changes if I don’t think others have, because surprising things sometimes reveal themselves

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

[auslegung]

So far everyone is saying, “it depends”, and that’s my experience as well. Depending on the complexity of the change, how impactful it will be in production, what the test suite is like, etc. I may or may not pull and run it.

[simplyinfinity]

For me personally, code review is the place where you take a look at the code structure, architecture and if it is using the correct ways to do things. It's not the place to check if business requirements are met.

Code compilation (and checks) & tests must be requirement as part of each PR

[rvieira]

Depends on the context and scope.

If it's something simple I can grok by reading the code and tests I usually don't.

If it's a complex piece of code, I find it valuable to run it locally, to setup some breakpoints, debug it and step it to understand it better.

[dec0dedab0de]

We have a server automatically deployed for every branch, so most of the time if I want to see it running I can just check out that server.

Though sometimes I'll check out their branch, open up a repl and run a few functions or something on my own.

[tootie]

We have a CICD pipeline for every branch. We use squash.io which spins up a VM and runs the docker compose to create a per-branch QA env. The process however is that a developer does a code review on just the code first, then a tester tests the feature branch, then we merge and do some regression testing (manual and auto), then release.

[nine_k]

Usually I don't, because the CI bot does it. I do read the code thoroughly though.

Only if something really draws my attention do I pull and run a particular PR (CR, CL, whatever yo call it). Once I did that to illustrate that the change introduces a security issue, so I had to write an exploit against it, and demonstrate how it works. (It helped.)

[wodenokoto]

I agree with this take. Generally it’s the tests job to run it, but test coverage is not 100%, so “sometimes” is a really good answer here.

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

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

[nameisname]

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.

[bradlys]

Varies by company/project/team.

In most of my companies - no. The reason? Not practical for time constraints.

To be honest - this is mostly how you find out if a company values quality or speed. Most I’ve been at valued speed more than anything.

[alkonaut]

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

[ChrisLTD]

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.

[bombcar]

For us the developer is assumed to have run it, the CI checks that it builds and loads, code review is more for the decisions and any gotchas.

Bugs introduced will be caught and fixed in general testing usually.