Sunday, May 29, 2011

EasyMock Issue #2: Bad Argument equals()

One of the two canonical EasyMock failure messages (along with "Expectation failure on verify" is "Unexpected method call". This failure happens between replay() and verify() whenever EasyMock sees the class-under-test making a method call on a mock object for which the test did not set up an expectation. Generally, there are two possible problems to immediately look for:
  1. A deficiency in the test, i.e. the behavior of the class-under-test is not correctly modeled during the expectation-setting phase of the test case.
  2. A bug in the class-under-test, i.e. the test is expecting the right thing but the code isn't doing it.

(Incidentally, when it turns out to be #1, programmers tend to complain about how difficult it is to write and maintain mock-object tests, and when it is instead #2, they often fail to give mock-object-testing enough credit for detecting their bug.)

But there is a third possibility, which I would argue is neither a bug in the test nor in the class-under-test, but rather a bug in a completely separate, possibly innocuous-looking class that they both use. As usual, let's look at an example.

Here is the class-under-test, CustomerController for this post (slightly modified from the one I used in the previous two posts:
public class CustomerController {
  private OrderService orderService;

  public CustomerController(OrderService orderService) {
    this.orderService = orderService;
  }
  
  public void postOrders(Order...orders) {
    orderService.postOrders(Arrays.asList(orders));
  }
}
And the test for the postOrders() method, which uses a mock OrderService:
public class TestCustomerController {
  @Test
  public void testPostOrders() {
    OrderService mockOrderService = EasyMock.createMock(OrderService.class);
    CustomerController controller = new CustomerController(mockOrderService);
    List<Order> expectedOrders = new ArrayList<Order>();
    expectedOrders.add(new Order(1));
    
    EasyMock.expect(mockOrderService.postOrders(expectedOrders)).andReturn(1);
    EasyMock.replay(mockOrderService);
    controller.postOrders(new Order(1));
  }
}
It is also important to note that we are posting Order instances:
public class Order {
  private int orderId;

  public Order(int orderId) {
    this.orderId = orderId;
  }
}
When you run this, alas, you get an error:

java.lang.AssertionError:
Unexpected method call postOrders([Order@1e4cbc4]):
postOrders([Order@b2fd8f]): expected: 1, actual: 0
at org.easymock.internal.MockInvocationHandler.invoke(MockInvocationHandler.java:45)
at org.easymock.internal.ObjectMethodsFilter.invoke(ObjectMethodsFilter.java:73)
at org.easymock.internal.ClassProxyFactory$MockMethodInterceptor.intercept(ClassProxyFactory.java:92)
at OrderService$$EnhancerByCGLIB$$6d00fba4.postOrders()
at CustomerController.postOrders(CustomerController.java:11)
at TestCustomerController.testPostOrders(TestCustomerController.java:19)

The CustomerController code is quite trivial and correct in this case. The test is less trivial but also, well, at least ought to work. So what is going on here?

When EasyMock compares a mock method call to the expectation, it checks equality on each argument. In this case, that fails because the Order instance in the expectation does not equal the Order instance in the invocation. Note that Order is not overriding equals(), and since I set the test up to use different instances, the default instance-equality check fails. This example is pretty contrived. In reality, the more common case I've seen is that there actually is an equals() override, but somebody changes the class and breaks it, in which case the test is finding a real bug (albeit not in the actual class-under-test!).

There are two ways to fix this problem. First, I could override equals() (and hashCode(), of course) if it doesn't already exist on the class in question (the case with Order) or fix equals() if it were already there. That solution is probably more robust. The quick-and-dirty fix is to make the expectation and the invocation use the exact same instance(s). I will actually show the code for that:
public class TestCustomerController {
  @Test
  public void testPostOrders() {
    OrderService mockOrderService = EasyMock.createMock(OrderService.class);
    CustomerController controller = new CustomerController(mockOrderService);
    List<Order> expectedOrders = new ArrayList<Order>();
    
    // I will use this here AND below...
    Order order = new Order(1);
    expectedOrders.add(order);
    
    EasyMock.expect(mockOrderService.postOrders(expectedOrders)).andReturn(1);
    EasyMock.replay(mockOrderService);
    controller.postOrders(order);
  }
}
Sorry if I've rambled on for too long about something that is actually quite obvious. But I have made and seen others make this type of error enough times that I can hopefully save someone some time. Next time, I will discuss an EasyMock limitation that might impact your interface design.

Saturday, April 30, 2011

EasyMock Issue #1: Missing Behavior Definition

I am planning to do a series of posts (including examples) on issues that people (myself included) run into while using EasyMock. The first of these involves EasyMock throwing an IllegalStateException "missing behavior definition for the preceding method call". The executive summary is that this is caused by the omission of a return value specification (andReturn(...)). If that is all you need to know, thanks for coming. If you would like to see a specific example, keep reading.

Let's say we are writing unit tests for a CustomerController class that uses an OrderService class to post a Customer's Orders:

// OrderService.java
public class OrderService {
  public int postOrders(List<Order> orders) {
    return orders.size();
  }
}

// CustomerController.java
public class CustomerController {
  private OrderService orderService;

  public CustomerController(OrderService orderService) {
    this.orderService = orderService;
  }
  
  public void postOrders(Integer...orderIds) {
    List<Order> orderList = new ArrayList<Order>();
    for (int orderId : orderIds) {
      orderList.add(new Order(orderId));
    }
    orderService.postOrders(orderList);
  }
}

In the real world, OrderService probably connects to external resources and therefore we want to mock it when creating a unit test for CustomerController. The interaction between CustomerController and OrderService consists of a single call to the OrderService.postOrders(List) method. Simple enough, here is our first attempt at writing this test:

public class TestCustomerController {
  @Test
  public void testPostOrders() {
    OrderService mockOrderService = EasyMock.createMock(OrderService.class);
    CustomerController controller = new CustomerController(mockOrderService);
    List<Order> expectedOrders = new ArrayList<Order>();
    expectedOrders.add(new Order(1));
    
    // You may do the following assuming postOrders returns void:
    mockOrderService.postOrders(expectedOrders);
    EasyMock.expectLastCall();
    
    // But unfortunately it doesn't.
    EasyMock.replay(mockOrderService);
    controller.postOrders(1);
  }
}

When you run this, it blows up with the following exception:
java.lang.IllegalStateException: missing behavior definition for the preceding method call postOrders([Order@82c01f])
 at org.easymock.internal.MocksControl.replay(MocksControl.java:174)
 at org.easymock.EasyMock.replay(EasyMock.java:1970)
 at TestCustomerController.testPostOrders(TestCustomerController.java:22)
Since I gave away the punch line in the opening paragraph of this post, you know that this happens because OrderService.postOrders() is not a void method; rather, it returns an int. I would argue that this is a pretty easy mistake to make, though, because the implementation of CustomerController does not care about or capture this return value. So unless we're paying pretty close attention to the OrderService interface, we might assume it returns void. And unfortunately, EasyMock requires that ALL non-void method calls on mocks have return values specified, even if the class under test will ignore it.

So the correct way to set up the OrderService.postOrders() expectation is as follows:

EasyMock.expect(mockOrderService.postOrders(expectedOrders)).andReturn(1);

In my next "EasyMock Issues" post, I will talk about a couple of things to look for when your expectations are mysteriously failing to match.

Saturday, April 16, 2011

The Law of Demeter Makes You Create Mock-Friendly Objects

One of the reasons why I haven't been blogging is that much of the time, I have been busy with teaching -- specifically, as an adjunct in the Master of Software Engineering program at Seattle University (of which I am also an alumnus).
The last two winters, I have taught software design and modeling. Something that I touch on with my students is the Law of Demeter. The LoD states (among a couple of other things) that a software object in an object-oriented program should only talk to its "immediate friends". That rules out long method call chains like this:

customer.getOrders().get(0).getLineItems().get(0).getProduct().getQuantity()

This is bad for at least two obvious reasons. First, the objects in this program are tightly-coupled, and changes to the object model will ripple through the system in annoying ways. Second, if you write this in a Java or C#-like language, rest assured that eventually you will run into a situation where something in this chain is null, and worst of all, the ensuing NullPointerException will not pinpoint exactly what it is. So don't write code like this.

But the reason I'm writing this post is to draw attention to a third disadvantage of long method call chains: They complicate mock object testing.

Let's look at a slightly less pathological example based on the following classes:

public class CustomerController {
  private Customer customer;
  public CustomerController(Customer customer) {
    this.customer = customer;
  }
  public void postOrders() {
    customer.getOrdersList().postOrders();
  }
}
public class Customer {
  private OrdersList ordersList = new OrdersList();
  public OrdersList getOrdersList() {
    return ordersList;
  }
  public void postOrders() {
    ordersList.postOrders();
  }
}
public class OrdersList {
  private List orders = new ArrayList();
  public void postOrders() {
    for (Order order : orders) {
      order.post();
    }
  }
}
public class Order {
  public void post() {
    System.out.println("Posting order");
  }
}

We have a CustomerController that controls (what else?) a Customer and triggers the posting of the customer's Orders. Forget the unlikeliness of this particular design. (In reality, the Customer, OrderList, and Order classes would probably be pure-data classes, and CustomerController would use a service class to post orders.) Let's think about how we would test CustomerController.

Presuming that the Order.post() method ultimately makes external connections, we want to mock the Customer.postOrders() method and simply verify that CustomerController calls it. If we try to write the test this way:

@Test
public void testBadPostOrders() {
  Customer customer = EasyMock.createMock(Customer.class);
  // Next line: NullPointerException!
  customer.getOrdersList().postOrders();
  EasyMock.expectLastCall();
  EasyMock.replay(customer);
  CustomerController controller = new CustomerController(customer);
  controller.postOrders();
  EasyMock.verify(customer);
}

We should have known better than to try to call customer.getOrdersList().postOrders() on an EasyMock mock. To fix this, we could add a mock OrderList, return it from an expected call to customer.getOrderList() on the Customer mock, and create an  orderList.postOrders() expectation. But doing that type of thing will rapidly become annoying. And this is a very simple example.

We can move toward a better solution by noting that in this case, the test itself (which you can think of as client code of the Customer class) is not following the Law of Demeter: It is talking to the OrderList, which is not an "immediate friend". Yet, from the perspective of the test, the Customer class doesn't leave it a lot of choice.

Fortunately, we can refactor the Customer and CustomerController classes to make them more mock-testable. We can add a postOrders() helper method to the Customer class:

public class Customer {
  private OrdersList ordersList = new OrdersList();
  public void postOrders() {
    ordersList.postOrders();
  }
}

And we can change CustomerController to call it:

public class CustomerController {
  private Customer customer;
  public CustomerController(Customer customer) {
    this.customer = customer;
  }
  public void postOrders() {
    customer.postOrders();
  }
}
Then we can easily write a working test that uses the mock framework to verify that the controller does, in fact, post the orders:
@Test
public void testGoodPostOrders() {
  Customer customer = EasyMock.createMock(Customer.class);
  customer.postOrders();
  EasyMock.expectLastCall();
  EasyMock.replay(customer);
  CustomerController controller = new CustomerController(customer);
  controller.postOrders();
  EasyMock.verify(customer);
}

And again, as many others have pointed out, good designs and good tests feed back into one another.

Back to Blogging

Wow, it has been over a year since my last post. Clearly I have been very busy. But still, I really do want to be here sharing my thoughts about development and testing. So I'm going to try and get back into the swing. I have a couple of ideas for posts queued up. I am planning to get them done over the next couple of weeks, and we'll go from there.
One thing that I will do differently from now on is that whenever I have a useful link, recommendation, or brief thought to share, I will do it on Twitter. If you're interested, follow me there -- I am @sdmcmaster.
Until next time...