Unit testing, factories, and the Law of Demeter

Question

Here's how my code works. I have an object that represents the current state of something akin to a shopping cart order, stored in a 3rd party shopping API. In my controller code, I want to be able to call:

myOrder.updateQuantity(2);

In order to actually send the message to the third party, the third party also needs to know several things that are specific to THIS order, like the orderID, and the loginID, which will not change in the lifetime of the application.

So when I create myOrder originally, I inject a MessageFactory, which knows loginID. Then, when updateQuantity is called, the Order passes along orderID. The controlling code is easy to write. Another thread handles the callback and updates Order if its change was successful, or informs Order that its change failed if it was not.

The problem is testing. Because the Order object depends on a MessageFactory, and it needs MessageFactory to return actual Messages (that it calls .setOrderID() on, for example), now I have to set up very complicated MessageFactory mocks. Additionally, I don't want to kill any fairies, as "Every time a Mock returns a Mock a fairy dies."

How can I solve this problem while keeping the controller code just as simple? I read this question: https://stackoverflow.com/questions/791940/law-of-demeter-on-factory-pattern-and-dependency-injection but it didn't help because it didn't talk about the testing problem.

A few solutions I've thought of:

1. Somehow refactor the code to not require that the factory method return real objects. Perhaps it's less of a factory and more of a MessageSender?
2. Create a testing-only implementation of MessageFactory, and inject that.

---

The code is pretty involved, here's my attempt at an sscce:

public class Order implements UpdateHandler {
    private final MessageFactory factory;
    private final MessageLayer layer;

    private OrderData data;

    // Package private constructor, this should only be called by the OrderBuilder object.
    Order(OrderBuilder builder, OrderData initial) {
        this.factory = builder.getFactory();
        this.layer = builder.getLayer();
        this.data = original;
    }

    // Lots of methods like this
    public String getItemID() {
        return data.getItemID();
    }

    // Returns true if the message was placed in the outgoing network queue successfully. Doesn't block for receipt, though.
    public boolean updateQuantity(int newQuantity) {
        Message newMessage = factory.createOrderModification(messageInfo);

        // *** THIS IS THE KEY LINE ***
        // throws an NPE if factory is a mock.
        newMessage.setQuantity(newQuantity); 

        return layer.send(newMessage); 
    }

    // from interface UpdateHandler
    // gets called asynchronously
    @Override 
    public handleUpdate(OrderUpdate update) {
        messageInfo.handleUpdate(update);
    }
}

Answer 1

The major concern here is that mocks can't (or shouldn't) return mocks. This is probably good advice, but talks around a solution: return a real Message. If the Message class is well-tested and passing, you can consider it to be just as friendly as a mock. Perhaps it is even friendlier as it will respond like the real thing because it is the real thing.

What kind of real Messages can you return? Well, you can return a full-fledged real Message, a simplified real Message (wherein well-known defaults are used), or you can return a NullMessage (as in the Null Object Pattern). A NullMessage is just as valid a Message as any other, and can be dropped in anywhere else in your application. Which one to use depends on the complexity of creating and returning a full message.

As to the Law of Demeter, there are multiple concerns here. First, your constructor takes its own builder as a parameter, then extracts elements from it. This is a clear violation of Demeter, and also creates a superfluous dependency. Worse yet, the builder is acting as a mini service locator, masking the real dependencies of the class. The OrderBuilder should create these objects and pass them in as their own parameters.

In order to test this, then, you would pass in a mock MessageFactory, which returns a real Message (either full, simple, or null), and a mock MessageLayer that takes the message. If you use a full or simplified Message, you could get it back from your MessageLayer mock and inspect it for testing assertions.

I would also look at the MessageFactory and MessageLayer as a functionality clump at a different level of abstraction, and so I would extract a MessageSender class that encapsulated that functionality. You could test this class by using a simple mock MessageSender, and shift everything I talked about above into the MessageSender's tests, thereby adhering more closely to Single Responsibility as well.

---

I see there are really two questions here. There is a specific question of how to test this code, and a general question about mocks returning mocks. The specific question is what I dealt with above to a larger extent, and I have more thoughts at the end of here about it now that some more details have come to light, but there is not really a good answer yet to the general question: Why should mocks not return mocks?

The reason mocks should not return mocks is that you can end up testing your tests rather than testing your code. Instead of just making sure that the unit is fully functional, the test now depends on a whole new piece of code found only in the test case itself (which often is itself not tested). This creates two problems.

First, the test now cannot tell me for sure if the unit is broken or if the interrelated mocks are broken. The whole point of a test is to create an isolated environment where there should be only one cause for failure. A mock on its own is generally very simple and can be inspected directly for problems, but wiring multiple mocks together like this becomes exponentially harder to confirm by inspection.

The second problem is, as APIs change for the real objects, tests may start failing far away since the mocks do not automatically change as well. The Law of Demeter comes into play here, as these are exactly the type of effects following the law avoids. In my tests, I would have to worry about keeping in sync not only the mocks of direct dependencies, but also the mocks of dependencies of dependencies ad infinitum. This has the effect of shotgun surgery on the tests when classes change.

---

Now, as to the specific question of how to test this particular piece of code, let's break down some assumptions.

Question 1: What are we really testing? While this is an abbreviated portion of the code, we can see three essential activities going on here. First, we have a factory generating a Message. We aren't testing whether the factory is producing the Message, as you're already mocking that out. We're not testing the Message, as it should be tested elsewhere, presumably in a suite of tests for the third-party API that generates the Message. In the second line, we can see from inspection that the method is simply called on the Message and so there is really nothing to test in the second line. Once again, there should be tests elsewhere that make testing this redundant. The third line calls the MessageLayer API, and simply passes through the result. Once again, MessageLayer's API should already be tested elsewhere. This leaves us with essentially nothing to test. There are no direct visible side effects to the external code, and we should not be testing internal implementation. That leads us to the one conclusion that it would be inappropriate to test this code at all. (For more on this line of reasoning, see Sandi Metz's presentation Magic Tricks of Testing, [slides, video])

Question 2: Wait, so then...wha?? Yes, that's right, don't test this at all. Now, as mentioned, this is an abbreviated version of the code. If you have other logic, test that, but encapsulate this into a separate unit (like the MessageSender implementation mentioned above). You can then mock this entire aspect of the code easily, while still having the ability to test other logic.

You are basically using a third-party API directly in your code. Third-party code is notoriously hard to test because it can have these types of dependency issues you have here. Encapsulating it off into a corralled area can make it easier to test your other code, and reduce shotgun surgery if that third-party changes their code (or just changes). While there may still be pain in testing the part that interacts with the third-party API, it is limited to one small facet that you can isolate.

Answer 2

I'm going to agree with @Robert Harvey. Just to be clear: Demeter is reasonable and good programming style. It's the "no mocks from mocks" that strikes me as more of a subjective preference supporting a particular coding style rather than a generally applicable (& well justified) practice. The fairy rule takes out "fluent" interfaces like:

Thing.create("zoom").setDomain("bo.com").add(1).flip().reverse().tuneForSemantics().run();

Kind of an extreme example, but essentially the fairy rule would disallow including that class in any code, because the code would become untestable. But that's a popular paradigm in OO code.

Also, the more general problem is how to mock a factory to return a mock you want to test with. I'm generally shy of using Factories as dependencies, but sometimes it's much better than the alternative. If you end up with

ThirdPartyThing ThirdPartyFactory<ThirdPartyThing>#create()

I don't see how you can get around it. You need a mock to return the mock. So that rule kind of knocks out two really powerful OO design patterns.

I can't think of a way to work around your problem without splitting that method into 2 or 3, pushing long lines up to the client, or else creating a weird stateful wrapper class.

I'd be really interested to see what the alternative looks like.

My answer: your code is fine! Excelcior!

(actually I'm curious about alternatives)

...

Let's take an boundary case: are mocks allowed to return themselves? Technically, they are returning a mock. If not, then that knocks the GoF prototype out, and that's one of the patterns that has held up over time:

MuActor prototype = ...
...
MuActor actor = prototype.create();
actor.run();

so does the rule permit:

prototype = Mock(MuActor.class);
when(prototype.create()).thenReturn(prototype);

Also, the fairy rule pretty much prohibits the use of Monads, because they are based on operation chaining for a particular container type. You could test the Monad type, but you couldn't test code in which the Monad appears.