Saturday, November 21, 2009

Mocking static methods: the road to Hell...

...is paved with good intentions.
On Thursday I came across the video presentation of a tool to mock static methods in Java:
PowerMock can be used to test code normally regarded as untestable! Have you ever heard anyone say that you should never use static or final methods in your code because it makes them impossible to test? Have you ever changed a method from private to protected for the sake of testability? What about avoiding “new”?
Horror. Not because of the technological revolution - maybe being able to subclass final classes only in the test suite would be fine, and this is just monkey patching - but because static methods have nothing to do with testability. Or, they make code untestable, but testing is not the reason why we want to avoid them.

Dependency Injection is not about testing: it is about good, decoupled design and reusable units. Static calls represent hidden connections between your classes that are not listed anywhere, and they give access to global and usually mutable state. They help creating a misleading Api and constitute the procedural part of object-oriented programming.
This is an example of bad Api:
<?php
class UserRepository
{
    public function getAllUsers()
    {
          return Db::findMany('User');
          // this would be the same: 
          // return Db::getInstance()->findMany('User'); 
    }
}
If in different tests we use this code:
$repository = new UserRepository();
$users = $repository->getAllUsers();
we probably get different results. But why? We had instantiated the same object and called the same method without any parameters. This Api is far from being referential transparent, and that's because it has a static reference that jumps to global state instead of having it injected.
So the question is not How can I mock a static method?, but How can I avoid static methods?
<?php
class UserRepository
{
    private $_db;

    public function __construct(Db $db)
    {
        $this->_db = $db; 
    }
    public function getAllUsers()
    {
          $this->_db->findMany('User');
    }
}
<sarcasm>It was very difficult, isn't it?</sarcasm> Dependency Injection is in fact very simple: ask for what you need, so that people that read your code know where collaborators are coming from and you are not secretly smuggling them in your classes.

I once heard a talk by Misko Hevery where he was making a point about dangerous global state, and he described the following situation. Suppose you have your beautiful test suite, and some classes in it accesses global state, so the tests are not real unit tests but have a bit of integration in their hearts. Keep in mind that in this environment tests are not isolated, since they depend on some global variables, maybe masked as singletons or static classes. The order of execution matters.
So you have MyClassTest, that is the last test case to run in the hour-long test suite, and it is failing. So you try to run it alone, and it pass. Then you run the suite again to figure out what is happening, and it fails again. The test is referencing some global state in which particular data is placed by the other tests before MyClassTest is run. The only way to hava a reliable failing is to run all the suite to set up the global state necessary.
The conclusion is: good luck in finding what is making MyClassTest failing.

Static methods are not object-oriented: they are a recipe for problems if they carry hidden state. They are global, because you cannot keep them on an instance you can throw away after tests. They make the Api difficult to grasp by hiding dependencies under the carpet. The solution is not "Let's open up the hood and wire these things differently so that we can mock static methods", but "Let's not use static methods."
You should even write the tests before the production code if you can. We do not write them only because they catch bugs and regression, but also because real unit tests force to code in a clean and decoupled design. If you are able to unit test your application, its architecture and flow is composed of focused, reliable and reusable parts. If you use static methods and spread new operators everywhere, there are hidden connections between components and mocking these things it's only Action at a distance.

4 comments:

Fake51 said...

Your example with a call to DB::method is not very good - if you're getting different results from it in different methods you have either changed the underlying dataset or you haven't done your setup properly before testing. Sure, you can move the setup into the test by using DI (and it's not a bad thing either) but don't go blaming static methods for problems that they don't cause.
Like singletons, static methods are not evil, they are just very often used in the wrong way (leading religious fanatics to call them evil). In other words, don't let static methods change state, only let them be service calls that don't change anything underlying - then they are perfectly fine and will the job great

Saulo Silva said...

Interesting article, but I don't think the question is "How can I avoid static methods?" but rather "How can I use static methods in the right situations?".

As Fake51 said, static methods are not evil, it's just people often do the mistake of using them in the wrong places.

I think you mentioned in another article that there is nothing wrong in making Math.cos() static.

Giorgio said...

For the testability there is nearly no problem for Math.cos() to be a static method, since it has no global state/mutable behavior: cos(Pi) is always equal to cos(Pi). For the design, it would be best Calculator.cos() or even 2.cos() because in the latter case numerical data will be kept together with behavior, but cos() is a so simple method that I will probably never need to mock it.

Nada_Surf said...

Nice article!

ShareThis