Unit testing methods which are calling static methods
Disclaimer: the article below talks about methods calling public static methods from other classes. This excludes the calls to private static methods.
Recently, I wrote a rather unpopular answer on Stack Exchange about static methods in a context of unit testing. The original question was about ways to test methods which, in turn, rely on static methods, and was itself illustrated by a method which relies on a static ToHex which takes a byte array as a parameter and returns a string which is a hexadecimal representation of the argument.
I answered that this is specifically one of the problems with static methods. Instead, I suggested to use dependency injection in order to be able to use during runtime an actual converter from a byte array to a string containing a hexadecimal representation, and during tests, a stub.
This question led to a discussion with one of the members of Stack Exchange community who contacted me by e-mail. In my response, I expanded on the subject of unit tests, static method and dependency injection. I believe it would be useful to share my answer here, since the subject appears to be quite confusing for some people. I was, for instance, quite surprised to find the same subject in the comments of my other answer.
Calling static methods from an instance method makes life miserable. Especially when it comes to unit testing. This has absolutely nothing to do with state. The problem is that the code under unit test relies on logic which is somewhere else, outside the tested method or even the class the method belongs to. This creates its own problems:
Unit tests are not unit any longer. You can't focus on a specific class and its interface with others. Instead, you need to consider the class itself, but also all the static methods it calls. And the static methods called by the original static methods. And so on. Some static methods could be within libraries and frameworks which don't belong to you, which doesn't make things any easier.
When a static method, used outside its class, is changed, it should break its unit tests, but could also break unit tests of other methods which simply relay on this static method. This is the opposite of what one wants from unit tests: what one wants is for those tests to pinpoint exactly and unambiguously the location of a regression. If the build shows me that my last modification broke sixty tests, I have to spend time doing work that should have been done by the unit tests in the first place.
When project grows, one of the complexities which may appear is that a specific implementation of something is not enough. Instead, there should be different variants for different situations.
Long ago, when I was using C#, I was regularly hitting an annoying pattern—well, regularly, until I solved it. I start a project using static methods such as File.ReadAllText or File.Copy, and one day, I need to store data in isolated storage. So I spend the next hours renaming all the methods in order to use IsolatedStorageFile.CopyFile (that's not static, hey!) and similar ones, and then I obviously forget some of them, and the application doesn't work as expected, and I ask myself what damn fool I was to think that I would have to rely on static methods from File class forever. Later on, I need to support both isolated storage and BLOB storage to a remote database, and I finally do what I could have done from the beginning: proper dependency injection with an interface which contains the common methods to work with files, and which is ready to have as many implementations as needed: one which simply relies on File members under the hood, another one which doesn't hit the stupid 259-characters limit in path names, another one which relies on a database, another one which adds caching, etc.
It is surprising to see how often developers assume that a bunch of things will never change. Some believe that it is perfectly OK to assume that the project will always rely on MySQL. Others think that it's fine to link their application to SharePoint, instead of treating SharePoint as just a dependency, which it is. Others believe that static methods from .NET Framework are the way to go. The fact is, requirements change, and if your code is interlinked with something that you thought will be here forever, you'll suffer.
To make life slightly less miserable, one needs to use dependency injection. It its most elementary form, the caller could be injecting an action to execute at some point. More complex cases would require to inject an object which implements a specific interface which defines which action or actions can be called.
A bit too abstract? Well, let's talk by giving concrete examples.
Unfortunately, people who are happy calling static methods give really bad examples. For instance, they ask: “OK, let's say you have code which should add two numbers. Would you inject an object which implements an addition operator, or would you simply call a static method which does the damn addition?” My answer is neither. When I need to compute the sum of x and y, I don't use dependency injection and I don't call static methods. I do z = x + y, and it ends there. This example is dumb, so let me give more realistic ones.
Example from the project 1
A simple application works with some set of data. Some data is stored in binary format, enclosed within the Blob class to the remaining part of the application. At some point, the BLOBs need to be displayed in hexadecimal format. The conversion is needed strictly in a single location (i.e. whenever we have an instance of the Blob class, we could end up doing a conversion to hexadecimal, but never outside the presence of the Blob class).
While some programmers could be inclined to create a static “utility” class with the conversion to hexadecimal inside, I strongly advise against this approach (and I'm also strongly against utility classes in general). The conversion method clearly belongs to the Blob class itself. It would possibly end up as a public instance method ToHex, or, if too complex or used indirectly by the BLOB itself, it could also be a private static member of the class.
The place of the method, as well as its visibility, is essential here when it comes to testing. The conversion is something internal to the Blob class. If it's a public instance method, it should be unit tested; as simple as that. If, on the other hand, it is a private static one, it cannot be tested directly, but will be through the unit tests of the public members of the class which rely on the conversion.
The first variant (public instance member) is straightforward in terms of tests failed after a regression: one or several tests which point to the method itself would fail, thus directly showing the location of the problem. The second variant (private static member) is slightly more difficult, as the failed tests would show only the methods which rely on the code which has a regression, and it's up to the developer to find which one is that. There is also a third variant: public instance member which is also used by other public instance members. Here, you'll have a bunch of tests which failed: some would pinpoint to the faulty member, but others would point to the members which only rely on the faulty one.
Both second and third cases are nasty, because they present several failed tests and let you chose among them, and also because you would likely spend time debugging. Unfortunately, there is no common approach to solve this issue. One may hope that the class is relatively small (and if it's not, there is possibly an issue with the architecture), or the diff for the file which corresponds to the class is small. In most cases, the precise location of the problem will be found quickly enough by glancing at the diff. In other words, any failed test will guide the developer to a specific file, and the diff of this file will likely guide to one or several lines. Not bad.
Example from the project 2
Let's now imagine another project, which is a business application which manages some entities within a company. It relies on the conversion from binary to hexadecimal in four locations:
To log small pieces of binary data. The logging format is handled by a specific logging middleware which is able to decide which data exactly should be converted to hexadecimal.
To trace input and output of all the calls to a quite unreliable third-party service, in order to report the issues to the service provider and be able to prove to the provider that the service is misbehaving. Since the service occasionally sends or receives binary data, this data should be converted to humanly-readable (in what world exactly hexadecimal representation could be considered humanly-readable ?...) form before being stored in trace files.
The application has a notion of user roles, and one of the roles is a system administrator, who is able to see within the UI itself some identifiers displayed in hexadecimal form.
And finally, the application sometimes need to compare pieces of data (including binary), and the third-party diff library which is used by the application crashes miserably when encountering the character 0x00. Therefore, the binary data is first converted to hexadecimal, and only then passed to the diff library.
Here, conversion is used at several locations, meaning that that approach which was used in the project 1 doesn't work there. Let's start by a naive approach of a public static method in a static class. This method is used across the application, and everything seems to be fine, unless two things happen.
Regressions
At some point, someone modifies the implementation of the conversion. The change introduces a regression. No big deal, the code is tested, so once the developer commits his changes, the build fails, reporting sixty-five failed tests. Among those sixty-five, there are, somewhere, the three ones which cover the public static method, which means that you have 5% chance opening one of those, and, inversely, 95% risk of opening one which will point you to a class which relies on the conversion. And if the code is in a bad shape, the class which relies on the conversion may not directly call the conversion; instead, it will call some method from another class, which would then call... well, you see the picture. Personally, I don't want to be that developer who finds sixty-five failed tests at 7 p.m. on Friday.
Change requests
Possibly even worse is a request to have a different implementation for one of the locations where the conversion is used. One could expect a result such as 09c3b2, or 09C3B2, or 0x09C3B2, or the one I absolutely hate, but which is the only one available in .NET Framework: 09-C3-B2.
Imagine the product owner tells that in the UI, the value should be displayed in all capitals. At the same time, there is a discussion that in the logs, the value should be prefixed with “0x” to make it clear that what follows is a hexadecimal value.
One way—and programmers who dare do that should be fired—is to add a ToUpper in the UI layer, and prepend "0x" string in the logs. This is wrong, because one is increasing the risk of having subtle bugs later on. For instance, imagine that six months later, another programmer is asked to add "0x" prefix everywhere. Nobody tells him that there is already such prefix in the logs, and he doesn't spot it when looking at the code itself, so he adds the prefix in the common public static class. And then, a few weeks later, someone is wondering what's the heck with all those 0x0x09c3b2 in the logs.
Another way is to split the code, i.e. to refactor it in order to have different conversions for different usages. The public static ConversionUtility.BinaryToHex will be replaced by three public static methods:
ConversionUtility.BinaryToLowerHexConversionUtility.BinaryToLowerHexPrefixedConversionUtility.BinaryToUpperHex
If the initial method was used directly (i.e. the actual code which needs to conversion calls the method), such refactoring will be relatively easy. If, on the other hand, the code is tangled, such refactoring could quickly turn into nightmare. Especially illustrative is the case where two pieces of code which now need different results rely on common code which, in turn, calls the conversion method. For instance, being logically close, the logging code and the code which traces the inputs and outputs from and to the unreliable third-party service may be implemented as derived classes of a common base class. This base class and only it was calling the utility. In this case, the refactoring would need to touch to the base class and the derived classes.
Compare it now with the DI approach. Originally, you have a IBinToHexConverter with a single member, Convert, which takes a sequence of bytes, and returns a string. The interface is implemented by, on one hand, a BinToHexConverter, and on the other hand, by a BinToHexConverterStub. The first one, naturally, contains the actual implementation. The stub contains some basic mapping between hard-coded values (it may be scoped to the consumers as well, so for instance the thing which sends the values to the UI has its own stub, while the logging code has its own).
Regarding the regressions, if I break the conversion within the BinToHexConverter, I get my three failed tests on the next build. Not sixty-five; three.
Regarding the change requests, I can just pop up additional implementations. First, rename BinToHexConverter to BinToLowerHexConverter. Then add BinToLowerPrefixedHexConverter and BinToUpperHexConverter. When it comes to the location where two derived classes are relying on the base class to call the conversion, the change is absolutely straightforward: pass a different implementation to the derived class.
In essence, the DI approach is a tiny bit more complex at the beginning: injecting a dependency requires more code than simply calling a static member of another class. This being said, even at the beginning, DI already brings a benefit of making the dependencies very clear. By looking at the list of interfaces it requires, I immediately know that it converts binary data to hexadecimal.
The full benefit, however, appears later, i.e. as soon as we start adding different implementations. The fact that we are reasoning in terms of interfaces here is also beneficial. Imagine that I need to add conversion to Base64. No big deal. The same interface remains; I just rename IBinToHexConverter to IBinaryConverter, and add an implementation for Base64.
It is common to believe that this approach makes refactoring more difficult compared to the approach where code was calling static methods directly. In reality, it gets easier. This applies both to the code, as well as the tests themselves: you essentially modify only the tests which cover the code you're changing. You may have noticed that in my example of a change of format from 09c3b2 to 0x09c3b2, tests weren't affected at all. An implementation was swapped. So what? It's not of the business of unit tests. Integration tests? Sure, they will be impacted. But this is a different story. At the unit test level, the stub used as a replacement for the binary converter would still return the same values as before; since it's a stub, the values it returns are dumb, and unrelated to the actual format which is used during runtime. For instance, the stub may just return the string "012345" when being called, independently of the actual input value.
On the opposite, if one was using a static method, a change in the format would propagate to the sixty-five tests (maybe less, depending on the change).
Of course, there are situations where a nasty interface change has severe cascading impact, requiring to change literally hundreds of classes and all the corresponding tests (and the stubs). An example of such change would be the need to introduce a possibility to decrypt data. In other words, in project 2, all data is now encrypted, and it is required to first decrypt it, and only then convert it to hexadecimal format. This would require IBinaryConverter.Convert(Blob) to be replaced by IBinaryConverter.Convert(Blob, IDecrypter). However, if we were using public static members, the change would still be radical.