(or “How to review a merge request” if you’re using GitLab like my team. I’ll use the term “pull request” here.)
What should you look for as you’re reviewing a pull request? It’s important to review the code and verify the functionality so you can be sure the change does what it should and doesn’t introduce any new issues.
1. Read the associated ticket
If you’re using a ticket tracking system like JIRA, read the ticket to see what you should be looking for in the pull request.
2. Manually test the app for UI and functionality
Check out the source branch of the pull request and manually test it to verify that the pull request does what it the ticket says it should do. Review the UI and any added or updated functionality.
3. Run the unit tests
If you have automated tests (as you should), running them is simple enough in Xcode with Product > Test or ⌘U. Make sure all the tests pass and let the pull request creator know if they don’t. This can and should be automated in a CI environment, but I like to run them on my machine as well.
4. Review the code for style, completeness, simplicity, and readability
Is the code style consistent with what’s already in the app? Look for brace style, naming conventions, line breaks, and the way the code is implemented.
Is the code complete? Are error cases handled appropriately? Is there any input that could break the app? Note that if the tests are complete, the code should also be complete. This is another reason to make sure you have good test coverage.
Is there anything that could be simplified in the code? Sometimes this means using a built-in class, framework, or method instead of doing things manually.
Is the code readable? Make sure you understand what it’s doing — if you can’t understand it now, you’re going to have a hard time maintaining it later.
5. Review the unit tests for style, completeness, simplicity, and readability
Assuming the creator of the pull request wrote tests, you should review them the same way you review other code in the app. Additionally, make sure that the assertions in the tests are strong. Do they use XCTAssertEqual to verify actual values rather than just XCTAssertNotNil to make sure a value exists? Do the tests cover a reasonable number of cases of the production code?
6. Make comments on the pull request
When you find something wrong, comment directly on the line of code in the pull request rather than discussing in Slack or somewhere else. This ensures that you’re having the discussion in context so you and the creator and other people can follow along more easily. If you do have a conversation in person or over the phone, summarize your discussion in a comment on the pull request so you can remember it later and others can benefit from your conversation.
One of my previous teams decided that sometimes it made sense to fix issues as part of that pull request, and other times it made more sense to create a new ticket to track and address the issue later. If your pull requests seem to live forever in and endless cycle of commenting and updating, your issue tracking system may be able to help.
7. Approve the pull request after comments are addressed
Once the creator has fixed all the issues (or created tickets) and updated the pull requests, test the app again and approve the pull request to let them know it’s ready to merge.
Tips
- Keep your pull requests small as a courtesy to the reviewer.
- Be thorough in your review; don’t review half a pull request now, wait for them to make changes, and review the second half later.
- Overlook some things. Not everything is important enough to spend time fixing and nothing will ever be perfect.
- Consider making small fixes yourself. Sometimes it’s just easier to reformat the code yourself than it is to ask the creator to do it.