8

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);
    }
}
  • I think you're going to have to show us the relevant code you have written for the Order object and the MessageFactory. This is a good description, but it's a bit abstract to address directly with a clear answer. – Robert Harvey Mar 14 '14 at 22:49
  • @RobertHarvey I hope that update helps. – durron597 Mar 14 '14 at 23:14
  • Are you trying to verify that layer.send sends the message, or that it sends the correct message? – Robert Harvey Mar 14 '14 at 23:15
  • @RobertHarvey I was thinking about calling verify(messageMock).setQuantity(2), and verify(layer).send(messageMock); Also the the updateQuantity should return false if the Order already has a pending update, but I omitted that code for sscce reasons. – durron597 Mar 14 '14 at 23:18
  • 3
    I'll have to look at your code some more... But for the record, I don't consider returning a mock to be that big of a deal. The complexity of mock-based testing is an unavoidable consequence of dealing with mutable objects, and since a mock has no bearing on your final released application, I don't see how returning one kills kittens, much less fairies. – Robert Harvey Mar 14 '14 at 23:39
12

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.

  • +1 Totally agree about the constructor. Not sure that returning NullMessage is any better than returning a Mock -- seems like slipping by on a technicality. Either way the result is a "Fake". – Rob Mar 15 '14 at 6:21
  • Lol I stole the "taking own builder as argument" trick from Guava's source. – durron597 Mar 15 '14 at 15:32
  • @RobY A null object doesn't have to be a fake if you use it as part of your application as a legitimate no op. If it really is only used as a test fake, though, then another type of real Message should be used. – cbojar Mar 15 '14 at 16:45
  • @cbojar Granted, but Optional/Maybe is emerging as a better way to handle nullable references. If you're only writing a NullObject for unit testing, then the NullObject is really just a hand-written mock, like in the old days. The problem is, without a clear justification for the "no mocks from mocks" rule, it's hard to say whether a NullObject is bad, or why, except that it may or may not break a subjective stylistic rule. – Rob Mar 15 '14 at 20:04
  • BTW, the only real Message implementation depends on the third party API; to be able to create instances of their classes you need to start their Engine which requires, among other things, a license key... not exactly what you want in a unit test. – durron597 Mar 16 '14 at 19:59
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.

  • Fluent interfaces can still follow Demeter because fluent interfaces return the self-same object. In other words, obj.law().of().demeter(); is the same as obj.law(); obj.of(); obj.demeter();, which is perfectly acceptable. A similar explanation can be made about prototypes. – cbojar Mar 15 '14 at 16:54
  • Yes, but if they actually do work and you want to mock them out, then you run up against the "no mocks from mocks" rule. Which means that the code that uses them becomes untestable. – Rob Mar 15 '14 at 20:05
  • @cbojar oh, and I should be clear. Demeter sounds reasonable, and a 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. – Rob Mar 15 '14 at 20:36
  • By the way, in a different part of the code I do use the fluent interface. I got some ideas from this blog post, and here is the actual code I use: pastebin.com/D1GxSPsy – durron597 Mar 16 '14 at 19:57
  • That's clever. Cool! I'll give that a try in the code I'm working on now. Just one note -- a fluent interface doesn't have to return itself. There's the variation where the fluent interface returns a series of immutable instances. i.e. instead of "return this" it uses "return new Fluent(...)". However, that's transparent to the client, so it doesn't affect any of the discussion here. But it's kind of a fun fact. – Rob Mar 16 '14 at 20:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.