A bug exists when something doesn’t work as expected. A bug fix is basically a patch (a pull request) to the existing code base that is supposed to solve the problem and make sure that “something” works as expected. Very often, such a patch fixes one thing and breaks many others. I believe that sometimes it’s necessary to reject a bug fix and ask its author to re-do the patch in order to protect the project from bigger problems. There are a few valid reasons for such a rejection, according to my experience.
It Degrades Code Coverage
This is a very common situation: After the changes are made in one place, unit tests fail in some other place. The bug is fixed, but some possibly unrelated unit tests start to report failure. Under pressure or simply because we’re lazy, we don’t fix them; we simply remove the tests or mark them as temporarily “skipped.” The problem is solved, the build is clean, so let’s merge the patch and call it a day, right? Wrong!
Even though I’m in favor of cutting corners as much as possible, this is the corner I don’t recommend you cut.
The unit tests are there precisely to prevent us from breaking the product when under pressure.
Obviously, there are situations when the unit tests are wrong and we have to delete them. In those cases, don’t forget to create new ones.
There are also situations when the bug must be fixed in a few minutes to put the system back online and fixing all unit tests will take an hour. Such a situation is a strong indicator that you’ve got a terrible underlying situation with test coverage in the product. There’s no doubt that we have to make a fix and ask our tests to shut up for some time. But in this case, make sure the next task your team is working on after the bug fix is released is correcting those disabled unit tests. I would recommend reading Working Effectively With Legacy Code by Michael Feathers, which tackles this very subject.
It Doesn’t Reproduce the Issue
Sometimes the entire system may be down simply because of a small typo in one line of code. An obvious bug fix is to remove the typo, but that’s not what a good project is expecting from us if we care about its quality. The problem is not the typo but rather the absence of unit tests that would catch the typo at the deployment phase.
The real problem is the lack of test code coverage in this particular section of the code. By removing the typo, we’re not helping the project in any way. Moreover, we’re doing it a disservice — we’re concealing the real problem.
Thus, no matter how small or cosmetic the issue is, its bug fix must contain an extra test that first reproduces the bug. Without such a test, a bug fix is a waste of the project’s money.
Furthermore, without a unit test reproducing the issue, there is no guarantee that our bug fix doesn’t introduce more bugs. I would even say that the more bug fixes we have, the higher the entropy. And the only way to decrease this uncertainty is by covering the code with unit tests. Without a test, a bug fix brings more disorder to the code base.
It Is Too Big
Bug fixes are not features; they must be small and focused. It’s a very typical mistake for programmers to get carried away while fixing a bug and introduce some refactoring together with a fix. The result is that the patch gets rather big and difficult to understand. I’m not against refactoring; it’s a very important and positive thing for a project, but do it separately after the bug is fixed and merged.
No refactoring while fixing a bug!
Create a new unit test, reproduce the bug, and commit it. Fix the bug in the existing code base, no matter how ugly it is. Create new bugs, asking the team to improve the situation with the ugly code base. If interested, assign those bugs to yourself. Or maybe somebody else will be interested in fixing them and refactoring the code. But all that will happen later in other pull requests with new code reviews and new merges.
It’s not about being lazy and unwilling to fix what looks bad. It’s about a discipline, which is much more important than good intentions.
It Solves More Than One Issue
Always fix one issue at a time — simple as that. No exceptions. When a bug fix patch contains code changes that fix multiple issues, it is very difficult to understand which issue is tested, which one is reproduced, and how they relate to each other. Combining several bug fixes into a single pull request is a very bad practice.
No matter how simple the fix is, keep it separate from others. Review, test, and merge it individually. This will also increase the traceability of changes. It will always be easy to understand who made that fix, who reviewed the code, and when it was merged (and deployed).
| Reference: | A Few Valid Reasons to Reject a Bug Fix from our JCG partner Yegor Bugayenko at the About Programming blog. |
“By removing the typo, we’re not helping the project in any way.” Weird. I guess I’ve been doing thing incorrectly my whole career. You know, making stuff work and whatnot. I guess I should keep the typo in?
Yes, you are doing it wrong and missing the point. You need to write a unit test that catches the bug first and THEN fix the typo.
Well, actually the problem here, what we might call a “journalistic bug”, is what is known as “burying the lede” — The real problem with the bug fix is that it is missing a unit test. That part isn’t mentioned until the third paragraph. Had that section been entitled “Pull Request Lacks Unit Test” everyone would have understood had he was talking about.