Couple of years ago I started exploring ways to improve the code review process for myself who was spending most of my days reviewing at that time. As I found it very effective for myself I formalized it and made it beneficial for other teams as well. I explored and implemented changes in many areas but one of the greatest value change was how we utilized and wrote pull requests (PRs).
There’s definitely tons we could talk about solely in the PR/review process —standardization, commit messages, rebase/squash, checks, actions, branch management, etc. but for this article I want to focus on the PR conversation tab here:
This tab alone has so much teams could utilize to make huge impacts on their code review process. Note: the screenshots here are all from Github I’ve implemented this process on Gitlab before as well.
The executables
Pull request template(s)
I have to give credit to my colleague Ben for this one. I’ve used PR templates loosely in the past but he pushed hard for this one and I don’t think I’m ever going back freestyle PR descriptions.
Setting a template for a repo ensures that the necessary information is captured. What’s great is that whatever is “necessary” is defined by the team. There shouldn’t be any irrelevant required write-ups. This will provide a rough guideline of what to fill in per PR. Not everything has to be mandatory and the level of details could fluctuate between PRs, ideally any information is better than no information. Of course, you’ll always have that one dev who doesn’t fill it out or just cmd+a and delete, but that’s what retros are for.
The following is not a PR template I’ve use on any project, it’s a combination of all the sections from various projects.
## Jira
https://personal.jira.com/browse/XX-XXX## Upstream PRs(Optional)## DescriptionA few sentences describing the overall goals of this pull request.## Approach(Optional)
How does this change address the problem?- #### Problem with the code:
- #### Fix :## Test Steps1. Login as [user-type]
2. Do...## Results(optional screenshots)## Definition of Done- [ ] Acceptance criteria completed
- [ ] Unit-Tests
- [ ] Responsive support
- [ ] Cross-browser compatible (Chrome (61), Firefox(56), Safari(11), Opera(48), IOS Safari(11) browsers and 2 major revisions)
- [ ] Documentation (Optional)
This serves as a starting point if you don’t know where to start. Every project and even projects at different cycles may require a different template so be flexible. Customize the sections as your team see fit — add sections if necessary, such as the Definition of Done, we rarely use that, but it has proven useful on a couple projects. Also, not all the sections need to be filled out, mark them as optional. I always include the Approach section in the template but only required to fill it out when our approach is unconventional or requires explanation/defence. Don’t get me wrong though, (implementation) conversations should still happen regularly through stand ups, planning and throughout the work days. There shouldn’t be too many surprises at this point.
The template should help reviewers easily gain context of the work and what to test without reading through ~1000 lines of code and some reviewers could just be functional testers who has no technical background. When they’re also ready to review the code the description could also provide additional context where simply reading the code might not.
Filling in the PR description could also help the dev collect his/her thoughts. Maybe it’s through the checklist or even just writing the test steps — they’re thinking through the steps and suddenly realize they missed two other scenarios where this bug could occur. Once, I was formulating my approach write up and realize in English it sounds way too “weird” compared to in code and I re-thought of my approach and realize there was a more orthodox method.
Here’s how to implement PR templates for Github and Gitlab. At the time of writing this article Github only supports a single template and Gitlab supports multiple. Having multiple templates is great because you could cater to different code updates: feature, documentation, test, etc.
Communication through labels
While your board should communicate the current state of tickets and progress there’s also a lot of subtext that’s part of code review that may not make sense to document on the board. Code review is not as simple as pass or not pass — at least not in most of my projects.
While a PR is being reviewed it could have it’s own “sub-state”. For example: maybe there are questions/conversation on the PR, dev is waiting for help from others, it’s temporarily blocked, or it’s ready but don’t merge yet (pending something). Rather than having columns in our board for all our scenarios we could communicate the state of the PR with labels.
Is there a problem with documenting the state in two different places? Well it depends on your team and workflow. Imagine if our board already had 8 columns and it’s being used by PMs, devs, designers and QAs on the team. Only one of those eight columns is for “code review” the rest of the columns is for the ticket to funnel through the other roles. If the others aren’t concerned with the sub-state of code review this is a great way to parse out and declutter, but maybe the team is small or only devs then maybe having more columns on the board makes sense. Another approach might be a hybrid, some of the state is defined on the board and some are on the PR based on who the information is relevant too — it’s all about being agile.
Another benefit for labels are that at a glance anybody can see and identify the state of each PR. If I’m looking to review I can immediately see where I should divert my attention. There’s no need to open each PR, ping the group chat or make any guesses.
If you’re looking for a starting point, here’s a list of some of the commonly used labels I have used across different projects:
Certain projects may have a few more/less customized labels based on our flow such as QA, Unit Test, UX Issue, and/or Requires Designer Input.
Screenshots, screenshots, and screenshots
Throw in some screenshots — I mean you built it and tested it right? Just take a screenshot of that and upload it. This provides great visual context for the reviewer, especially somebody non-technical testing the PR. It could also provide a point of reference of what to look for.
Screenshots could also be handy to aid your test steps. We’ve all been there. Sometimes we don’t built apps in a straight-forward order, sometimes we have to mock certain data locally, and sometimes test steps are just as confusing as shit. Whatever the reason maybe if the test steps could be confusing or just too much to write out you could always throw a screenshot or two to guide the reviewer.
As an added bonus for your audience— throw in an animated gif. Sometimes a static screenshot may not be sufficient. Imagine I’m adding an Ajax modal which also requires the throbber to show up inline. An animated screenshot could demonstrate both of these function as well as where the trigger might be located (if it wasn’t clear just from the written steps). It’s not always necessary and could be an overkill sometimes, use your own judgement. I prefer to include one whenever there’s something interactive. This has saved my ass more often than not when reviewers would come back saying something like “this doesn’t work — are you sure it works?”.
For Mac users: Github doesn’t quite support videos in the PRs so if you’re on Mojave+ just do cmd+shift+5 to do a screen recording and use something like Gifrocket to convert it.
Additional benefits — documentation
One of the biggest selling points of well written PRs is they naturally act as part of the documentation. It may not be immediately apparent but further down the road when you revisit the repo or somebody new joins/takes over the PRs will provide loads of information. They’ll outline what was being developed from the description, the intended use case from the test steps, the state the completed work was at the time from the code combined with screenshot, potentially why code was written the way it was from the approach and potential comments on the PR, and anything else the team included as part of their template. Even if the team no longer has access to the board (which happened to me before) all the context remains.
At any point if you’re trying to understand more than the functionality of a codebase you’re going to end up at the PRs, either through commits, history, blames, issues/tickets or just straight up searching the PRs.
The conclusion
Like any process it’s only as good as its users. If a team can’t religiously act on the executables then it all falls apart. I know it sounds like a lot of work. After coding up a storm the last thing anybody wants to do is spend additional time writing up a PR, but I always argue, it’s just a bit more upfront legwork to save headaches on the long run. Before all this I’ve experienced PRs wasting more of the dev and reviewer’s time — back and forth about test steps, screen shares, endless chat threads for clarity. I’m not saying that having well documented PRs will eliminate those things but for the last few years I’ve definitely experienced significantly less of those disruptions upfront.
