How mocking can be a recipe for disaster

Testing is a Mere Trifle

As a software developer, how would you advise someone to test if a dessert was good enough to serve to your friends and family?

Got your answer? OK. No changing your mind.

In a thanksgiving episode of Friends, Rachael Green decides to make an English trifle. These desserts are traditionally made with custard, sponge cake fingers, and raspberry jelly (jello if you are American).

The pages of her recipe book are stuck together, and she ends up mixing in beef, sauteed with peas and onions. She doesn’t question this, thinking that the British have odd traditional food, and it’ll probably work out.

The result “tastes like feet”.

Hands up if you answered “I would taste it,” or a variant thereof.

Good. You can invite me over anytime.

Did you answer “I would compare the steps taken to the steps in the recipe?”

Sorry – I’d love to come over but I’m really swamped with twitter posts I need to catch up on.

This is one example of a tautological test. “I compare the steps I planned to take to the steps I ended up taking” gives you no confidence that you had a good plan in the first place.

In general a test is Tautological when it repeats some of the implementation of the system under test.  These tests end up adding very little value.

Unfortunately software developers write this sort of test all the time, and don’t even know they are doing it.

Why do we use TDD?

Here are four commonly expressed reasons:

  1. To increase confidence that the code works
  2. To help us make better design decisions
  3. To make refactoring easy
  4. To help prevent regression bugs

Take a look at this code…

public class CarService
{
    private readonly ISearchQueryFactory _queryFactory;
    private readonly IRepository _repository;

    public CarService(ISearchQueryFactory queryFactory, IRepository repository)
    {
        _queryFactory = queryFactory;
        _repository = repository;
    }

    public List<Car> FindAll()
    {
        var query = _queryFactory.Create<Car>(); 
        return _repository.Search(query);
    }
}

This example presupposes we have a Repository that takes a Query<T> object to which we may add any search filters and allows us to Search the database for matches. Since we want all cars as results, in this case we use the default unfiltered query.

…and the code that tests it.

[Test]
public void FindAllShouldSearchForAllCars()
{
    var mockFactory = new Mock<ISearchQueryFactory>();
    var mockRepository = new Mock<IRepository>();

    var testee = new CarService(mockFactory.Object,
        mockRepository.Object);

    testee.FindAll();

    mockFactory.Verify(x => x.Create<Car>(), Times.Once);
    mockRepository.Verify(x => 
        x.Search(It.IsAny<SearchQuery<Car>>()),
        Times.Once);
}

This is a modified example from an excellent introduction to the tautological test antipattern by Fabio Pereira, which I am reusing to look at alternatives.

When we discussed this example in my office, at least initially, the room divided into two camps – was this test good or not?

Let’s rip it to shreds.

This test gives the coder no confidence that the implementation is correct (1) – because the recipe in the code is repeated in the test.

var query = _queryFactory.Create<Car>();
return _repository.Search(query);
mockFactory.Verify(x => x.Create<Car>());
mockRepository.Verify(x => 
    x.Search(It.IsAny<SearchQuery<Car>>()));

From a TDD point of view it does (emptily) perform the function of being a failing test that forces certain code to be written – but unlike black box TDD, this test gives no scope for the problem to be solved in any other way. This test cannot help a good design grow out of the requirements. (2)

A worthwhile question then is – if you knew you would write this code this way anyway, why did you bother with a test?

How about refactoring? Well if we try and implement the code another way, the test will fail, even if all the production code works. But if we try and change the test first, we still hit a red light. This is terrible – we should be able to refactor production code while maintaining a green light all the time! For each change we must edit the code and the associated test(s) – incidentally this also breaks the single responsibility principle. (3) We can’t have any confidence that our refactoring is side-effect-free if we require the tests to be rewritten – so we have no protection against regression bugs. (4)

And yeah, that eliminates all four of our reasons why we work with TDD in the first place.

So we could write tests like this and claim to be performing TDD. But if we do, we get none of the benefits of TDD whatsoever. Rather the tests have negative value – reducing the flexibility of the code; slowing down maintenance.

There must be a way of testing this feature that isn’t tautological, right?

Sure!

Let’s start with an integration test.

[Test]
public void FindAllShouldReturnAllCars()
{
    var expectedCars = 
        new List<Car> { new Car() { Id = 1 } };

    var factory = new SearchQueryFactory();
    var repository = new Repository();

    var testee = new CarService(factory, repository);
    var results = testee.FindAll();
    
    CollectionAssert.AreEqual(expectedCars, results);
}

Well this is fine. This tests that the code works, and there’s no ugly knowledge floating around in the test about the internal implementation.

In terms of being black box, this is ideal, and will have the best possible chance of remaining robust even if the implementation changes.

Supposing that the repository talks to a database, then the big issue with this test is that it does not isolate the system under test from its external dependencies. We can’t know that the test database will always be available; it may have maintenance costs of its own; and external resources may be slow to access, which reduces the effectiveness of our test suite as a fast-feedback system.

By stubbing just the external dependencies, we can resolve these speed and maintenance issues (but we trade away the knowledge that the system works correctly even across the architectural boundary).

[Test]
public void FindAllShouldReturnAllCars()
{
    var expectedCars = 
        new List<Car> { new Car() { Id = 1 } };

    var factory = new SearchQueryFactory();
    var stubRepo = new Mock<IRepository>();
    stubRepo.Setup(
        x => x.Search(It.IsAny<SearchQuery<Car>>()))
        .Returns(expectedCars);
            
    var testee = new CarService(factory, stubRepo.Object);
    var results = testee.FindAll();

    CollectionAssert.AreEqual(expectedCars, results);
}

I call this test a “classicist test” because Martin Fowler refers to Classic TDD in contrast to Mockist TDD in his vital article Mocks aren’t Stubs.

The classical TDD style is to use real objects if possible and a double if it’s awkward to use the real thing” – Fowler

When I demoed this example to my colleagues, Steven asked “What about isolation? This test isn’t a proper unit test.”

I probably would have agreed before reading Fowler’s article – but he says that unit testing has not always meant the same thing to all people. As a Classic TDDer, his method is to assume all other systems are working correctly and write the test to test the next unit in the stack based on that assumption.

“[when multiple tests break…] Classicists don’t express this as a source of problems. Usually the culprit is relatively easy to spot by looking at which tests fail and the developers can tell that other failures are derived from the root fault.”  – Fowler

The advantages this test has are speed and black box requirements testing – meaning that it is reasonably robust against implementation change – however a little white boxing has leaked through now that we are providing the return values from the stub.

We can go further.

[Test]
public void FindAllShouldSearchForAllCars()
{
    var expectedCars = 
        new List<Car> { new Car() { Id = 1 } };
		
    var stubFactory = new Mock<ISearchQueryFactory>();
    stubFactory.Setup(x => x.Create<Car>())
		.Returns(new SearchQuery<Car>());
		
    var stubRepo = new Mock<IRepository>();
    stubRepo.Setup(
        x => x.Search(It.IsAny<SearchQuery<Car>>()))
        .Returns(expectedCars);
		
    var testee = new CarService(mockFactory.Object,
        mockRepository.Object);

    var results = testee.FindAll();

    CollectionAssert.AreEqual(expectedCars, results);
}

Now all the dependencies have been stubbed, this is a true isolation test. But there’s a trade off – each time a stub is introduced, a little more white box knowledge leaks into the test.

It’s not as bad as mocking though – each time a mock is introduced, considerably more white box knowledge leaks through.

Compare this test to the mockist test – the mockist test has absolutely no advantages. And in my experience, far more often than not, a test with mocks can be re-written to use stubs. (I’ll look at some times when this isn’t true in a future blog.)

Give me one kind of test I can always write that always works.

No can do.

If you need a rule to take away, then habitual mocking is an unnecessary practice that prevents us from achieving the benefits of TDD.  Sometimes there’s no other choice though.

But there is no one best way. Understand the trade-offs of integration, classic, isolation and mockist testing, and choose whatever works best in each circumstance.

Blindly following a dogma is just a way of making sure you never innovate.

Tautological tests don’t need to include mocks – just assuming what your implementation is can be enough to make blunders. Read more!

4 thoughts on “How mocking can be a recipe for disaster

  1. Pingback: How to spot the Tautological Test Anti-pattern | codertom

  2. Pingback: Exploring alternatives to tautological tests | codertom

  3. Mark Powell

    Interesting read. However, your last example is an exact copy of the original bad test example.

    I’m interested to see how you stub out the Query object.

    Reply
    1. taharrison Post author

      Thank you so much. You were quite right, I had a cut-and-paste fail which rather destroyed the point of my post. I’ve fixed it now – the code above is the code I intended to post.

      You raise an interesting point about how I stub out the Query being important. I hadn’t really considered the Query object as being another external dependency but in a way it is. Some programmers advocate scattering interfaces all over the codebase. If the CarService trades in Query objects, then maybe it should trade in IQuery interfaces instead. I think that kind of thinking could maybe lead to madness.

      In general I’ve seen this sort of thing in real code in terms of :
      Consumer relies on A, which returns a DTO; Consumer passes the DTO to B and then returns the result of B. Maybe I should have rewritten Fabios example further to be like that. Because the DTO might have been a struct… or a POCO class… or a primitive. In any case the test structure where

      Arrange: A returns BAR when passed FOO; B returns BAZ when passed BAR
      Act: I pass FOO to my service
      Assert: my service should return BAZ

      relies on stubs and feels less invasive and brittle than the mock version:

      Arrange: A returns BAR when passed FOO
      Act: I pass FOO to my service
      Assert: A should have been called with FOO; B should have been called with BAR

      Reply

Leave a Reply

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