Test Bottlenecks

When a developer or team adopts TDD they frequently find their tests begin running too slow to get proper feedback. What is slow? If you hesitate before running the tests - it's too slow. You may find that developers don't run all the tests on their system because it takes to long, or that the continuous integration system is so slow that by the time somebody learns that their changes broke the build hours have passed and they've moved onto a new task.

On a healthy system you can run all the unit tests locally and they will complete in seconds, even as the system grows to thousands of tests. Of course at a certain point you'll need to break up the system into independent components: jar files, dlls, gems whatever your environment's packaging system is but in general 1000 tests should be able to run in a few seconds. On most systems this is not the case. There's two big reasons for this.

Your Database is not your System

When I work with teams on TDD frequently they'll tell me that they can't avoid the database in their system because "it's the entire system!" Horse hockey. Unless you program exclusively in SQL or the database language of your NoSQL system you have code that executes before and after you interact with the database. You might have stored procs, or complicated queries, but unless your primary use case is to hand the customer a database and a command prompt you have code that runs in between. What people really mean when they say this is that they either don't know how to abstract their database from their system, or that they don't trust anything works unless it hits the database which amounts to the same thing.

Fortunately this is not that hard to fix. At a macro level what we will do is take anything related to the database and wrap it in abstractions, probably objects, and then only interact with the database through abstractions. Yes plural. Tests thaj actually test the database works, such as tests for those very abstractions will hit the database and will be slow, but they will be vastly outnumber by tests that don't hit the database and are fast. If need be we can run the tests in separate builds, but it won't come to that.

This concept is easy to explain but hard to understand so let's take a look at an example.

ToDo List

For this example I've written a small ToDo list application in Express.js. I'm not an expert at the framework, to be kind, so if you aren't an expert either don't worry you won't need to be to understand this section. I'm using the Sequelize ORM and supertest to test the HTTP requests, which are reportedly the conventions amongst active Express.js users. I won't be explaining those frameworks in detail here, but I've added that to the appendix1. In depth knowledge is not needed to follow along, and it will get even easier to follow along as we refactor it to isolate dependencies.

I built this example by taking the linked Todo app and writing it Test-First. I wouldn't call it TDD since I intentionally ignored design issues caused by standard tutorial examples for supertest and Sequelize. This doesn't make the example less realistic, a few years ago I might very well have written the code this way. Let's get to it.

The Original App

Express is a MVC framework for web apps and if you're familiar with those you probably have some idea how this works. To get a todo list that looks like this:

Users: users list

Tasks: tasks list

You need routes, models, an app.js file, a public directory...remember when these were simple? The app has a list of users, and new users can be created. Click a user and you get their list of tasks, which can be added and deleted. Pretty straightforward. Before I dive into the code let me run the tests.

  14 passing (1s)


  real  0m4.260s
  user  0m0.964s
  sys 0m1.883s

5 seconds to run 14 tests? Seems steep. 1000 tests will be ...about 6 minutes. That is not acceptable. Looking deeper at the test output I see:

    post /users
    Executing (default): CREATE TABLE IF NOT EXISTS `Users` (`id` INTEGER …
    Executing (default): PRAGMA INDEX_LIST(`Users`)
    Executing (default): CREATE TABLE IF NOT EXISTS `Tasks` (`id` INTEGER …
    Executing (default): PRAGMA INDEX_LIST(`Tasks`)
    Executing (default): DELETE FROM `Users` WHERE 2=1
    POST /users 302 2.286 ms - 40
    Executing (default): INSERT INTO `Users` (`id`,`username`,`updatedAt`, …
    Executing (default): SELECT `id`, `username`, `createdAt`, `updatedAt` …
✓ creates a user

The log information actually goes much farther to the right, as each SQL query is getting logged. This is noise that makes it hard to read the tests. More importantly look at how many database queries that makes. I count 7. Let's continue to unwind and see what the code for the post route looks like. You can tell this is hitting the post route from the post /users line in the output.

router.post('/', function(req, res, next) {
  models.User.create({username: req.body.username });
  res.redirect('/users');
});

So to test two lines I make seven database queries? Jeez something looks pretty terrible. Let's look at what the test creates a user looks like:

beforeEach(function() {
  return models.sequelize.sync().then(function() {
    return models.User.destroy({truncate: true});
  });
});
...

describe("post /users", function() {
  it("creates a user", function(done) {
    request(app)
      .post("/users")
      .type('form')
      .send({'username': 'paytonrules'})
      .expect(302, /to \/users/)
      .end(function(err, res) {
        if (err) return done(err);

        models.User.findAll({}).then(function(users) {
          expect(users.length).to.be(1);
          expect(users[0].username).to.be('paytonrules');
          done();
        }).catch(function(err) {
          done(err);
        });
    });
  });
});

Now that's pretty terrible. The test begins by syncing the models with the database (so that tables will be present) then destroys any users that are in that database. This is in the beforeEach so it will happen with any test. It creates a request from the app object, which is declared before everything, then posts to "/users" with a form type and the data {'username': 'paytonrules'}. The test checks that the response redirects to users, and that there is a user present with that username. There's a bunch of error checking as well.

Tests Give Feedback

What feedback is this one test giving us? More than you might realize:

  • It demonstrates the code works, as it does create a user and does redirect.
  • It's confusing, meaning future tests will be hard to write.
  • It's filled with knowledge that is only indirectly related to the domain. Things like promises and database access.
  • It's slow. Notice how it tests the redirect and the creation of the database row. Why? Because it took forever to write one working test.

TDD zealots are fond of saying "It's not about testing!" but that's a crock. Test is right there in the title. What they really mean is "It's not just about testing your functionality, it's also about testing your design. It also doesn't replace QA, it makes their job better by removing simple defects." Of course that's not catchy. What I'm getting at is that while this test, and the other tests in this system, verify that the system is working they don't provide any of the other benefits of TDD. Not because TDD was wrong, but because I wasn't listening to the feedback the tests were giving me.

Refactoring

The first rule of refactoring is that we don't refactor without tests. Fortunately we have those. The second is we don't refactor without a good reason. This code is tested, it works, and if we never change it then it's done. Fortunately we have a feature, or rather a bug. You see almost none of this code handles errors. We need to fix that.

Bottom-Up

I'm going to start this bottom-up, dealing with the problematic database first. That's not the only way to do it, but it works for me and this book. Let's look at some of the tests for the user model again:

beforeEach(function() {
  return models.sequelize.sync().then(function() {
    return models.User.destroy({truncate: true});
  }).then(function() {
    return models.Task.destroy({truncate: true});
  });
});

...

it("has many tasks", function(done) {
  models.User.create({username: "Eric"}).then(function(createdUser) {
    createdUser.getTasks().then(function(tasks) {
      expect(tasks.length).to.be(0);
      return createdUser.createTask();
    }).then(function() {
      return createdUser.getTasks();
    }).then(function(tasks) {
      expect(tasks.length).to.be(1);
      done();
    });
  });
});

Now these is a noisy test, primarily because Sequelize uses promises, which it has to do to coexist in a Node environment. However it also has to account for every single possible thing you might do with a database. It's API is wide, because it's reasons for existing are many. Now let's take a look at one of its router specs:

describe("post /users", function() {
  it("creates a user", function(done) {
    // Create a request from the app
    request(app)
      .post("/users") // Post to the users route
      .type('form')   // with form data
      .send({'username': 'paytonrules'})  // send it these form parameters
      .expect(302, /to \/users/)          // Expect to get back a 302 (redirect) to the users page
      .end(function(err, res) {           // when the page is done
        if (err) return done(err);        // fail the test if there are errors

        //find all the users
        models.User.findAll({}).then(function(users) {
          expect(users.length).to.be(1);                    // make sure there is only one
          expect(users[0].username).to.be('paytonrules');   // named paytonrules
          done();                                           // async test is finished
        }).catch(function(err) {
          done(err);                                        // async test finshed with an error
        });
      });
  });
});

I've commented up the sample so you can see what everything does. That is a gnarly, nasty test. Let me assure you this took forever to write. I spent a ton of time reading documentation for Supertest and Sequelize, instead of actually thinking about my domain. Now to write this to make an error? Dear lord forget it. I'd probably need to mock out the user model, and that breaks the first rule of mocking:

Don't Mock What you Don't Own

That's probably a chapter on it's own. So to proceed?

Introduce Service Objects

Without changing functionality I'm going to introduce service objects to wrap the database objects in my own domain. Let's start at the user. In a new directory services at the root level we'll add an object called user named user.js.

Some things to write

  • Basic explanation of the express.js app
    • Explain Sequelize
    • Supertest
    • JADE templates maybe?
    • Explain how painful this was. You needed to learn Sequelize/Express/Supertest despite not being an expert in any of them.
    • This becomes two chapters for this example (Framework Bottlenecks)
    • Error on user.create in the routes - right now you'd have to parse HTML. YUCK!
    • Note how these tests give you feedback
      • Slow
      • Noise (all those logs)
      • Multiple Assertions (a smell)
      • Hard to write - lots of time spent debugging the requests
      • Lots of dependencies in the tests
      • Lot's of duplication in the code
      • When you take a long time to make a test pass, you end up introducing a lot of "do it later" refactorings"
      • userId vs. UserId and find({where: {userId: }}) will return everything
        • Your own wrappers can address these issues