Top 10 things which make your code hard to test

July 30th, 2008 · 7 Comments ·

by Miško Hevery

So you decided to finally give this testing thing a try. But somehow you just can’t figure out how to write a unit-test for your class. Well there are no tricks to writing tests, there are only tricks to writing testable code. If I gave you testable code you would have no problems writing a test for it. But, somehow you look at your code and you say, “I understand how to write tests for your code, but my code is different <insert excuse here>“. Well your code is different because you violated one or more of the following things. (I will go into the details of each in a separate blog posts)

  1. Mixing object graph construction with application logic: In a test the thing you want to do is to instantiate a portion (ideally just the class under test) of your application and apply some stimulus to the class and assert that the expected behavior was observed.  In order to instantiate the a class in isolation we have to make sure that the class itself does not instantiate other objects (and those objects do not instantiate more objects and so on). Most developers freely mix the “new” operator with the application logic. In order to have a testable code-base your application should have two kinds of classes. The factories, these are full of the “new” operators and are responsible for building the object graph of your application, but don’t do anything. And the application logic classes which are devoid of the “new” operator and are responsible for doing work. In test we want to test the application logic. And because the application logic is devoid of the “new” operator, we can easily construct an object graph useful for testing where we can strategically replace the real classes for test doubles. (see: How to Think About the “new” Operator with Respect to Unit Testing)
  2. Ask for things, Don’t look for things (aka Dependency Injection / Law of Demeter): OK, you got rid of your new operators in you application code. But how do I get a hold of the dependencies. Simple: Just ask for all of the collaborators you need in your constructor. If you are a House class then in your constructor you will ask for the Kitchen, LivingRoom, and BedRoom, you will not call the “new” operator on those classes (see 1). Only ask for things you directly need, If you are a CarEngine, don’t ask for FuelTank, only ask for Fuel. Don’t pass in a context/registry/service-locator. So if you are a LoginPage, don’t ask for UserContext, instead ask for the User and the Athenticator. Finally don’t mix the responsibility of work with configuration, If you are an Authenticator class don’t pass in a path of the configuration information which you read inside the constructor to configure yourself, just ask for the configuration object and let some other class worry about reading the object from the disk. In your tests you will not want to write a configuration into a disk just so that your object can read it in again. (see: Breaking the Law of Demeter is Like Looking for a Needle in the Haystack)
  3. Doing work in constructor: A class under tests can have tens of tests. Each test instantiates a slightly different object graph and than applies some stimulus and asserts a response.  As you can see the most common operation you will do in tests is instantiation of object graphs, so make it easy on yourself and make the constructors do no work (other than assigning all of the dependencies into the fields). Any work you do in a constructor, you will have to successfully navigate through on every instantiation (read every test). This may be benign, or it may be something really complex like reading configuration information from the disk. But it is not just a direct test for the class which will have to pay this price, it will also be any related test which tries to instantiate your class indirectly as part of some larger object graph which the test is trying to create.
  4. Global State: Global state is bad from theoretical, maintainability, and understandability point of view, but is tolerable at run-time as long as you have one instance of your application. However, each test is a small instantiation of your application in contrast to one instance of application in production. The global state persists from one test to the next and creates mass confusion. Tests run in isolation but not together. Worse yet, tests fail together but problems can not be reproduced in isolation. Order of the tests matters. The APIs are not clear about the order of initialization and object instantiation, and so on. I hope that by now most developers agree that global state should be treated like GOTO.
  5. Singletons (global state in sheep’s clothing): It amazes me that many developers will agree that global state is bad yet their code is full of singletons. (Singletons which enforce their own singletoness through private constructor and a global instance variable) The core of the issue is that the global instance variables have transitive property! All of the internal objects of the singleton are global as well (and the internals of those objects are global as well… recursively). Singletons are by far the most subtle and insidious thing in unit-testing. I will post more blogs on this topic later as I am sure it will create comments from both sides. (see: Singletons are Pathological Lairs)
  6. Static methods: (or living in a procedural world): The key to testing is the presence of seams (places where you can divert the normal execution flow). Seams are essentially polymorphism (Polymorphism: at compile-time the method your are calling can not be determined). Seams are needed so that you can isolate the unit of test. If you build an application with nothing but static methods you have procedural application. Procedural code has no seams, at compile-time it is clear which method calls which other method. I don’t know how to test application without seams. How much a static method will hurt from a testing point of view depends on where it is in you application call graph. A leaf method such as Math.abs() is not a problem since the execution call graph ends there. But if you pick a method in a core of your application logic than everything behind the method becomes hard to test, since there is no way to insert test doubles (and there are no seams). Additionally it is really easy for a leaf method to stop being a leaf and than a method which was OK as static no longer is. I don’t know how to unit-test the main method!
  7. Favor composition over inheritance: At run-time you can not chose a different inheritance, but you can chose a different composition, this is important for tests as we want to test thing in isolation. Many developers use inheritance as code reuse which is wrong. Whether or not inheritance is appropriate depends on whether polymorphism is going on. Inheriting from AuthenticatedServlet will make your sub-class very hard to test since every test will have to mock out the authentication. This will clutter the focus of test, with the things we have to do to successfully navigate the super class. But what if AuthenticatedServlet inherits from DbTransactionServlet? (that gets so much harder)
  8. Favor polymorphism over conditionals: If you see a switch statement you should think polymorphisms. If you see the same if condition repeated in many places in your class you should again think polymorphism. Polymorphism will break your complex class into several smaller simpler classes which clearly define which pieces of the code are related and execute together. This helps testing since simpler/smaller class is easier to test.
  9. Mixing Service Objects with Value Objects: There should be two kinds of objects in your application. (1) Value-objects, these tend to have lots of getters / setters and are very easy to construct are never mocked, and probably don’t need an interface. (Example: LinkedList, Map, User, EmailAddress, Email, CreditCard, etc…). (2) Service-objects which do the interesting work, their constructors ask for lots of other objects for colaboration, are good candidates for mocking, tend to have an interface and tend to have multiple implementations (Example: MailServer, CreditCardProcessor, UserAthenticator, AddressValidator). A value-object should never take a service object in its constructor (since than it is not easy to construct). Value-objects are the leafs of your application graph and tend to be created freely with the “new” operator directly in line with your business logic (exception to point 1 since they are leafs). Service-objects are harder to construct and as a result are never constructed with a new operator in-line, (instead use factory / DI-framework) for the object graph construction. Service-objects don’t take value-objects in their constructors since DI-frameworks tend to be unaware about the how to create a value-object. From a testing point of view we like value-objects since we can just create them on the fly and assert on their state. Service-objects are harder to test since their state is not clear and they are all about collaboration and as a result we are forced to use mocking, something which we want to minimize. Mixing the two creates a hybrid which has no advantages of value-objects and all the baggage of service-object.
  10. Mixing of Concerns: If summing up what the class does includes the word “and”, or class would be challenging for new team members to read and quickly “get it”, or class has fields that are only used in some methods, or class has static methods that only operate on parameters than you have a class which mixes concerns. These classes are hard to tests since there are multiple objects hiding inside of them and as a resulting you are testing all of the objects at once.

So here is my top 10 list on testability, the trick is translating these abstract concepts into concrete decisions in your code.

Tags: Advice · Rant · Testability

7 responses so far ↓

  • Greg M // Aug 1, 2008 at 2:18 am

    None of these things are specific to testability, they’re just plain good ideas for your code - except for static methods. So what’s going on here?

    The thing is, I don’t think it’s the static methods per se that are the problem, it’s when you call out from within a supposed “unit” to an external static method. You shouldn’t really be doing this anyway - you should have composed them, possibly via IOC.

    Let me also point out that the problem of static methods at a high level in your call graph might actually be a problem of non-static methods at a low level in your call-graph - remember to keep your stateful/side-effecting control code at the top level of your call graph.

  • Mike Johnson // Aug 1, 2008 at 8:11 am

    Oi. Some of these are common sense, but others are limiting. I’m tired of reading articles like this listing all the things you can’t do in order to test. Can’t use the new operator? Seriously?

    Now I’m supposed to jump through a _lot_ of hoops to avoid ‘new’ making my app more complicated and more prone to failure.

    Oh wait, just write _more_ tests for that, too! Ach.

    Maybe, just maybe, the current crop of testing tools aren’t up to snuff. Instead of doing funky things in your application code, maybe somebody should figure out a smarter testing framework and let the rest of us focus on getting work done.

  • misko // Aug 1, 2008 at 10:09 am

    @Mike Johnson -> “Now I’m supposed to jump through a _lot_ of hoops to avoid ‘new’ making my app more complicated and more prone to failure.”

    Well separating the ‘new’ operator is called “Dependency Inject” and judging by high number of dependency-injection frameworks out there I think it is a good idea.

    Contrary to your statement it makes your app less complicated, easier to understand and hence less likely to fail, even if you don’t write any tests.

  • Jeremy Ross // Aug 5, 2008 at 12:28 am

    Ok, so maybe there are some items here that could stand on their own as general advice, but testing and testability don’t occur in a vacuum. I’ve found that when I focus on writing testable code, I ended up with code that is well factored.

    I don’t think writing testable code is second nature for most people. So revisiting the basics can be reinforcing and in some cases enlightening, kinda like seeing a movie for the second time.

  • Top 10 things which make your code hard to test « Quero ser Ágil // Aug 8, 2008 at 12:12 pm

    [...] 8, 2008 de Rafael Mueller No embalo do post de ontem, outro artigo do Miško Hevery com coisas que você deve evitar no seu [...]

  • Ben Sanders // Aug 29, 2008 at 2:56 am

    Aren’t the titles for 7 and 8 backwards? I.E., “Favor composition over inheritance” - that makes things easier to test, not harder.

    In any case, great list!

  • Ben Taylor // Sep 9, 2008 at 6:37 am

    Hi Misko,

    Nice article. Thanks.

    I have a question. You say above in “Ask for things, don’t look for things” the following:

    “So if you are a LoginPage, don’t ask for UserContext, instead ask for the User and the Athenticator.”

    In this example, the User is something that (it seems to me) is not easy to inject with a DI framework as it varies, based on the current User. However, the Authenticator is something you could easily inject. So, how do design this example, when you want to use a DI framework and constructor injection?

    Ben

Leave a Comment