Your unit tests are probably slowing you down

If you are looking at this post, you are probably interested in test driven development, or at the very least unit testing.

TDD, unit testing, and other levels of testing, are, in my mind, just a set tools we can use to help us write better software quicker.

I’m not going to come down hard on people and say thou shalt only write code if there’s already a failing test. There are many times writing tests first is the wrong thing to do.

That said, I think TDD is a tool that is often under used.

I think it’s under used for one reason. And, it’s the same reason I see so many unit tests that are actually harming your development not helping it. These are tests that at surface level seem like they are good tests, adding value, but on closer inspection are actually hurting your development, and just storing up problems for you, and actually even in the short to medium term, slowing your team down.

So, what is this reason?

Well, I believe it’s that for the most part, most developers, even some of the great developers out there, are actually approaching their tests with the wrong mindset, thinking about their tests in the wrong way.

And, by the way, I don’t blame them for this.

In the world of information overload that we live in, I see so many well intentioned blog posts, describing TDD, unit testing, or even just software development in a way that is overly simplified. The problem with this, is that people then come away with the impression that this simplified view of the world covers everything they need to know. That’s all there is to doing TDD.

It’s not helped by the fact that some of the downsides to the tests that most people end up with aren’t actually seen until a little further down the road, when they try to build upon or refactor the functionality they have ended up with.

Then they are in a world of pain, usually with large amounts of tests failing, no one knowing whether the tests or the code is wrong, and spurious bugs because of this.

The developers that I have seen though, that have learnt themselves, or through others like myself coaching them to adopt the right mindset when approaching TDD, are able to work quicker both in the short term and the long term, where tests speed up their work, not slow it down, avoiding problems of brittle tests failing for unknown reasons.

So, if mindset is the important, then what is the wrong mindset, and how should you think about tests instead?

So, I want to describe this by showing an example. This is taken from an open source project that I won’t name, because I don’t want to make it sound like I’m being critical of the person that created it. As I said before, there’s so much information out there that tells us that this is all we should do.

The code that this is testing is from a TODO list application, something that’s often used as an example application because there’s enough complexity there to be interesting, but not too much.

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

 

So looking at this test, at first glance, it might seem OK. It’s testing that we can PUT a new task. It’s a pretty small test, so we can read it relatively easily.

The test name is ok. Again, it seems to indicate this test is about PUTting a new task. It also ends with ‘200’ which I think we might assume is the expected response code. So, I assume from the name that this is testing the successful test case.

Aside from the name giving us a few things we have to think about or assume, there are some other problems with this test, and tests like this.

This test is more brittle than it should be, and from looking at this and other surrounding tests, I can see that the approach taken to testing, which is the most common approach I see, is one in which the developer is clearly focused on the method, or action being invoked in the test, in this case the PUT method.

Now, you might not see a problem with this, but bear with me.

Instead of focusing on the method or action being tested, I think that we should instead focus on the behaviour of the system under test, which might seem like a different way of describing the same thing. But it’s not. It is different.

Through my own experience and that of others I have coached, I have seen much better results, much more maintainable code, giving the ability to work much quicker at a higher quality when adopting a more purposeful mindset. And no, I’m not talking about BDD syntax here.

So, breaking this test down, let’s look at what it is doing line by line.

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

So, here, we are creating a PUT request. That’s great, that’s our action being tested.

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

We have some input data here – an object with a title field

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

Now, our first assertion, we are making sure that we get a 200 response back

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

Now, another assertion, this time, asserting that no error is returned, but also unfortunately losing those error details.

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

Next, we are making an additional request. This time, a GET request to retrieve the newly created task

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

Here, again, we have another assertion, making sure we get a 200 response back

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

And another again, ensuring there is no error.

it('should PUT /tasks/:id 200', function (done) {
    request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, function (err) {
      assert(err == undefined);
      request(app)
      .get('/tasks')
      .expect(200)
      .end(function (err, res) {
        assert(err == undefined);
        assert(res.body[0].title === 'foo');
        done();
      });
    });
  });

And finally, another assertion, making sure the task title is the one we sent in the PUT request.

So, from this set of assertions, it seems our assumptions about this test are correct. It is testing the success case.

The problem though, is that this test is actually testing many many things all at once. It is testing multiple behaviours in the same test method. We can see this by the number of assertions we have, but also the types of assertions.

Just because a behaviour of the system is triggered by one particular method, with a particular input, does not mean that it should be verified in the same test as a different behaviour.

We also have coupling. The way this test verifies the task has been created is via the GET method, which in turn has a couple of assertions, relating to status code etc.

If the GET method code became broken for any reason, then this test would fail, even if the code relating to task creation was working perfectly.

Taking a more behaviour oriented, purposeful approach to thinking about the PUT method would result in more tests, each with a more clearly defined purpose e.g. rather than the test we have seen, why not have

it('should return a successful response when attempting to PUT task title changes', function(done) {
request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .expect(200, done)
}

This test is only verifying the response, not any side effects.

We can then have a second test

it('should store the updated title against the given task when making a PUT request with a new task title', function(done) {
var datastore = this.datastore
var taskID = this.id
request(app)
    .put('/tasks/' + this.id)
    .send({title: 'foo'})
    .end(function () {
      datastore.updateTask.should.have.been.calledWith(taskID,{title:'foo'})
      done();
    });
}

 

This test should verify the data store interaction required to update a task’s title, probably using a fake or a mock version of the data store. It should only verify this. We don’t need to repeat assertions for the 200 response that we already have in the new test above

In addition to these tests, we probably want other test cases too, like what response should be received if we give an invalid task ID, what happens if we attempt to modify a task with no new task data etc

So, in this example, we have taken 1 test, and by taking a more behaviour oriented approach have done a couple of things.

We have split it into 2 tests, to decouple verifying our different behaviours

1 – the HTTP handling and response generation
2 – the interaction with the data store, the business logic if you like of updating the task

We have also decoupled the test relating to PUTting tasks from the GET request. We no longer need to make a GET request to verify our behaviour.

These changes might seem subtle, but shifting your approach and mindset in this way, will lead to test improvements in terms of having more obvious, clearly defined tests that are not as brittle, leading to a more maintainable system going forward.

I go into much more detail of this technique, and other principles and strategies you can apply to your own unit testing and test driven development in my new 6 week course, TDD Made Easy.

Leave a Reply