by Adam Brett

Test Smells: Counting on Spys

In my post on Test Doubles, I describe the different types of double that are available, and how each is different from the other.

In some code I've been reviewing recently, I've come across a common pattern that I would actually consider an anti-pattern, as it's hurting the overall quality of the test-suite, and hindering the teams' ability to refactor or add to their code.

The anti-pattern I'm talking about is injecting a spy in place of a dependency (or collaborator), and spying on how the consuming class calls it. The tests then make assertions on the number of times each method is called, or the parameters they're called with. It looks something like this (example simplified):

class User {
    constructor (session) {
        this.session = session;
    }

    isLoggedIn () {
        return (this.session.get('user')['id'] !== false);
    }
}

And then the test would be something like:

import Session from 'PersistentSession';
import User from 'User';

describe('User.isLoggedIn', function () {
    let session, user;

    beforeEach(function () {
        session = spyOn(Session, 'get');
        user = new User(session);
    });

    it('should check to see if the user exists in the session', function () {
        user.isLoggedIn();
        assert(session.get.calledWith('user').count()).equals(1);
    });
});

This is a test smell for a very simple reason: When you refactor your code, your tests can break. They become fragile.

Refactoring is the process of changing the implementation of something without changing the behaviour, and is one of the biggest benefits of having an automated test-suite. When you spy on a method, you're making your assertions against an implementation, and not against the behaviour, and this means you can't change the implementation without breaking your tests.

When you update your tests alongside your implementation, you have no confidence that the changes you've made work exactly as they did before. The only thing you can be sure of is that your new tests check how you think your new implementation works. That's not the same thing.

In our example, the behaviour of isLoggedIn is simple: It should return true if the user is logged in, and false if the user is not. We don't care how it does that (implementation), we just want to make sure that it always does.

As an example, asserting on the number of times a method was called will break with the following refactoring of the User class, even though the behaviour of isLoggedIn hasn't changed, just the implementation:

class User {
    constructor (session) {
        this.session = session;
    }

    getUserValue (key) {
        if (this.session.get('user') === false) {
            return null;
        }

        return this.session.get('user')[key];
    }

    isLoggedIn () {
        return (this.getUserValue('id') !== null);
    }
}

Now, a successful call to isLoggedIn will call session.get twice, instead of once. The behaviour hasn't changed, but your tests have broken.

The astute amongst you will probably now be thinking, but you don't need to call this.session.get() twice! You should just call it once and assign the result to a variable and then the tests won't fail! Yes, you could. This is a simplified example, and the point still stands.

The following changes improve the tests slightly:

import Session from 'PersistentSession';
import User from 'User';

describe('User.isLoggedIn', function () {
    let session, user;

    beforeEach(function () {
        session = spyOn(Session, 'get');
        user = new User(session);
    });

    it('should return true when a user.id is present in the session', function () {
        session.get.with('user').return({'id': 123});
        assert(user.isLoggedIn()).equals(true);
    });
});

This is most definitely an improvement, because we're testing the behaviour of the SUT (given this input, I expect that output). Technically speaking, this is now a Mock, not a Spy and sometimes this test would be okay, but we're still leaking implementation details into our tests.

This is part of the trade-off between Classical and Mockist styles. I definitely come down on the side of Classical, and avoid Mocks whenever I can, using real collaborators wherever possible. That's because I prefer testing the code as it will be used in the real world, and in addition, if a collaborator of my class changes its interface, and I'm using that interface, it should break my tests. With a mock, it wouldn't.

With that in mind, we can improve this type of test significantly by using a Fake. It looks like this:

class InMemorySession {
    constructor (data) {
        this.data = data;
    }

    set (key, value) {
        this.data[key] = value;
    }

    get (value) {
        return this.data[key];
    }
}

And then to use it in our tests:

import Session from 'InMemorySession';
import User from 'User';

describe('User.isLoggedIn', function () {
    let session, user;

    beforeEach(function () {
        let data = {'user': { 'id': 123 }};
        session = new Session(data);
        user = new User(session);
    });

    it('should return true when a user present in the session', function () {
        assert(user.isLoggedIn()).equals(true);
    });
});

Now our test has absolutely no knowledge of the internal workings of the SUT, and our tests call the SUT exactly as we're going to call it in real-life code, there is nothing here that is test specific.

Testing code as it will be consumed in the real world means testing inputs and outputs, and not caring about how it does what it does.

Spys Aren't All Bad

As a conclusion, I want to say that Spy's aren't always bad, and nor is asserting the number of times a method is called, but as with all things software development related, it's important to understand its place.

If you're testing a class that consumes an API, for example, and that API returns results across multiple pages, you probably want to make sure your getAll method calls getPage the correct number of times, that would be an acceptable use-case for a Spy, and there's an example of that in my Understanding Test Doubles post, if you want to see what it looks like.

For exclusive content, including screen-casts, videos, and early beta access to my projects, subscribe to my email list below.


I love discussion, but not blog comments. If you want to comment on what's written above, head over to twitter.