The Top 10 Checks You Should Do Before Raising a Pull Request
Raise PRs That Everyone Will Thank You For
Introduction
You might think that NASA astronauts spend most of their time trying to break records for the longest spacewalk. Of course, you’d be wrong. The vacuum of space is fraught with dangers, so astronauts spend most of their time going through checklists — lots and lots of them.
Checklists are essential for mitigating common problems and are used in all kinds of scenarios. For software engineers, a Pull Request (PR) checklist ensures that everybody’s PRs satisfy a standard set of quality assurance rules.
In this article, I discuss my Top 10 checks I do before raising a pull request.
1. Compare Changes
Carefully check before-and-after changes made to each file in your PR.
For example:
- Have you removed/updated other code in error unintentionally?
- Does each unit test correctly describe what's going on, and for updated tests, are the descriptions still accurate?
It's best to catch these problems earlier when committing code. It’s much better to take an extra few minutes now examining your changes than having to fix them later.
2. Check Naming Conventions
Ensure that you follow the naming conventions used within your organization: give classes, methods, and variables appropriate names that describe their function correctly without the need to use comments.
Also, compare your code against:
- Capitalization rules
- The use of underscores (or not) in private variables
- File naming conventions
If your conventions aren’t documented on the team wiki, make an effort to add them.
3. Evaluate Accessibility
Determine which methods could be kept private or protected. For example, if a method is only ever used within a class, it makes sense to make that method private.
4. Confirm Unit Tests Pass
If a PR is blocked because of unit tests that don’t pass, it suggests you haven’t taken care to run the unit tests beforehand. Execute unit tests ahead and take advantage of any features to prevent you from checking in work if unit tests don’t pass.
5. Make Only Meaningful Changes
Remove all comments made in the code. Comments should only be short-term notes to help and remind you about what’s required while writing code.
Also, don’t add unnecessary whitespace changes to edited files. Unless whitespace checking is disabled, whitespace distracts the PR reviewer as she/he needs to check each highlighted change, only to discover that it’s not relevant.
6. Drop and Recreate the Database
If you make any database-related changes, it’s always good practice to drop your local database and re-create it to ensure there are no errors.
7. Do Local Smoke Testing
There’s nothing worse than utilizing everyone’s time to approve a PR, only for the QA Analyst to re-assign the task back to you because of a bug found during testing.
Therefore, perform local smoke testing of the changes you’ve made to ensure you’ve captured bugs and met the acceptance criteria. Also, while doing your local smoke testing, keep the browser console up to check for console errors.
8. Check the Pull Request Has a Title and Description That Follows Team Rules
A PR description should be a simple explanation of what the impact of your code changes will be. Following that, you should explain how you made those changes and why.
A PR description should be automatically composed of bullet points created from the commit descriptions. So it follows that commit messages should themselves be accurate.
9. Attach Testing Evidence
I’m a nice person (honest); however, I have some harsh words:
“If no testing evidence is attached to the PR, I assume that testing never took place!”
There are so many reasons why you should include step-by-step screenshots in a testing document attached to the PR.
Mainly, testing evidence:
- Shows the PR reviewer you have tested your code.
- Gives some context to the changes you have made to another developer.
- Helps newer QA Analysts understand the workflow in their testing.
- Provides you with evidence that you have been diligent in your testing should something go wrong at a later point.
In particular, for database changes:
- Developers should take a screenshot of data in relevant tables between screenshots of UI actions.
And for UI changes:
- Take a screenshot of the UI before making changes, then again after making changes, and attach both screenshots to the PR. Doing this makes it a lot easier for everybody to visualize CSS changes.
A personal preference is to place screenshots into a document, convert this to a PDF, then attach the PDF to the PR (as shown below).
Notice also in this example that I’m helping the PR reviewer by clearly marking the description and test evidence headers using bold font and utilizing bullet points to improve readability?
10. Reference Tasks and User Stories
Ensure that the PR references all relevant tasks and user story IDs so there is a paper trail for changes.
Some DevOps platforms — such as Azure DevOps — allow you to search and attach IDs to your PR (see below).
Final Thoughts
If you can, edit your PR template and include checkboxes that every developer needs to review before raising a PR, e.g.:
[X] Story ID referenced?
[X] Task IDs referenced?
[ ] Local Smoke Testing Passed?
[ ] Test evidence attached?
[ ] DB re-build successful (where relevant)?For example, the Pull Request page in an Azure DevOps controlled project looks likes this:
Thanks for reading! Let me know what you think in the comments section below, and don’t forget to subscribe. 👍
