Ask HN: Do you pull and run code as part of code review?
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?
145 comments
[ 6.1 ms ] story [ 202 ms ] threadcurrently at a place with extremely large code base and with dedicated testers, I don't run the code.
Sometimes even with dedicated testers etc. I will run the code if I think something might be problematic and I want to double check. I tend to only comment on code though if I think it could really be improved, if you have named a variable something I wouldn't I probably won't care.
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?
If you can't trust the person who created the PR to have tried to compile the code... What are they even doing.
I'd say much of the value of a code review on senior devs is making sure that it's just all properly committed and nothing breaking.
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.
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.
Which is to say 'it doesn't work' is nitpicking that reviewers shouldn't be wasting their time checking.
Though sometimes I'll check out their branch, open up a repl and run a few functions or something on my own.
Code compilation (and checks) & tests must be requirement as part of each PR
Bugs introduced will be caught and fixed in general testing usually.
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.
I should probably learn from him.
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.
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.
[1] https://github.com/magit/forge
- 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.
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.
Wish I could remember the github service that did this?
I think render.com is going to I introduce this soon too.
You can also easily do your own version with Cloudfront and S3 or just a custom nginx config.
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).
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.
- 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.
I have failed to create a proper software development process.
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.
http://replayable.io/
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.
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?
To your second point, sure, if that's what the team agrees is better.
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.
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.
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.)