Mocking Concrete Classes in Java: Not Recommended

javajunitmockingobject-orientedunit testing

I've just read an excerpt of "Growing Object-Oriented Software" book which explains some reasons why mocking concrete class is not recommended.

Here some sample code of a unit-test for the MusicCentre class:

public class MusicCentreTest {
  @Test public void startsCdPlayerAtTimeRequested() {
    final MutableTime scheduledTime = new MutableTime();
    CdPlayer player = new CdPlayer() { 
      @Override 
      public void scheduleToStartAt(Time startTime) {
        scheduledTime.set(startTime);
      }
    }

    MusicCentre centre = new MusicCentre(player);
    centre.startMediaAt(LATER);

    assertEquals(LATER, scheduledTime.get());
  }
}

And his first explanation:

The problem with this approach is that it leaves the relationship
between the objects implicit. I hope we've made clear by now that the
intention of Test-Driven Development with Mock Objects is to discover
relationships between objects. If I subclass, there's nothing in the
domain code to make such a relationship visible, just methods on an
object. This makes it harder to see if the service that supports this
relationship might be relevant elsewhere and I'll have to do the
analysis again next time I work with the class.

I can't figure out exactly what he means when he says:

This makes it harder to see if the service that supports this
relationship might be relevant elsewhere and I'll have to do the
analysis again next time I work with the class.

I understand that the service corresponds to MusicCentre's method called startMediaAt.

What does he mean by "elsewhere"?

The complete excerpt is here:
http://www.mockobjects.com/2007/04/test-smell-mocking-concrete-classes.html

Best Answer

The author of that post is promoting the use of Interfaces over the use of member classes.

It turns out that my MusicCentre object only uses the starting and stopping methods on the CdPlayer, the rest are used by some other part of the system. I'm over-specifying my MediaCentre by requiring it to talk to a CdPlayer, what it actually needs is a ScheduledDevice.

The relationship that he's worried about re-discovering later is the fact that the MediaCentre class doesn't need all of the CdPlayer object. His claim is that by using an Interface (presumably limited to just start | stop) that it's easier to understand what the interaction really is.

"elsewhere" simply means that other objects may have similarly limited relationships, and the full member object is not required - a subset of the functionality wrapped through an Interface should be sufficient.

The claim starts to make more sense when you explode out all the potential functionality:

  • start
  • stop
  • pause
  • record
  • random play order
  • sample tracks, beginning of song
  • sample tracks, random sample of song
  • provide media information
  • ...

Now his claim of "I just need start & stop" makes more sense. Using the concrete member object instead of an Interface makes it less clear to future developers as to what is really required. Running unit tests from MediaCentre on all the other functions within CdPlayer is a waste of testing effort since they belong to the "don't care" state. If the Record function wasn't working in this case, we truly don't care since it isn't required. But a future maintainer wouldn't necessarily know that based upon the code, as written.

Ultimately, the author's premise is to use only what is needed and make it clear to future maintainers what was required before. The goal is to minimize rework / reanalyzing the module of code during subsequent maintenance.

Related Topic