Ben Halpern

Posted on Jan 16, 2019

What's your typical process for reviewing a pull request in GitHub?

#discuss #github #opensource #productivity

I'm never sure whether I'm using this tool as effectively and efficiently as possible.

Comment describing your workflow so we can share tips and get to know our unknown unknowns.

Top comments (27)

David J Eddy • Jan 16 '19

When I did code reviews, and for any team I lead from a technical aspect for, my a review has three phases of confirmation:

Automatic
- Did the testing pass? If not, stop.
- Did the change add/remove tests? Why?
- Did the code quality decrease measurably?
- Did the change introduce possible future technical debt? Is it acceptable?
- Did any of the tooling indicate a decrease in maintainability or compliance?
Operation
- Does the change satisfy the statement of work submitted for the change?
- Does the change logic look reasonable?
- Can I follow the logic change in text, mentally, and not get lost.
Experience
- From the end/admin/manager/etc users perspective does the change function as expected?

Sure it seems like a lot, and yes it may slow down the merge process a bit. But in my experience taking a little longer to review code can help prevent issues from getting to production.

The holy grain though is a 100% automated process with confidence in the automation that when a change begins throwing errors the changes are rolled back, the errors logged, and the committing developer alerted. But that is a story for another time.

Joshua Mervine • Apr 21 '19 • Edited

Love this list!

A comment on the first point...

I don’t always stop if the tests aren’t passing , especially when reviewing less senior level engineers PRs. I’ve found that in a collaborative environment, a detailed review in spite of test failures often shows an obvious mistake that can quickly be pointed out, increasing the overall velocity of the initiative.

One possible addition...

Reviewing the test additions or updates first can often guide the review nicely, framing the changes in an easy to follow and logical way. Plus, if reviewing someone for the first time, it helps clarify the patterns and structures they use.

Ben Halpern • Apr 21 '19

Awesome

Mateusz Bełczowski • Jan 17 '19

Wow, I need to write that down. Great list

Arthur Vincent Simon • Mar 13 '20

For frontend development, there seems to be an additional step that is needed.

Pull Requests should be accompanied by UI reviews. Because you can only see so much in code.

pgangwani • Jan 17 '19

Adding to this I ask to use screencastify plugin and create gif to compare experience/features before and after changes.

Victor Darkes • Oct 5 '19

Really thorough answer. I agree with it all!

Ben Halpern • Jan 16 '19

Great looking checklist!

Isaac Lyman • Jan 16 '19 • Edited

Generally I'm sanity checking. Does this look like code? Are there any obvious typos? Any residual debugging statements? Anything that feels like a mistake?

If I want to get more involved, I'll pull the branch and test it. But sometimes it's better to get the code merged and fix bugs later, in smaller PRs.

The longer a pull request is, the less likely the reviewer will catch anything of importance.

Juan F Gonzalez • Jan 17 '19

It started as nice javascript in the 1st picture and in the 2nd is just a clusterfuck of symbols and letters hahaha. I agree with the smaller PRs, easier to catch stuff there.

Piotr Szymaszek • Jan 18 '19

I would advise against merging code with bugs, which can end up in production pretty easily if you are not careful.

Isaac Lyman • Jan 18 '19

That's a great point, and to clarify, I wouldn't usually approve a PR with bugs. But I think there are cases where it's acceptable:

The only bugs are design inconsistencies or unclear requirements (in larger organizations, these can take weeks to resolve).
We're doing new feature development, the PR is a major net improvement over what we had before, and the bugs are minor and part of the new feature (not regressions).
The team agrees that the PR is large enough and affects enough things that we should merge, deploy it to the company's internal server, and get a lot of eyes on it as quickly as possible. This is an intentional delay of all other work until the code in question is thoroughly tested.

Every day that a PR sits unresolved is technical debt; it gets further and further out of sync with the master branch, the original author starts to forget what they did, and all the people that could have been using and testing the new code, aren't. So the risk of letting bugs into production isn't the only risk in this situation, nor, I submit, is it always the greatest risk.

Ali Spittel • Jan 16 '19

When I was teaching, students would submit their homework/labs as pull requests, which leads to reviewing a couple hundred a week. My quickest tip is the tiny command line instructions link on GitHub saves a ton of time.

I would copy and paste the Step 1 changes into the CLI and then test the code on my computer. Then, I would normally do inline comments using the plus button that pops up when you hover over a line of code in the files changed tab with things I would change or things that were awesome!

Ben Halpern • Jan 16 '19

Yeah I do that too. This morning (prompting this discussion), I had the feeling that maybe I want to do this via GitHub Desktop because it just seems like the more flowy way to do it. I don't like that app much for most of my workflow, but in terms of quickly going from web to review it could be better.

Jeremy Forsythe • Oct 29 '19

I'd add that you can now make multi-link comments (comments on multiple lines of code) which makes it much easier to provide context

Molly Struve (she/her) • Jan 17 '19 • Edited

Flow for when I am familiar with the code

The Why - Understanding the the reason for the changes. Was this a bug? Was this a feature request?
a. Bug - Does this change fix the bug? Does it introduce any other possible bugs?
b. Feature Request - Look at the corresponding JIRA ticket to ensure that all parts of the feature request are complete.
Logic - Dig into the logic and ensure that it all works like it should. Unless the logic is extremely complex, I usually will not pull a branch down and test it myself. We have a pretty comprehensive test suite and if there are good tests written for the new code its usually unnecessary to test manually unless its a big feature which will go through Q/A.
Performance - Are there any unnecessary MySQL queries? Are indexes in place where they should be? How will this code perform when executed a million times? How will this code affect our databases?
Edge Cases - Since becoming a Site Reliability Engineer, I feel like a lot of my work deals with edge cases. Code works 99% of the time, but if given this scenario, it breaks. I like to take a step back and look at the big picture for PR. How will this interact with other moving pieces of the code?
Code style and cleanliness - Are there bits that could be refactored to be more readable or more reusable? Are any variables not quite clear based on their naming?

When the PR deals with code I am unfamiliar with

ASK QUESTIONS! - I cannot tell you how valuable it can be when someone asks you questions about a PR. For a while, at my job I was the resident export at Elasticsearch. Because of that people, who shy away from reviewing my PRs bc they went beyond their own Elasticsearch knowledge. Since I wanted my code reviewed I started to tell people to just ask me questions. Have me explain the parts you dont get. By explaining how things worked, half of the time I would find the bugs myself. Plus, if you ask questions then you become more familiar with the code yourself.

Adam Parkin • Jan 17 '19

+1 to "Ask Questions". Not only from the perspective of giving the author a chance to explain why they chose the path they took, but also from the perspective of a reviewer trying to learn, as well as a mechanism for prompting discussion.

I've learned a ton asking questions in PR (ex: "oh, so you can define a nested class in this language?", etc), but I've also used it with great effect to guide a more junior dev to realize ways they could improve their code and understand the "why" of one approach to solving a problem being better than another (ex: "so if we had to change this value here, what else would have to change?" or "if this value was null here, what would happen in the subsequent lines that follow?", etc) Ie "ask don't tell" can be an effective review technique for avoiding putting the author on the defensive.

Molly Struve (she/her) • Jan 17 '19

I actually do that a lot when I am teaching in person but I never thought about doing it on a PR! Good call!

Rinky Goyal @GraphQLConf • Jul 13 '19

Huge +1 about asking clarifying questions. You get to learn something and the person who raised the PR also ends up with more clarity. Sometimes a better understanding and sometimes better solution - win-win!!

Dave Nguyen • Jun 24 '19

For long pull requests, I use a practice that I call "micro/macro reviewing"

Stage 1: Micro reviewing

Find code that doesn't follow best practice (which formatters and linters don't catch). Make direct suggestions using Github web tool
Find good code sections / good tests then publicly thank / praise the author

Stage 2: Macro reviewing

Understand what the issue is about
Read every changed files, ask myself if the change solves the issue or not, how it solves the issue. Write down any questions that I have. For files that I have no questions and be independent from other files, I fold it.
Re-read every unfolded files to find answers for the noted questions. Make a comment for anything that I can't find an answer or things that don't seem right.
Beg for tests for non trivial changes.
Find more good code sections / good tests then publicly thank / praise the author

I hardly run the code myself. Mostly because I'm lazy, but I trust code authors and our test suite.

Gift Egwuenu • Jan 16 '19

My day to day job requires PR to be reviewed by my teammates and typically what the procedure is like is

Look through the PR file changes and go ahead and pull the branch to test the feature is working correctly.
Also make use of GitHub's tool for suggesting changes and adding comments to individual lines of code

Mateus Oliveira • Jan 17 '19

0 Should pass in the test (all) to open a pull request
1 Pull the code. For me is not easy to understand the changes on a pull request
2 I can reviewing in different ways it depends if is a senior or a junior developer if is a junior the reviewing process is in pair.
3 Look through duplication code, performance and the most important details
4 Reviewing process is not a feature/bug test remind the developer of this

Devashish Datt Mamgain • Sep 1 '20

We use specific PR template which includes points about how was the code tested, new learnings, etc

Sample here github.com/Kommunicate-io/Kommunic...

Piotr Szymaszek • Jan 18 '19

I am not going to repeat what others have already said, but I will just add that it is sometimes important to ask the author to explain what he wanted to do and why. Just because the code says it does something and it is properly executed, it does not mean it was what suppose to be done. Make sure they actually understand what the task they are trying to do is about.

Rinky Goyal @GraphQLConf • Jul 13 '19

Adding to the awesome list -

I would expand the file a bit to understand the context better.
would add labels whether my comment is a minor/major - that is blocking or non-blocking
use Squash & merge :)

Justin Lam • Jan 17 '19

Are you asking what we should be looking for while code reviewing or how to use the tool to efficiently review the code? I think are lots of examples on dev.to on code review checklists.