Friday, August 1, 2014

Complex Logic in Unit Tests

There is a rule somewhere in the land of best practices that says you should avoid putting logic in Unit tests. For example, if you were making a suite of unit tests that had some common logic then you wouldn't create a method to reuse code because that would lead to the test becoming harder to read and understand.

The idea is that unit tests should read like a behavioural specification. The test should be so simple and clear that it tells the reader exactly how the function should behave without having to poke around inside different functions or crank a for loop mentally to guess what happens.

Basically,

  1. Don't worry so much about Don't Repeat Yourself because when you put things into variables, constants or functions you make people hunt down these pieces of code and it makes the test harder to read.
  2. Don't use fancy flow control like loops if you can possible avoid it because it means the user has to work out what the flow control is doing and that hurts readability.
I've tried this approach for several years and I can tell you that's it's all crap. The ultimate result is an unmaintainable mess.

I feel I've heard these unit test arguments before. Somewhere in our codebase there's a pile of older C/C++ code. It contains functions that are very long; pages and pages of source code per function. I remember this coding style being considered best practice by some, not just for performance but because it made the code easier to read precisely for the same reasons that are now being used for unit tests. In retrospect, it doesn't make anything easier to read and it makes code really hard to maintain.

Code is code. The reasoning behind Don't Repeat Yourself doesn't magically get suspended because you're writing a unit test. Whenever you're writing code you're constantly building abstractions that help you to solve the problem you're working on. Things like Don't Repeat Yourself allow you to find and leverage these cases of code reuse into tiny frameworks. Unit tests are no exception to this. If your code is maintainable and well factored it will be intrinsically readable because you'll have 5 well chosen lines of code instead of a page of boilerplate.

Here's an article about not putting logic in tests from Google's testing blog. In this article the example the author chooses is the following:


@Test public void shouldNavigateToPhotosPage() {
  String baseUrl = "http://plus.google.com/";
  Navigator nav = new Navigator(baseUrl);
  nav.goToPhotosPage();
  assertEquals(baseUrl + "/u/0/photos", nav.getCurrentUrl());
}

In this test there is an error. Can you spot it? Ok, let's write the test without the code reuse:

@Test public void shouldNavigateToPhotosPage() {
  Navigator nav = new Navigator("http://plus.google.com/");
  nav.goToPhotosPage();
  assertEquals("http://plus.google.com//u/0/photos", nav.getCurrentUrl()); // Oops!
}

The error is now obvious. Hurrah for no logic in tests!

However, what if you didn't spot it? What if the error was "photso" instead of a double slash? I don't know about you, but there are a bunch of words I always type wrong. Things like spelling height as "hieght" or "heigth". What if the URL was http://www.heigth.com/? In that case I would be testing a typo... but only sometimes.... If you're not reusing then you're duplicating so this error is going to be in a bunch of places and you have to find and fix them all.

For that matter, aren't there libraries to append a path on a base url already? Modifying example #1 is easier than example #2 because in the first case we know that the base URL is common text because it's explicitly marked and check by the compiler right there in the code. In the second example, how do we know what the text in assertEquals() is supposed to represent without doing a tedious, manual character by character comparison?

If logic in unit tests has you concerned about making errors then don't worry. You'll make plenty of errors no matter how you do things. In one case it's a single error in one part of the code has to be code reviewed once and fixed once. If you repeat yourself it's like the 1970s and code is duplicated all over the place with slight differences it has to be fixed many times and code reviewed until your eyes bleed. If logic in unit tests really bothers you, make your complex logic into re-usable functions and unit tests that. Yes, it's a little "meta" but at least you're being honest about the complexity instead of copying around madness without worry.

To reiterate, best practices like "Don't Repeat Yourself" came into existing to make reading and maintaining all code easier. I can tell you from personal experience that ignoring it results in difficult to maintain tests. 

What you really need to do is make what you're doing in your test as clear as possible - don't hide your function's inputs and outputs. I can't go into detail here but here's what the test would look like if I wrote it:


@Test public void shouldNavigateToPhotosPage() {
  mNav.goToPhotosPage(); // mNav is a member defined in the fixture
  assertEquals(BASE_URL.withPath("/u/0/photos").toUrlString(), 
               mNav.getCurrentUrl());
}


In the example above I've created a test fixture (not displayed). An mNav member variable is built in the test fixture's setUp(). The mNav is always initialized with a base URL. I've made a BASE_URL object using an existing library. In this library BASE_URL is immutable but has several methods that return a mutated (also immutable) object.

The net result is you can see that calling goToPhotosPage() should go to the "/u/0/photos" path.

If mNav had a getBaseUrl() and it was tested elsewhere then you might use that instead of BASE_URL constant since it's more explicit about what the expectation is.

Don't worry about logic in unit tests - that is a red herring -worry about making what you're testing clear to the reader.

2 comments:

Michael Buhot said...

Maybe a string concatenation isn't a convincing example, but can you really tolerate ifs and loops in your tests?
The worst cases I've seen are when the test duplicates the implementation logic of the SUT - rather than giving concrete examples of how it should behave.

Andrew said...

In order to avoid loops, flow control or functions I have seen programmers do monstrous code duplication and various other atrocities.

If the advice is to not put flow control, functions or loops - then I am against it. To me this advice is basically "write unmaintainable unit tests". If the advice is to make unit tests read more like a specification then I am for it.

The key difference is that it's possible to write any unit test in such a way that avoids both code duplication and fancy things like loops, flow control, functions; so the unit test reads more like a spec. However, if you can't figure out how to do that then write the loop or whatever because the reasons behind these coding best practices still apply in unit tests.

To put it another way, I would rather see a loop where a loop makes sense than an unrolled loop with the loops contents duplicated ten times or so.

For example, you see a loop in a unit test. If the advice is there should be no loops in a unit test, the easiest way to fix this is to unroll the loop. This solves nothing, because the unit test still has whatever problem it had originally with the addition of redundant code coming from unrolling the loop. This is the sort of thing I have seen in actual code reviews. This is the sort of thing I've had the unpleasant task of arguing about. This is where I'm coming from.

Better solutions to the loop question depend on why the loop exists.....

If the loop exists because it's really testing multiple things then those things should be in separate unit tests (with no copy and pasted code setup code). If the loop exists because it's building some test data then that's perfectly fine too - put it in a function. Stick the function somewhere that makes sense and make sure there's some basic test to make sure you're getting what you think you are. If the loop exists to compare the contents of two arrays then you can delete it because there's already a library for that.. etc..

So yes, it should read like a spec. But no, you don't get a free pass from coding best practices to accomplish that.