My Pre-PR Checklist
I’m impatient. Fundamentally I want things to go fast, I want immediate results, and I want instant gratification. This is true in my personal life, and especially true in my work: one of the least favorite parts of being a developer is waiting for builds, waiting for CI, and waiting for results of tests. There are engineering solutions and optimizations that can be applied to many of these areas to make life progressively less painful, I am consistently grateful for the devops and platform engineers who dedicate their time to making improvements to the development experience. That kind of work is often thankless but really makes a difference.
As I develop features in my work I get quite excited when it comes time to open up my code for review: creating pull requests to receive feedback from my peers is a great way for me to learn and grow, I also look forward to having my work be released to users. As I get closer and closer to sending a PR I find myself getting physically more engaged with the development process, racing to get my code in front of reviewers and integrated into the mainline branch. In my haste and excitement I have sadly fallen into a pattern of sending work for review that isn’t as high-quality as I could have produced. In these cases with just a little more care and time I would always have been able to produce a product that I could be truly proud of. I know that I need to combat this, in order to ensure that every change that I make is shipshape I have developed and adopted a checklist that I follow before requesting review of a new change. Below I list out the steps that I take and the questions that I ask myself as I near the point of submitting code.
Diligent manual testing! This is the hardest part of this process for me, and also the most important. I miss edge cases, I fail to test all paths, ignore error states, and sometimes miss interactions between app (or user) states. I’m actively working on improving the care and time that I take in this area - as I find new areas of potential concern when testing a given piece of work I add that area to a running list that I keep near me, then every time I go to make a new change in that area I ensure that I check on these cases. The ever-growing list makes me less likely to forget about something. It still takes more willpower than I’d like to slow myself down and put in the time that testing takes, but I’m improving bit by bit.
Have you written new unit tests? Can you? Can you expand existing tests to cover any new behavior? Unit tests turn manual processes into automatic ones - they have immense value and make us faster overall; ensuring that new work is as thoroughly covered by unit tests means that future developers (ourselves included) can spend less time testing and more time implementing.
Run code linter, considering if there might be new lint checks that you can add as a part of the work to ensure that downstream users will make proper use of it. Custom lint checks are an aspect of development automation that I’m increasingly seeing value in: they can do much more than keep the code consistently styled, they have the ability to prevent downstream developers from misusing your code and making common mistakes when consuming the APIs and systems that you’re working on. I believe that custom lint checks have extra value when compared to unit tests or other forms of automation since (thanks to IDE integration) they can prevent mistakes at implementation time, before a PR is ever opened.
Run your file formatter (if it’s not automated into your CI workflow). File formatting should be integrated into your CI workflow, but running the formatter is just good hygiene and could reduce feedback cycles on open PRs.
Double-check that your work doesn’t do anything catastrophic in offline mode. It can be easy for developers to forget how common it is for mobile apps to be used in cases where the device doesn’t have an internet connection. Spending any amount of time on the subway, in an airplane, or just outside of a major metro and you’ll find that it’s important for app developers to consider how their work will function in these cases. Users don’t expect full behavior when they are offline, but you should handle the case gracefully, at least providing an error message that indicates the offline state.
Include screenshots (or better yet, GIFs) of new behavior and user interface elements in the summary of your PR. Taking screenshots acts as a forcing function for me, especially in the course of implementing new behavior. It requires me to prove to myself and others that I have implement UIs that match the specifications given, and it makes sure that reviewers are able to double-check that the UI looks right without needing to patch down the changes and build them locally - though this is the gold standard for collaborative testing and PR review.
Is the work that you’re submitting tracked in your team’s task-tool? Should it be? Sometimes we engineers take initiative to “make it better” without being handed down a task tracking ticket. This is great most of the time, but it’s a good idea to retroactively file a ticket and link it to the PR that you’re making in order that the rest of your team will be able to keep track of the work in progress. The process of creating this ticket will also help you craft the description for your PR since it forces you to summarize the intention of your changes.
Will this need targeted testing from QA? Have they been notified of the impending change? If your team has a dedicated QA organization it is a good idea to loop them in to the changes that you are making as early as possible so that they have time to prepare and can help you identify test scenarios that you might have overlooked. With that said, it happens on occasion that you get to the end of a piece of work and realize that QA might not be aware of the changes coming down the line - it’s still better to notify them before opening a PR than after merging it.
What time of day is it? Is this the optimal time for you (and your reviewers) to be looking at this code? One of the pieces of advice that I was recently given by my manager was to consider how the time that I choose to send a PR for review would affect the quality of review that I get. Normally in the past I had been submitting code for review at the end of the working day, after finishing up a ticket right before heading out the door for the night. Eager to be done for the day, I had noticed a pattern in myself of not always sending out the cleanest change sets; sometimes missing obvious mistakes or areas that could have been improved. Refraining from requesting review of these PRs until I’d had a chance to give them another look the following morning (and seeing the changes with a fresh set of eyes) helps me to make sure that I’m requesting review of high-quality work. This reduces the number of cycles that the code needs in review and has improved my overall output velocity.
Review the size of your change set, ensuring that it is in line with your team’s standards. Most teams like to keep their PRs to a certain size and scope, reviewing your work to make sure that it’s in line with these expectations is respectful of your teammate’s time and will make PR reviews more effective. Keep in mind that there isn’t a clear mapping for lines or files changed to complexity; the context and content of the change matters.
Identify the ideal number of reviewers for this change and make sure that you are requesting feedback from the right people. You want to make sure that you are getting reviews from a variety of people while also making sure that the work is seen and acknowledged by those who have context for the changes and can raise questions relevant to the product surface that you are working on.
Being militant with myself and following this checklist every time I make a new PR or update an existing PR has been a challenge for me, making me feel like I’m going slow and sometimes boring me with the repetitive nature of the process. As I continue to use this framework I’ve found that the larger-picture speed at which I’m able to complete work has remained consistent or slightly increased. Having this structure has meant less time with code pending review thanks to fewer comment/update cycles; I’m sure that the more times I use this process the more I’ll become efficient with it.
I’m going to keep iterating on and improving my process, I’m sure that I’ll add more to it as I continue to make mistakes. I’m thankful to work in an environment where making mistakes once is accepted, so long as one learns from them and can demonstrate that the same mistake won’t be made repeatedly.