How to review a Pull Request: rules and checklist

It’s not rare to use Pull Requests in test teams nowadays. But to make the process of reviewing Pull Requests more enjoyable and expected, you need a set of rules or recommendations. So it’s a kind of a contract of communication between team members in area of Pull Requests.

I’d like to share the rules and checklists that we use on our team that help us be efficient in the pull request review process. These rules are highly depend on a team and a company, also I would emphasize that it’s not a law and you need to use these rules wisely.

Pull Requests workflow: General rules

* WIP: Work In Progress

1. Pull Request description should contain a Jira issue number (or a Jira issue url) if this Pull Request has a related Jira issue. So, everyone can go and find additional information in the Jira issue.
2. It’s better to add the Jira issue number to a branch name and a commit description.
3. If Pull Request is not ready for review, please, add the “WIP” title prefix to it.
4. If you develop some code and you want your teammates to watch it, please, create a Pull Request when your work is about 30-50% done and add the “WIP” title prefix to it.
5. If the Pull Request is ready for review make sure you removed the “WIP” title prefix.
6. Please, try to make Pull Requests not bigger than 300 changed lines (there might be exceptions).
7. Pull Requests should not live longer than 2-5 days and up to 2 days without any review (there might be exceptions).
8. There can be several rounds of review process:
   • 1st round: please, pay attention to the architecture, antipatterns, maintainability, scalability, “code smells” characteristics, proper naming of functions and variables.
   • 2nd round: pay attention to previous fixes and changes, as well as less important things (i.e. things with lower priority), e.g. comments.
   • 3rd round (if needed): spell checking, typos, wrong spaces etc.
9. If the Pull Request is over 90% good, it’s enough and it can be merged.
10. Direct pushes to master branch without PR are forbidden for any repositories with 2+ people involved in the project inside the given repo.

Merging Pull Requests

The Pull Request can be merged if the following criteria are met:

1. Approved from at least 2 members (might be exceptions)
2. All discussions were resolved (comment “Fixed” or conclusion between commenters)

While merging

1. Be sure that you checked the “squash commits” checkbox (if applicable – squash all fixups)
2. Mark “delete source branch” checkbox
3. Be sure that all pipelines passed without errors

Submitter rules

1. Before creating a Pull Request do a self-review of your code (Please check the Checklist for Submitter for additional information).
2. After creating the Pull Request add all members of your team and a member/members from another team if he is related to this Pull Request or you want him to join the reviewing of the Pull Request.
3. Assign the Pull Request to a teammate who will be responsible for merging.
4. Add understandable title to the Pull Request which covers all changes in this Pull Request (If there are multiple, then, maybe it’s better to create separate Pull Requests?)
5. Add detailed enough description to make life of reviewer easier or a Jira issue number if this Pull Request is related to the issue.
6. Provide all information related to this Pull Request:
   • Add all links to CI pipelines (to prove that it works fine).
   • Add any links related to this Pull Request to other Pull Requests.
   • Add any other necessary and/or related links.
7. Write a notification in case this Pull Request should be merged together with another Pull Request.
8. Check “squash commits” checkbox
9. Urge your teammates to review your Pull Request 🙂

Reviewer rules

1. When writing a comment, if it’s possible, add links to additional information (blog articles, stackoverflow, documentation, etc).
2. If you are mentoring someone, please, pull the branch on your machine and do a detail review.
3. Use Checklist for Reviewer during the review process.
4. Suggest a code changes if needed.
5. Don’t put reports of scanners – you still need a manual review of scanner’s results. If you want to use a code scanner, then don’t push it onto a submitter but review it by yourself instead or include it as a required step on submitting a Pull Request.
6. Use your head and don’t follow the rules blindly 🙂

Conclusion

How do you perform the Pull Request workflow? If you have your own rules in your team, please, share them with me!