Let your tests tell you when your code design sucks.

Google’s testing on the toilet blog is a really great read for people who are looking at getting into test-driven development and unit testing as a development methodology. Their most recent post called “Don’t overuse mocks” makes an important point – overmocking in your tests makes your tests less readable.

For the uninitiated, “mocking” in the unit-testing world is basically where you replace an interface with a “mock”, whose behaviour can be specified at runtime. If the code that you’re trying to get under test is dependent on some behaviour from an interface provided to it, then the mocking framework effectively allows you to specify that behaviour so that you can make your code do certain things.

For example:

class Animal
{
    public:
        virtual bool feet () const = 0;
};

class Seal :
    public Animal
{
    public:
        Seal ()
        {
             makeLotsOfNoise ();
        }

    private:
        virtual bool feet () const { return false; }
};

bool canJump (Animal const &animal)
{
    return animal.feet ();
}

We want to test that canJump will return false if the animal has no feet and will return true if animal has feet. We don’t want to depend on Seal, because its constructor makes lots of noise. Also, Seal might get feet in the future and that would make our test invalid. So we use a mock:

class MockAnimal :
    public Animal
{
    public:

        MOCK_CONST_METHOD0 (feet, bool ());
}

TEST (Main, JumpIfFeet)
{
    MockAnimal animal;
    ON_CALL (animal, feet ()).WillByDefault (Return (true));
    EXPECT_TRUE (canJump (animal));
}

TEST (Main, NoJumpIfNotFeet)
{
    MockAnimal animal;
    ON_CALL (animal, feet ()).WillByDefault (Return (false));
    EXPECT_FALSE (canJump (animal));
}

The example in “Don’t overuse mocks” is a bit more involved. It involves the use of mocking to control the interaction between a CreditCardServer, TransactionManager, Transaction, Payment and a PaymentProcessor. Here’s the example of how its’ done:

public void testCreditCardIsCharged() {
  paymentProcessor = new PaymentProcessor(mockServer);
  when(mockServer.isServerAvailable()).thenReturn(true);
  when(mockServer.beginTransaction().thenReturn(mockManager);
  when(mockManager.getTransaction().thenReturn(transaction);
  when(mockServer.pay(transaction, creditCard,
                      500).thenReturn(mockPayment);
  when(mockPayment.isOverMaxBalance()).thenReturn(false);
  paymentProcessor.processPayment(creditCard, Money.dollars(500));
  verify(mockServer).pay(transaction, creditCard, 500);
}

[some of the variable names have been changed for the sake of line-wrapping]

Its quite clear that the usage of mocks in this case is used to describe a multi-step process:

  1. The server must be available (say it is)
  2. We need a TransactionManager from the server (return our mock one)
  3. We need a Transaction from the TransactionManager (return our mock one)
  4. We need a payment from the TransactionManager (return our mock one)
  5. We need to know if the payment is over balance (say it isn’t)

If all of those conditions are satisfied, then the payment should be processed on the server. The fact that we had to wriggle so much to get the paymentProcessor to call pay () results in a somewhat fragile test. If we added a new condition unrelated to what we are testing here then we also need to change the behaviour of the mocks otherwise that test will fail. This is the peril of “over-mocking” – you’ve removed a dependency on some behaviour, but you need to re-implement the rest yourself.

Testing on the Toilet’s solution to this problem is “don’t use mocking for stuff like this”.

public void testCreditCardIsCharged() {
  paymentProcessor = new PaymentProcessor(creditCardServer);
  paymentProcessor.processPayment(creditCard, Money.dollars(500));
  assertEquals(500, creditCardServer.getMostRecentCharge(creditCard));
}

And to some extent, they’d be right. But I think its treating the symptom (the test smell) as opposed to the problem (the code smell).

There are three real problems here:

  1. paymentProcessor.processPayment does too much. It needs to ask if the server is running, ask for a transaction manager, ask for a transaction, ask for a payment and ask if the payment is over balance.
  2. paymentProcessor.processPayment is also asking too much. It should be told what do rather than just being provided some interfaces and figuring out what to do.
  3. testCreditCardIsCharged is too vague – what conditions exactly are we testing that the credit card is charged? Are we just testing that it is charged? Are we testing that it is charged when the payment server is available? Are we testing that it is charged when the payment is over balance? In reality, they’re testing that a series of steps results in success rather that testing that the system behaves as expected under particular conditions.

How can we fix this?

Well, first of all, we should figure out what processPayment is doing and whether or not it can be split up. We don’t have the code, but judging by the mock’s behaviour – it does the following:

  1. Checks if the server is running
  2. Gets a payment object for the amount requested
  3. Checks if the payment is over-balance
  4. Calls pay () on the server

Steps 1 & 2 and 3 & 4 can really be split up into two separate functions. We’re going to make one method get our payment () object from the server and another method responsible paying if appropriate. So we have:

  1. Preconditions for being able to pay at all (server running, get objects)
  2. Preconditions for being able to pay the amount requested (not over balance)
  3. Postcondition (payment succeeded)

So lets do that:

public class PaymentProcessor
{
    public Transaction prepareTransaction (CreditCardServer server)
    {
        if (!server.isServerAvailable ())
            return null;

        return server.beginTransaction ().getTransaction ();
    }

    public Payment preparePayment (Transaction transaction,
                                   Money money)
    {
        return new Payment (transaction, money);
    }

    public void processPayment (CreditCardServer server,
                                Transaction transaction,
                                Payment payment)
    {
        if (!payment.isOverMaxBalance ())
            server.pay (transaction, payment);
    }

Now, we have three distinct methods which each do different things. Now we want to test them.

The first thing we want to test is that a payment which is not over-balance is paid:

public void testPaymentPaidIfNotOverBalance ()
{
    Payment payment = new Mock <Payment> ();
    when (payment, isOverMaxBalance ()).thenReturn (false);

    paymentProcessor.processPayment (mockServer,
                                     stubTransaction,
                                     payment);

    verify (mockServer, pay (stubTransaction, payment);
}

One assert done, two to go. Lets check that a non-active server does not give us a transaction to play with:

public void testNoTransactionIfServerNotOnline ()
{
    Mock <CreditCardServer> server = new Mock <CreditCardServer> ();
    assertThat (paymentProcessor.prepareTransaction (server),
                IsNull ());
}

And now make sure that we get a payment object from our transaction

public void testPaymentObtainedFromTransaction ()
{
    StubTransaction transaction;
    assertThat (paymentProcessor.preparePayment (transaction,
                                                 Money.dollars (500),
                NotNull ());
}

Our client code is going to be a little bit more involved now:

Transaction transaction = processor.prepareTransaction (server);
Payment payment = processor.preparePayment (transaction,
                                            Money.dollars (500));
processor.processPayment (server, transaction, payment);

On the other hand we now know what the process is going to be within the client, and testing PaymentProcess is not so involved anymore, because each of its parts only perform a very specific task.

Also – in the client we don’t ask for anything. We just tell the processor to prepare a transaction, prepare a payment and pay. And in the tests, we don’t have to say “yes the transaction manager is running, yes, you can have a transaction” in order to make the part of the code responsible for paying if not over balance to work correctly. It just works correctly as long as you pass it some data it expects.

The moral of the story is: If you end up with fragile tests, go and have a look at your code. The tests will tell you if your interface sucks.

About these ads

3 thoughts on “Let your tests tell you when your code design sucks.

  1. Hi, I’m the author of that article. You did a really good job analyzing it, I’m impressed by how in depth you went.

    I agree that refactoring can help reduce usage of mocks in some cases, but I don’t think your solution helps here. There are two problems:

    1) PaymentProcessor has a simple interface when it just has a single processPayment method, but you’re suggesting to make this interface more complex just for the purpose of testing. So now any time someone needs to call this method, they now need to call two more methods in order to call processPayment.

    It’s one thing to refactor internals of your code to make them more testable, but it doesn’t make sense to change interfaces in the name of testability, all you’re really doing is making your code more complex.

    Also remember that this is a relatively simple example, that method could have been much longer, in which case you’d have to add even more methods to the interface if you wanted to split it up.

    2) You’re making PaymentProcessor easy to test, but what about your code sample that calls PaymentProcessor? If you want to test that, you need to mock out three methods, and the test would be just as ugly as the one in the example.

    Refactoring can help reduce mock usage if you use it create small, simple classes that have little context. So for example, if you had to calculate tax in PaymentProcessor, you can move that logic into its own class that doesn’t know anything about the credit card server, and it would be really easy to test. You would still need to test PaymentProcessor with the tax code, but you don’t need to test every edge case of the tax code inside the PaymentProcessor test.

    1. Hey, thanks for the detailed reply! You pretty much touched on the two issues I was thinking about while writing this, and I agree with you mostly in principle and in substance.

      I think both 1 & 2 are addressable by working at a second level of abstraction – probably in the end you’d end up with something similar to PaymentProcessor.processPayment () but it would effectively do what the client code is doing now. For the clients that don’t care about the details, there’s now only one method to mock out instead of three.

      Possibly the biggest problem with my code is that PaymentProcessor has now become an interface with three tightly coupled methods operating at different levels of abstraction. prepareTransaction operates on a CreditCardServer and gives you a Transaction, getPayment operates on a Transaction and gives you a Payment and processPayment operates on a Payment and puts data into a CreditCardServer. In that sense, the PaymentProcessor interface is probably a little big.

      But maybe I didn’t really intend for it to be an interface.

      This is where one of my gripes with Java and C# generally come into play, because there are no free functions. In C++, I generally put related functions into namespaces, for example:

      namespace pp = payment::processing;

      Transaction pp::obtainTransactionFromServer (CreditCardServer const &s);
      Payment pp::createPaymentFromTransaction (Transaction const &t);
      bool pp::makePayment (CreditCardServer &, Transaction const &, Payment const &);

      I don’t think its that much of a burden on clients to call these three methods when its clear from the structure of the code that they’re to be called in sequence. If a client didn’t really care about the details though, its would be straightforward for it to wrap it up in another function.

      bool makePaymentToCreditCardOnServer (Money const &, CreditCardServer &, CreditCard const &);

      As for testing, starting to pull together components into sequences looks more and more like integration, and that would best be covered by an integration test. Perhaps that was the original intention of PaymentProcessor. In that case, using “fakes” as opposed to stubs or mocks would be a better solution. But using a fake wouldn’t be a good solution if you wanted to test some of the behavior of PaymentProcessor.

      At least in the integration test, I think one might still want to mock out CreditCardServer or at least delegate the pay () method to a mock. It feels weird to expose methods like getMostRecentCharge in the public interface). Using a mock for verification is probably a nicer way of doing that, since whether or not the pay () method was called is primarily what the test cares about.

      1. By interface, I don’t mean a Java or C# interface, I mean an API. You should be designing APIs so that they’re easy to use and easy to maintain, and testability should only come into play in your implementation.

        So maybe using three method calls instead of one may not be “much of a burden” in some cases, but there’s no doubt that this is a more complex API. This is also a simple example: does this mean you might need 10 or 20 calls for more complex code?

        So testability really means that you should be able to swap out dependencies rather than always being tied to real thing. This is where dependency injection comes into play, since it allows you to easily pass in a mock or a fake implementation instead of the real implementation. There are some good examples of making code testable here: http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/. Notice how all the changes are just to the constructor, and the rest of the class doesn’t change.

        I agree that the code sample might be better off as an integration test, and I think that’s really part of the point i was trying to make in the article: you shouldn’t force all your tests to use mocks just so you can have unit tests for everything, and in some cases you might need an integration test. Forcing everything to use mocks causes the problems that are mentioned in the article (tests become harder to understand, harder to maintain, and provide less assurance that your code is working properly).

        A fake implementation is definitely a good option to fix this problem, but in many cases you might not have one available. You can try to create your own, but this may not be easy if the API you’re trying to fake is complex. In this case, using the real thing is an option, as long as you can start a local version of it, and it’s fast enough to use in your tests (e.g. it doesn’t take longer than a few seconds to start and individual tests can still run fast).

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s