What developers merge when nobody's looking?
Zero required approval
This article is about practices we use at Entria to ensure the quality of our code and keep the team's speed and productivity
Why do we need code review?
When you start a new project, you are usually the only developer working on it. You probably just commit your code directly to the main branch without using pull requests and without any code review. You are the only one responsible to ensure the quality of the code, and making sure the code works as expected.
As you add a second developer, you can't ensure his code quality without doing a code review. You need to add more structure to ensure what worked before to keep working. As a consequence, instead of committing directly to the main branch, you both need to open a pull request, ask for code review from another, and then merge it to the main branch. This is the Paradox of Structure, it enables and limits at the same time. More structure enables you to work with more than 1 developer, but also limits how fast can you move.
The first decision you need to take is to make a code review required or not to enable merging the pull request. This decision has a lot of consequences over time, let's analyze both required and not required consequences and also some unexpected consequences.
Required approval for merging
As your team grows, you decide to have 1 or more required review approvals to enable merging the pull request. The largest the number of required reviews, the harder it is to merge a pull request. The owner of the pull request needs to wait a lot of time until N developers review their code and approve the changes, or ask for changes. The below chart shows how the probability of waiting time increases when you add more required reviews.
Here you are changing speed/time for “quality”. “Quality” because you can't ensure that code reviews are done properly by the team. Indeed, adding more evaluators helps in finding problems, as Nielsen described in How to Conduct a Heuristic Evaluation.
However, only more experienced evaluators will find hard issues in the code review.
One unexpected consequence of required review for the merge is that developers won't review well to focus on their own work, they can review using “LGTM” (looks good to me) without providing any code improvement feedback. LGTM is the worse type of review, as it does provide any feedback for the code and can approve something that should not be approved.
No required approval for merging
If you decide to go in the other path of not required review to merge pull requests. Developers can merge unfinished code too fast, and cause new bugs and regressions more often. The code will also diverge if nobody is reviewing the code to make sure it follows the patterns. This is the classic “move fast, break things”.
A better approach for code reviews
As we analyzed both extremes of code reviews (required review, not required), we can understand better the tradeoffs we are doing, and which approach is better for our project. What if we could have the best parts of both worlds? What if we could enable merge without required review and also keep the quality of the codebase?
Automated code review
As I have written before, your QA team won't get this bug before it hit production, your team won't always provide the best code review possible. Our approach is to automate most part of the code review, so no manual reviews mean only automated review. We use typescript, to make sure our code types are all valid. We use a linter to ensure the patterns in our codebase. Furthermore, we have integration tests to ensure we won't break any existing behavior when adding a new feature, solving a bug, or refactoring some code.
In short, tests, type checking, lint, other checks review every pull request. Manual reviewers are still needed to ensure the business logic is correct, and the tests are written well, but they do not need to be required.
Post-Merge Code Review
We recommend all developers in the team review all requests. This enables every developer to learn how other developers solve a given problem. This also gives them experience reviewing code. Besides, it gives context to work on that part of the codebase when needed.
As we want to have a fast pull request workflow, where you open a pull request, wait a few minutes for review, and then merge. Not all developers can review in a few times, so they do a review after the merge. As we use git to track the version of our code. The developer of the pull request can always improve the code in the next pull request.
We also use small pull requests to ensure the code review is easy and fast to be done. We use a feature flag to keep unfinished work to break production.
All the changes from the last release are reviewed by the whole team before reaching production. So we always review at least twice each piece of code.
Code Review Culture
The first code review of a given pull request should be the review of the owner of the pull request. The owner of the pull request should make sure that his/her code works as expected, that types all type checks, that lint is working well, and the tests are written to document expected behavior. After all this check, the developer can open the pull request and request a code review from the team.
Not all pull requests need to review right away, if you are completely sure where code changes works, or the code change is just a simple refactor or style adjustment. You can merge the pull request without any review. If there were something wrong with your code, the post-review will ensure you correct it later on. You can always revert a wrong pull request.
In summary, we don't want to block developers for small and simple changes.
Developers don't want to break production. No developer will merge a big structural change in the codebase without waiting at least a few reviews, or a review from a specific team member. For these hard pull requests, the developer himself doesn't want to do the merge alone, he/she wants to share the responsibility with other teammates.
Conclusion
Required review to enable merge of the pull request is the easiest way to ensure that developers will code review, but it is not the optimal one. Adding automated reviews (type check, lint, tests, and more) enable you to avoid required review and also keep the quality of your code.
Your culture and developer training will tell, “What developers merge when nobody's looking?”
great article!!