Unit testing smells

At work, we focus quite heavily on TDD and, therefore, unit testing (although our TDD extends to other levels, such as writing automated acceptance tests for a story before beginning development of the story). As is always the case with large bodies of code created by a transient group of people over a reasonable period of time, there are good bits, and there are not-so-good bits.

Today, I’d like to focus on some examples of the latter and talk about why I think they could be improved.

Proliferation of constants

Take the following. This is the beginning of a test class:

public class XyzImplTest {
    private static final long ASK_EXCHANGE_RATE = 200000L;
    private static final long BID_EXCHANGE_RATE = 100000L;
    private static final long NON_ZERO_QUANTITY = 5000000L;
    private static final Currency ACCOUNT_CURRENCY = Currency.getInstance("GBP");
    private static final Currency JPY_CURRENCY = Currency.getInstance("JPY");
    private static final long DEMO_ACCOUNT_ID = 101L;
    private static final long INSTRUMENT_ID = 1235L;
    private static final BigDecimal START_BALANCE = new BigDecimal("10.00");
    private static final BigDecimal DEFAULT_FUNDING_PERCENT = new BigDecimal("4.00");
    private static final BigDecimal MARGIN_PERCENTAGE = new BigDecimal("13.0");
    private static final Account DEMO_ACCOUNT = new AccountBuilder().id(DEMO_ACCOUNT_ID).type(AccountType.STANDARD_TRADER).newInstance();
    private static final long INSTRUCTION_ID = 1598739879387L;
    private static final long SNAPSHOT_ID = 345L;

Now, the actual list of constants goes on for 50 lines. This causes me issues for a number of reasons:

  1. Most of the time, you want constants of the same type to have different values. If I pass x and y to a method and accidentally swap them around in the method implementation, I’ve got no way of detecting that in the test if they don’t have distinct values. But with this many constants going on, it’s hard to check them all to make sure the value to choose for your new one is unique.
  2. Sometimes, you want them to be related! For example, if you were testing a dynamically resizing indexable data structure, you might want to say something like put(10, “blah”) followed by asserting that size() == 11. In this example, 10 and 11 are related. You couldn’t change the value of one of those constants without breaking the test. So now I have to wade through 50 constants to try and understand their relationships – some of which will unfortunately be implicit.
  3. No one test can possibly be using all of these constants. Can it?
  4. I cannot understand the test in isolation. I have to refer to the constant values to understand what’s going on.

This last point is important. The first two can be countered by just being clinical about making sure you express the relationships between the constant value in their declarations. Sort of. But the last point cannot.

I want to be able to understand a test, just by looking at the test. If I have no idea what value the constants are, that’s hard. Taking the data structure example, is:

    collection.put(10, "blah");
    Assert.assertEquals(11, collection.size());

really that much harder to read than

    collection.put(COLLECTION_INDEX, COLLECTION_VALUE);
    Assert.assertEquals(COLLECTION_SIZE, collection.size());

The latter, surely, is much more opaque? It’s hard to tell what the test is doing without understanding the values of COLLECTION_INDEX and COLLECTION_SIZE, for example. And what if another test wants a different value for COLLECTION_SIZE? You end up declaring COLLECTION_SIZE_2 or something equally nasty. In the former example, it’s pretty easy to see that 10 and 11 are related.

Ask yourself this. How comfortable am I changing COLLECTION_SIZE from 11 to 12? How many tests will I break? Sure, you could run them (and you should!) but the pain of fixing them is going to be quite big. But really, you only wanted it to be 12 in one test. Oh well, back to defining COLLECTION_SIZE_2 when really, all you needed to do was change the literal constant 11 to 12 in the test where you cared. Oh my, you really have got all your tests tightly coupled by these constants now, haven’t you?

Which brings me on to my next point…

Test length/complexity

If I can’t see that 10 and 11 are related, then either I shouldn’t be writing code, or the test is too complicated and the relationship is not obvious. Now, the examples I gave above are a bit extreme from a test (non-)complexity point of view, but really, if your test extends to more than 10-15 lines, you’re probably testing too much (or have too many collaborators to setup/mock, see later). The class I’m quoting from has 50 constants, a JUnit @Before method running to some 45 lines, the tests themselves don’t start until line 260 and go on to line 2,216 (yes, two thousand, two hundred and sixteen), and they are then followed by another 400 lines of “private expectations” (see later).

Now, DRY (it’s not new, we were doing in BBC BASIC in 1980 and probably for a long time before that) says that pulling constants out so you don’t use them over and over is a good thing. And in a class that has 105 separate tests, you can see how we ended up here. But these constants aren’t really “repeated” in the DRY sense. If COLLECTION_SIZE had a different value in each test, that would be fine (provided it was relevant to that test, of course!) So by pulling constants out, we haven’t really improved anything about the tests, all we’ve done is misapplied the DRY principle on too broad a scale, and in doing so, we’ve made the tests less readable and more brittle.

This is compounded by the fact that some of the tests in this class are 40+ lines long. Now, I don’t know about you, but if a single test case starts to take more than a screen (for whatever your preferred definitions of screen resolution/font size/etc. are), then that’s a smell! I’m trying to understand what an individual test does. How can I possibly expect to do that, when I don’t know what the values of the constants are, what their relationships are, and I can’t even see all the test case code in one go? And then I’ve got the other 104 to understand… Maybe after lunch!

“Private expectations”

One particular smell that’s prevalent in this code base was christened “private expectation” by one of its primary proponents. This one quickly becomes a religious war because there are people in the team who support it and people who don’t and, by and large, the views are quite strongly held — very few people take the middle ground on this one!

A “private expectation” is just a private method which sets up some expectations. Sometimes, they’re used to set actual expectations, sometimes they’re used to set pre-conditions (the “given” part in “given/when/then”, typically expressed using the allowing(…) method in JMock).

But again, this means I can’t see what the test is doing without looking at all these private methods. Their names might help me, provided they’ve been maintained as the method contents have evolved. (Generally, they’re not all that helpful – the most helpful ones tend to just be a summary of the method contents which is kind of weird.)

This “pattern” has again been born of DRY. But this, again, is a smell. If you have to set the same expectations and/or pre-conditions in test after test then you have too many collaborators which you’re trying to mock out. And, as before, in the one case where you want something just slightly different, you end up duplication the “private expectation” method and tweaking it slightly (yes, you just repeated yourself!) or parameterising it – which is even more obtuse!

I repeat: the only reason you are doing this is because you’ve finally realised that your test methods are getting too long and you’ve noticed that you’re setting the same expectations/pre-conditions repeatedly and for some twisted reason, you chose this as a solution instead of refactoring the class under test so it did less/had fewer collaborators!

The testing framework

I will wrap up by noting that all of the above discussion take place in the context of JMock. I realise that there are less verbose frameworks out there. JMock particularly suffers from the checking(…) block heinously mixing the test pre-conditions and the test expectations. It also forces you to describe every interaction with a mock, whereas other frameworks provide sensible default behaviours.

Conclusions

  • Long tests: your method under test does too much/you are trying to test too much/your class under test has too many collaborators. Non-solution: pull out constants and repeated bits of expections. Why: this greatly decreases the readability of the tests and increases brittleness since the extracted bits really can’t be reused very flexibly. All you’ve done is move code to a different method; it still smells. Solution: refactor the class under test.
  • Lots of tests: your class under test does too much (or you’re testing it to death!). Non-solution: as above. Why: as above. Solution: as above.
  • Framework: if you’re starting from (almost) scratch, pick one that’s not verbose. I’m playing with Mockito right now and quite like it. It’s certainly less verbose than JMock (which, by the way, is awesome and I have no axe to grind against it — it’s just verbose and conflates expectations with pre-conditions).

If you find yourself doing anything like this, your unit tests smell, which means your class under test smells. The solution is not to refactor your tests, it’s to refactor your class under test.

Share this:
Facebook Twitter Digg Email

This entry was posted in Java, Unit testing. Bookmark the permalink.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>