Untestable 2

A couple of days ago I posted some untestable code. I’ve had a couple of emails saying that people couldn’t see why the code was untestable. Here’s why, and here’s how to fix it.

The code shown manages a list of listeners who are interested in being notified when the system date changes. It’s externally driven by calling CheckForChange(), this could be called in response to a timer event or from a polling thread, or whatever. CheckForChange() compares the date now with the date we last checked and notifies the listeners if the date has changed. It’s thread safe as it uses a critical section to lock accesses to the list of listeners, allowing listeners to safely register and unregister from multiple threads or for CheckForChange() to be run on its own thread.

There’s not really that much to test, which is good. It should be easy and quick to write a few tests for this piece of code; but it’s not.

First let’s think of how we might test the code. When starting to write a test harness for a class I tend to write a test that simply constructs and destructs the object. This makes it easy for me to start. I simply build the test harness and get it linking and running and create the object. To be able to construct the object I need to be able to provide access to any services that the object requires. This may involve creating some mock objects to act as instrumented service providers to the object under test. In this case the constructor takes no parameters, so we simply create the object and we’re done. Run the test.

Now that we can create the object we can use that test as the basis of a second test. The second test could register a listener and then unregister it. We need to create an object that can act as the listener; this could be a mock object that implements the listener interface or our test harness itself could use the ‘self shunt’ technique and implement the listener interface itself. I’ll go for the mock object here as I foresee the need to register more than one listener in a later test and self shunt will be less useful for that. So we construct a mock listener and write a test to register and unregister the listener. Run the test.

We’re now in a position where we can test the notification aspect of the code. We use the previous test as the basis for this test and simply call CheckForChange(). Of course, we now just have to wait for the system date to change and… Oh. Untestable.

The problem is that the code uses a global variable. It’s not obviously a global, it’s disguised as a static method on a class, but it has the same effect as if it were a simple global variable. In two places the code obtains the current date by calling CTime::GetCurrentTime(), first to initialise “today” and then each time that CheckForChange() is called to get the current time so that it can be compared to the stored time to determine if the date has changed.

The code ‘knows’ where to get the current date and it reaches out and grabs it whenever it needs it. This means that it’s impossible to drive the code with a test harness unless the test harness can change the system’s view of the time and that seems a little excessive. If we were testing CTime’s implementation of CTime::GetCurrentTime() then we’d have a valid reason to write a test harness that fiddled with the system time directly, but we’re not.

So how can we fix it?

This is a case where parameterize from above will help. Rather than having the code go off on its own and locate a source for the current time we could pass it a source when we create it. We could create an interface with a single method that returns the current time and pass an object that implemented this interface to the DateChangeManager when we construct it. The implementation of this date provider would be pretty simple - return CTime::GetCurrentTime() should do…

Now rather than reaching out to somewhere that we have no control over the object obtains the information that it requires from a source that we have complete control over. We can test the object by creating a mock date provider that implements the interface that we defined and returns dates that we can control. Now we can set the date in our mock date provider, create the object to test, wire up a listener and then change the date in the date provider and call CTime::GetCurrentTime() to trigger the date change check. We can then check that our mock listeners were notified of the change.

Once we have this in place we can write a few more tests; testing for no change, testing 1 second before and 1 second after a change, testing to see what happens if the date goes backwards, etc. We have complete control over the environment in which the code under test is operating and can test effectively.

Parameterise from above via an interface is perhaps a bit of an overkill solution to this particular problem though. It may be simpler to pass the results of CTime::GetCurrentTime() into the constructor and the call to CheckForChange(). Parameterise from above at point of use - the Add Parameter refactoring. This removes the need to create an interface, removes sizeof(pointer) bytes from the size of the DateChangeManager object and reduces coupling slightly - I always feel that simple types create less coupling than custom types…

When I first wrote the tests for this piece of code I used parameterise from above via an interface, mainly because I’d been testing a piece of code that needed similar functionality and already had the IProvideTodaysDate interface and a mock implementation. Once I got the tests working I went back to the main project to integrate the changes and decided that in this situation the ‘unsightly wiring’ that this kind of usage of parameterise from above seems to imply was too much in this case. I sometimes find that a problem with paramaterise from above; I often end up with messy constructors that are creating objects which they need to pass *this to (sometimes multiple times) because the object under construction implements the parameterise from above interfaces for a variety of contained sub objects…

Anyway, I digress, I decided that the wiring was ugly, thought about it some more, came up with the second solution above, adjusted the tests and refactored the DateChangeManager to match.

This is one of those pieces of code where writing the test is more valuable than running the test. The code was simple enough for me to be tempted not to bother with a test, but it was late on a Friday and I didn’t fancy writing new code so I started to write a test instead. Being unable to write the test made me identify a subtle problem in the code. I’m now more likely to spot this problem when writing code that I do need to test…

Global data, Singletons and any ‘well known’ object that is used ‘without asking’ will lead to code that is hard, or impossible to test.

Parameterise from above via an interface is a heavy weight solution to this problem. Parameterise from above at the point of use, the Add Parameter refactoring, may be preferable and may introduce less coupling.

I notice that Wayne Allen is having similar thoughts…