Code pollution



This is another post from my unit testing anti-patterns article series. Today, we will talk about code pollution.

Code pollution: mixing test and production code

Another anti-pattern that often comes up in unit testing is code pollution. It takes place when you introduce additional code to your main code base in order to enable unit testing.

It usually appears in two forms. The first one is when you add methods to the existing classes. For example, let’s say that you have a Repository class that saves an order to the database and retrieves it back by its Id:

And let’s also say that you write an integration test that creates a couple of orders and saves them to the database via the system under test (SUT). Now you need to verify that the SUT did everything as expected. You query data in the Assert part of the test and make sure it is correct. In other words, do something like this:

How would you do this, assuming there’s no code in the main code base that can query all customer’s orders yet? The natural thing to do here would be to add this code to OrderRepository. After all, this functionality relates to both orders and the database, so that feels like an organic place to put such code to:

However, his is an anti-pattern. You should avoid adding code to the main code base that is not used in production.

Here, OrderRepository is a class from the production code base. By adding GetByCustomerId to it, we are mixing it up with code that exists solely for testing purposes.

Instead of mixing the two up, create a set of helper methods in the test assembly. In other words, move the GetByCustomerId method from the production code base to the test project.

So, why do that? Why not just keep it in OrderRepository?

The problem with mixing the test and production code together is that it increases the project’s cost of maintenance. Introducing unnecessary code adds to that cost, even if it’s not used in production. And so, don’t pollute the production code base. Keep the test code separate.

Code pollution: introducing switches

The other form is bringing in various types of switches. Let’s take a logger class for example:

How would you test the Controller’s SomeMethod? This method calls the logger which records the text to, say, a text file. It’s a good idea to avoid inducing such a side effect because it has nothing to do with the behavior you want to verify. How would you do that?

One way is to introduce a constructor parameter in Logger to indicate whether it runs production. If it does, it can log everything as usual. If not, it should skip logging and return:

Now you can write a test like this:

It works but, as you might have guessed already, this is also an anti-pattern. Here, we introduced additional code to the production code base for the sole purpose of enabling unit testing. We have polluted Logger by adding the switch (isTestEnvironment) to it.

To avoid this kind of code pollution, you need to properly apply dependency injection techniques. Instead of introducing a switch, extract an interface out of Logger, and create two implementations of it: a real one for production, and a fake one for testing purposes. After that, make Controller accept the interface instead of the concrete class:

Now you can refactor the test too and instantiate a fake logger instance in place of the real one:

As the controller expects the interface and not a concrete class, you can now easily swap one logger implementation for the other. As a side benefit, the code of the real logger becomes simpler because it doesn’t need to handle different types of environments anymore.

Note that the ILogger itself can be considered a form of code pollution too. After all, it belongs to the production code base and the only reason it exists is that we want to enable unit testing. And it probably is a form of code pollution. Therefore, replacing the switch with the interface haven’t solved the problem completely.

However, this kind of pollution is much better and easier to deal with. You can’t accidentally call a method on an interface that is not intended for production use. It’s simply impossible: all implementations reside in classes devoted specifically to the use in production. And you don’t need to worry about introducing bugs in interfaces either. An interface is just a contract, not the actual code you need to cover with unit test.

Summary

  • Don’t pollute production code base with code that exists solely to enable unit testing.
  • Don’t introduce production code that doesn’t run in production.
  • Instead of switches that indicate where the class is being used, implement proper DI. Introduce an interface and create two implementations of it: a real one for production and a fake one for testing purposes.

I have two more unit testing anti-patterns to go. Stay tuned! 🙂

If you enjoy this article, check out my Pragmatic Unit Testing training course.

Other articles in the series

Share




  • Daniel Olewski

    For logging I have recently used an alternative approach to remove even the `ILogger` interface: I make my product code expose logging events. Both my test code and the top level product code subscribe to the logging event if they want to.

    This helps with things like logging to the Terminal and to a file at the same time (or a few log files: full.log, normal.log, etc. …).

    This also helps greatly with testability: many of my tests don’t care about logging, so they don’t even subscribe. But those tests that do want to verify the logging produced (my team does a lot of command line tools so we care about the logging output a lot) use an in-memory log capturer hooked up to the event, so that they can inspect each logging event – in an easy functional verification unit test (instead of as a side-effect) as an added benefit.

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      Very nice approach, indeed. It reminds me of the domain event implementation I wrote about here: http://enterprisecraftsmanship.com/2017/10/03/domain-events-simple-and-reliable-solution/

      • Daniel Olewski

        Domain events as you described them seem to have everything go via a central location which defines and dispatches the events. That’s more coupling than I want in this case.
        I like the flexibility to define a logging event at any level – a class, python module.

    • Steven Roberts

      Very interesting approach. Would you share some little code? Maybe a gist or something?

      • Daniel Olewski

        My only implementation right now is in Python. C# will be a little bit more verbose.


        from site_packages.events import Events
        from log_type import LogType

        class Log:
        def __init__(self, spaces_in_indent=2):
        self._events = Events('on_logging')
        self._spaces_in_indent = spaces_in_indent
        self._indent_level = 0

        def subscribe(self, subscriber):
        self._events.on_logging += subscriber

        def log(self, entry_type, message):
        self._events.on_logging(entry_type,
        self._make_spaces_from_indent_level() + message)

        The use:


        class A:
        def __init__(self):
        self.log = Log()

        def do_something():
        self.log.log(LogType.Warning, 'be warned')

        def print_to_terminal(entry_type, message):
        print('{}: {}'.format(entry_type, message))

        a = A()
        a.log.subscribe(print_to_terminal)
        a.do_something()

        • Daniel Olewski

          oops, auto-formatted broke Python code here. is there a way to mark text to be displayed ‘as is’? or better with syntax highlighting of a specific language?

      • Bas Dam

        If you want to know more about this, look for the Observer Pattern. There are examples of implementations in all kind of languages and it is great for logging at will to all kind of targets.

  • E.

    For the Logging you have used an Interface. But, isn’t is possible to call Dependency Injection with Unity for example to the rescue?
    With a Bootstrapper class I register all my dependencies, also the Logger.
    But, in my test-class I can overrule this registration with another one using for example following approach : Ioc.Container().RegisterType(new InjectionFactory(c=> new MyTestLogger()));
    MyTestLogger is then a class that inherits from the Logger class and here I override the methods to achieve the requiered result in my tests.
    I was thinking of this approach because of your former blog about Repositories where you explained that we don’t need interfaces for them.
    Or is this a bad practice?

    • http://enterprisecraftsmanship.com/ Vladimir Khorikov

      I wouldn’t say that a class with virtual methods is a worse approach than the interface. They are more or less identical: introducing an (unnecessary) interface on the one hand or making a method (unnecessarily) virtual on the other. I personally prefer interfaces but that’s just a matter of taste.

      There’s a parallel with the Repositories, indeed, but I don’t see a way not to introduce at least a small amount of pollution here (that is, either an interface or a virtual method). Other than maybe using a delegate – which often not a very useful approach.