Avoid Test Code in Production
Have you ever seen code like that in Listing 5-3?
Listing 5-3: Example of test code in production
public class MyClass {
private boolean testing = false;
public void doSomething() {
if (testing) {
// Mock something up
} else {
// Run the real code
}
}
}
I’m sure you have. Unfortunately, something like this or one of its variations exists in most code bases. Let’s go over all the things that are wrong with this from a software-verification point of view.
First, how is testing set? I may have omitted a setter that would allow directly controlling the behavior through the interface. That would probably be the safest way to manage it. I’ve also seen people use a toggle interface instead. That is a little less safe because you have to always check the value to use it correctly. Both approaches are sometimes implemented without the corresponding getter. Maybe it is controlled through some form of dependency injection, in which case the value is separated from the implementation and the apparent default value may not be the truth. Another approach is to simply change the initialization in the code itself. But then what happens if the developer forgets to change it back before committing? If the mock is good enough, no one may notice until bad things happen after deployment to production. Each of these possibilities carries the risk that the test code can be turned on in production.
Next, ask yourself whether we can test all of the code. We cannot. If we turn on the testing flag, which we ostensibly want to do when we are testing, then we only run the first branch of the if statement. The second branch is only executed when we are not testing. That means that any code in the else branch will not be tested prior to deployment, introducing the risk of undetected bugs.
Finally, if you are using code coverage to drive your testing, this technique creates a “dark patch,” a block of code that is uncoverable. This may be acceptable if you do not have code coverage targets or if your coverage targets have enough slack to absorb the amount of code in the else branch. Most of the code I have seen handled this way is critical initialization or resource-management code that significantly contributes to the behavior of the system. In addition to the untested aspect of the block, these kinds of holes in coverage lead to an expectation of incomplete coverage that weakens the value of coverage as a guiding metric.
Each of these risks compounds the more this technique is used. The chances of any one of the risks manifesting may be small, but each use of this technique increases the overall risk. If they are used broadly in a large enough code base, it is almost guaranteed.
The underlying principle is that test code should be kept separate from the code it is testing. Testing hooks should not intrude into the code under test in ways that are blatantly for testing. Design accommodations for testability should retain the integrity of the design and should preserve the intent of the code as much as possible.