11 April 2007

Test Smell: I need to mock an object I can't replace (without magic)

Synaesthesia One approach to reducing complexity in code is to make commonly useful objects accessible through a global name, usually implemented with a Singleton to fool oneself into thinking that its not really a global variable. The idea is that any code that needs access to a feature can just refer to it by its global name instead of receiving it as a parameter. Here's a common example:
Date now = new Date();
Under the covers, the constructor calls the singleton System and sets the new instance to the current time using System.currentTimeMillis(). This is a convenient technique, but it comes at a cost. Let's say we want to write a test like this:
@Test public void RejectsRequestsNotWithinTheSameDay() {
  receiver.acceptRequest(FIRST_REQUEST);
  // the next day
  assertFalse("too late now", receiver.acceptRequest(SECOND_REQUEST));
}
The implementation looks like this:
public boolean acceptRequest(Request request) {
  final Date now = new Date();
  if (dateOfFirstRequest == null) {
    dateOfFirstRequest = now;
   } else if (firstDateIsDifferentFrom(now)) {
    return false;
  }
  // process the request
  return true;
}
where dateOfFirstRequest is a field and firstDateIsDifferentFrom is a helper method that hides the unpleasantness of working with the Java date library. To test this timeout I must either make the test wait overnight or do something clever, perhaps with Aspects or byte-code manipulation, to intercept the constructor and return suitable Date values for the test. This difficulty in testing is a hint that I should change the code. To make the test easier, I need to control how Date objects are created, so I introduce a Clock and pass it into the Receiver. If I stub the Clock, the test might look like this:
@Test public void RejectsRequestsNotWithinTheSameDay() {
  Receiver receiver = new Receiver(stubClock);
  stubClock.setNextDate(TODAY);
  receiver.acceptRequest(FIRST_REQUEST);

  stubClock.setNextDate(TOMORROW);
  assertFalse("too late now", receiver.acceptRequest(SECOND_REQUEST));
}
and the implementation like this:
public boolean acceptRequest(Request request) {
  final Date now = clock.now();
  if (dateOfFirstRequest == null) {
   dateOfFirstRequest = now;
  } else if (firstDateIsDifferentFrom(now)) {
   return false;
  }
  // process the request
  return true;
}
Now I can test the Receiver without any special tricks. More importantly, however, I've made it obvious that a Receiver is dependent on time, I can't even create one without a Clock. One could argue that this is breaking encapsulation by exposing the internals of a Receiver, I should be able to just create an instance and not worry, but personally I want to know about this dependency — especially when the service is rolled out across timezones and New York and London start complaining about different results.

From procedural code to objects

The introduction of a Clock suggests to me that I've been missing a concept in my code: date checking in terms of my domain. A Receiver doesn't need to know all the details of a Calendar system, such as time zones and locales, it just need to know whether, for this application, the date has changed. There's a clue in the fragment:
firstDateIsDifferentFrom(now)
which means that I've had to wrap up some date manipulation code in the Receiver. It's the wrong object, that kind of work should be done by the Clock. I'll write the test again (in jMock2 syntax):
@Test public void RejectsRequestsNotWithinTheSameDay() {
  Receiver receiver = new Receiver(clock);
  context.checking(new Expectations() {{
   allowing(clock).now(); will(returnValue(NOW));
   one(clock).dayHasChangedFrom(NOW); will(returnValue(false));
  }});

  receiver.acceptRequest(FIRST_REQUEST);
  assertFalse("too late now", receiver.acceptRequest(SECOND_REQUEST));
}
The implementation looks like this:
public boolean acceptRequest(Request request) {
  if (dateOfFirstRequest == null) {
   dateOfFirstRequest = clock.now();
  } else if (clock.dayHasChangedFrom(dateOfFirstRequest)) {
   return false;
  }
  // process the request
  return true;
}
This version of Receiver is more focussed, it doesn't need to know how to distinguish one date from another and it only needs to get a date to set the first value. The Clock interface defines exactly those date services a Receiver needs from its environment.

But I think I can push this further. The Receiver only stores a date so it can detect a change of day, perhaps I should delegate all the date functionality to another object which, for want of a better name, I'll call a SameDayChecker.

@Test public void RejectsRequestsOutsideAllowedPeriod() {
  Receiver receiver = new Receiver(sameDayChecker);
  context.checking(new Expectations() {{
    allowing(sameDayChecker).hasExpired(); will(returnValue(false));
  }});

  assertFalse("too late now", receiver.acceptRequest(REQUEST));
}
With an implementation like this:
public boolean acceptRequest(Request request) {
  if (sameDayChecker.hasExpired()) {
   return false;
  }
  // process the request
  return true;
}
All the logic about dates has been separated out from the Receiver, which can concentrate on processing the request. With two objects I can make sure that each behaviour (date checking and request processing) is unit tested clearly.

Implicit dependencies are still dependencies

I can hide a dependency from the caller of a component by using a global to bypass encapsulation, but that doesn't make the dependency go away, it just makes it inaccessible. For example, I once had to work with a .Net library that could not be loaded without installing ActiveDirectory — which I couldn't do on my machine and wasn't actually required for the features that I wanted to use. The author was trying to be helpful and make the library "just work", but the result was that I couldn't even try it out.

The point about Objects, as a technique for structuring code, is that the boundaries of an object should be clearly visible. An object should only deal with values and instances that are either local, created and managed within its scope, or passed in explicitly.

In this example, the act of making date checking testable forced me to make the Receiver's requirements more explicit and then to think more clearly about its structure.

5 comments:

Neil said...

Nice points Steve.

At somepoint you have to decide to stop though right? A SamDayChecker is rather fine grained. If your tests are all passing and your happy I guess it would be tempting to stop at clock...

Steve Freeman said...

At some level it becomes a matter of context and taste.

I might pull out the SameDayChecker if the logic was a bit messy (timezones, daylight savings, etc). Also, it makes the date representation completely opaque to the receiver which might be a good thing.

Alternatively, I might chain a series of handlers, of which one would check the date before passing on the request.

It all depends...

Colin Jack said...

I can definitely see why you'd extract the Clock/SameDayChecker and they have improved the design.

However I'd be in two minds about whether to inject the object or just construct it when needed (at least initially).

You might need to replace one SameDayChecker with another but is it really likely?

Steve Freeman said...

What's the problem with injecting a SameDayChecker? There's a dependency on an external construct, so make it explicit. You can always package up the composition in some helper thing.

If you don't then you have to do something magical which means that the dependency is still there (you've just jumped through some hoops in the test because of it) but the code now pretends that it doesn't exist.

Now I look at it, this is a nice example of why Magic Substitution is such a bad idea. The process of avoiding the dependency on system time forced me into finding a more meaningful construct. If I'd just found a way of manipulating system time, I'd have settled for a weaker design.

Colin Jack said...

"If you don't then you have to do something magical which means that the dependency is still there (you've just jumped through some hoops in the test because of it) but the code now pretends that it doesn't exist. "

The active directory example is one where you obviously need to inject and the date time is one you could argue for as well.

Equally I don't think having a class depend on SameDateChecker that in turn depends on Date/DateTime is bad design.

"The process of avoiding the dependency on system time forced me into finding a more meaningful construct. If I'd just found a way of manipulating system time, I'd have settled for a weaker design."

I'm surprised you say that, I wouldn't have. I think the Clock idea is a good one and its the sort of design I go for, making it injectable though is another matter.