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

paradite · 4 years ago

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 · 4 years ago

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.

bradwood · 4 years ago

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.

stephen_cagle · 4 years ago

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?

ryan_lane · 4 years ago

Isn't this what automated tests are for? Manually testing every change is a huge burden, requires most of the automation necessary for automated testing (from the infrastructure point of view), and actually discourages people from writing automated tests.

paradite · 4 years ago

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

mnutt · 4 years ago

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

paradite · 4 years ago

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

dinkleberg · 4 years ago

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

slaymaker1907 · 4 years ago

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.

tomatohs · 4 years ago

I'm working on an "instant replay for developers" desktop app for exactly this case. Create desktop instant-replays to embed in pull requests:

http://replayable.io/

bobobob420 · 4 years ago

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.

paradite · 4 years ago

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.

flippinburgers · 4 years ago

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

Folcon · 4 years ago

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

paradite · 4 years ago

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

Loading comment...

lazypenguin · 4 years ago

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

tharne · 4 years ago

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 · 4 years ago

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.

Loading comment...

azornathogron · 4 years ago

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.

Loading comment...

maximus-decimus · 4 years ago

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.

Loading comment...

bobobob420 · 4 years ago

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 · 4 years ago

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 · 4 years ago

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

Loading comment...

Pooge · 4 years ago

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 · 4 years ago

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 · 4 years ago

makes me sad to see this so far down.

ricardolopes · 4 years ago

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 · 4 years ago

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

Loading comment...

xen0 · 4 years ago

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 · 4 years ago

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.

Loading comment...

imetatroll · 4 years ago

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 · 4 years ago

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 · 4 years ago

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