Dependency injection and mocking with reflection.

[Stephan van Hulst]

I'm a huge fan of dependency injection, and Inversion of Control frameworks in particular. I'm very well aware of the fact that using dependency injection makes unit testing a breeze in combination with mocking frameworks.

Recently I've encountered a lot of code containing static helper classes, responsible for returning application state. One such class is a Config class that returns configuration settings from some database. When I voice my concerns over not being able to mock this Config class, I am pointed out that the testing framework we use can mock private and static members of these helper classes. The code is written in C#, here's an example:

PrivateType configWrapper = new PrivateType(typeof(Config));
configWrapper.SetStaticField("myPrivateStaticField", myValue);

This left an extremely bad taste in my mouth, but I can't explain why my concerns are valid. After all, helper classes *are* easy to use and understand, and apparently we can mock them as well with the help of some reflection.

Can anybody explain why this is a bad idea, or whether this spells bad news for inversion of control?

[Tim Cooke]

In his book "Effective Unit Testing":

Lasse Koskela wrote:
Most static methods shouldn’t be. The reason to have them is generally either
because they don’t relate to a specific instance of a class or because we couldn’t bother
figuring out where it belongs so we made it a static method in this utility class. The former
motive is solid but the latter is mere ignorance or lack of interest. Yet another reason,
and an unfortunately common one, is to make it easier to provide global access to
the method by making it static.
Some methods are naturally static. For example, a utility method that performs a
calculation on numbers would likely be a good candidate for a static method. So
what distinguishes a method as one that should or shouldn’t be static? Perhaps the
example in the following listing serves to make the idea more clear.

public static int rollDie(int sides) {
return 1 + (int)(Math.random() * sides);
}

The rollDie() method produces a random result as if you’d rolled a die with a given
number of sides. Dealing with random factors in automated tests is generally something
you should avoid, so you’ll likely want to stub out the rollDie() method in your tests.
My rule of thumb is to not make it static if you foresee that you might want to
stub it out in a unit test one day. It turns out that I rarely need to stub out a pure calculation.
On the other hand, I frequently find myself wanting to stub out static methods
that serve as an entry point to a service or a collaborator object.
Pay attention to the kind of methods you declare static. A static method is easy to
write, but you’ll have a hard time later if you ever need to stub it out in a test. Instead,
create an object that provides that functionality through an instance method.

So in your example, there is no compelling reason to have a method for interrogating the OS or database as a static method. In fact, the difficulty of testing is a far more compelling reason for them to not be static. As a general rule, if I find myself needing to mock private or static methods then I take that as a warning that my design in wrong.

As a temporary measure when working with Legacy code, as you are, I would wrap the static call with an instance class for easy replacement with a test double. For example:

public class SystemProperties {
  public String getProperty(String propName) {
    return System.getProperty(propName);
  }
}

Using static methods for database calls is an example of tight coupling, which is why it's difficult to test.

[Stephan van Hulst]

Thanks for your response Tim.

Tim Cooke wrote:In fact, the difficulty of testing is a far more compelling reason for them to not be static. As a general rule, if I find myself needing to mock private or static methods then I take that as a warning that my design in wrong.

This has always been my viewpoint, but I have a hard time defending it when my colleagues say "we can easily mock static methods". I'm looking for a good reason why using static methods (which traditionally shouldn't be static) is bad, even when it's possible to replace their behavior during testing.

I'm not going to switch to the dark side, but I need some restoration of faith :P

[Tim Cooke]

Your colleagues are right in that you can mock static calls in your tests, however I would contend that it's 'easy' and it's certainly not nice. Even with the testing tools available for this kind of thing, the resulting test code is normally quite unpleasant.

There a couple of things I'd say about it:

The main downside of your design is one of coupling. Having your database code in a static method is tightly coupling your application to your database implementation. What if in the future you decided to change database vendor or decide not to use a database at all, how easy will that be to do? Not very easy because any change has a direct effect on every client of the class. Polymorphism is a powerful feature of Java which would be useful in this case, but you are not making use of it. Decoupling the implementation with an interface would ease in the transition in this example (and as an added bonus would make it simper to test).

A lot of the testing tools available for mocking static and private method calls do make it 'easy' to do this, if you're comparing it to the alternative of using reflection directly. However, in my experience these tools add a lot of noise to your test cases and have a tendency to hide the real intent of the test making them less readable and thus harder to maintain. I might have a different opinion on this point if there were tools available that provided a nice clean way of stubbing out static calls, but given that they are essentially window dressing for the Reflection library I don't expect this to be a reality any time soon.

In general, using static methods is not an inherently bad thing. Functional languages do it all the time! However, Java is an Object Oriented language for which its core features, constructs, tools, and testing tools are all geared towards dealing with objects. When you move away from using objects and start working with static functions you choice of available tools diminishes significantly and you have to start conjuring magic tricks in your tests to get a handle on your code again.

I have a rule of thumb when it comes to unit testing. I expect the level of complexity of my unit tests to be roughly equal to the complexity of the code under test, and if this is not so then a red flag goes up. This is why I hesitate to use heavy frameworks necessary to mock static and private calls. Should a simple method that marshals the retrieval of some values from an external system require the use of Reflection? I say not. There's a significant imbalance of complexity there, which smells like bad news.

In summary, yes you can mock static calls. But the necessity to do so likely indicates that there's a better design begging to be discovered.

[Jayesh A Lalwani]

They are using a Singleton pattern, and then mocking the Singleton for testing. There are many documented drawbacks to the Singleton pattern. In this case the biggest one is...

They are essentially tying the code that uses one of their classes to the Config singleton. Essentially, if they want to take a class out of Project A and use it in Project B, the Config class will have to go with it the class. This means that essentially Project B needs to have the same properties files as Project A. But, project B has it's own properties files!

The problem is not testing. The problem is that they are violating the Single Responsibility Principle. A class that talks to the database should only talk to the database. It shouldn't be responsible for talking to the config class to read the configuration items. In an IoC container, the container is responsible for reading the configuration files.

[Mike. J. Thompson]

This problem is exacerbated by some poor design decisions by the language makers (in my case Java but I'm sure the same problem exists in other languages). My application needs to perform basic operations on the filesystem (checking files exist and reading their contents), and java provides the Files class that has a lot of nice functionality for doing that.

Unfortunately the Files class consists entirely of static methods. This means that any component that uses it can then not be Unit Tested since it depends on the file system. The only way to use it and maintain testability is to produce a facade that just forwards all method calls to Files but that provides non-static methods, thus allowing it to be mocked.

[Stephan van Hulst]

I tend not to use the File or Files classes that much. Generally when I need to perform I/O, I have my methods accept either an InputStream or a Reader.

If you need to mock methods from the Files class, you can either specify your own default FileSystemProvider (Files delegates to such an instance), or you can pass a FileSystemProvider to whatever class is responsible for performing operations on files, and have it use the provider directly, rather than the Files class.

Honestly, I think it should be illegal for fields to be static if they're not value types.