Are You Using Mocks When You Should Be Using Test Data Builders?

I want to show what happens when you use mock objects when you should be using test data builders.

This post is a follow-up to a question that came up during a presentation that Jason and I gave at the March XP Toronto meeting.

We want to test the addMember() method of the CareTeamMembershipService. This method makes a Patient a member of a CareTeam.



As you can see from the implementation below there are two business rules to enforce: (1) patients can only be added to care teams at facilities where the patient is registered and (2) patients have to meet the care team membership criteria.

public void addMember(Patient patient, CareTeam careTeam) {
  if (!patient.isRegisteredAt(careTeam.getFacility())) {
    throw new CareTeamAdminException();
  }
    
  if (!patient.meets(careTeam.getMembershipCriteria())) {
    throw new CareTeamAdminException();
  }
    
  careTeamMembershipDao.create(patient.getId(), careTeam.getId()); 
}

Here is the test for the addMember() method using mock objects and test data builders:

public void should_permit_add_for_appropriate_care_team() {
  final Facility jacobi = aFacility().build();
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
 
  context.checking(new Expectations() {{
    one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
 
  sut.addMember(patient, careTeam);
}

Now that I'm quite used to jMock and test data builder syntax, I find that this test reads like a specification.

During our presentation there was one very interesting question from the audience that was never answered directly: Could you use a Mock instead of a Test Data Builder for the Facility instance?

How about we try right now and see what happens?

Use a mock for the jacobi Facility instance.

public void should_permit_add_for_appropriate_care_team() {
  final Facility jacobi = context.mock(Facility.class);
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
 
  context.checking(new Expectations() {{
    one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
 
  sut.addMember(patient, careTeam);
}

The test fails because Facility is not an interface.

One solution is to create interfaces for all your domain objects. Yech. Alternatively the jMock framework will let you mock concrete and abstract classes although it is discouraged. To enable this feature you have to configure the context in a special way.

Use a ClassImposteriser mockery.

final Mockery context = new JUnit4Mockery() {{
  setImposteriser(ClassImposteriser.INSTANCE); 
}};

Now the test fails because of an unexpected invocation: facility.register().

Add expectations on the facility mock object.

public void should_permit_add_for_appropriate_care_team() throws Exception {
  final Facility jacobi = context.mock(Facility.class);
    
  context.checking(new Expectations() {{
    one(jacobi).register(with(any(Patient.class)));
    one(jacobi).register(with(any(CareTeam.class)));
  }});
    
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
    
  context.checking(new Expectations() {{
    one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
    
  sut.addMember(patient, careTeam);
}

This fix is not so simple because we have to put both of the register expectations before we've actually created our patient and careTeam. We also have to specify the expected arguments with matchers rather than the actual arguments.

Now the test fails because of an unexpected invocation: facility.getPatients().

Add another expectation on the facility mock object.

public void should_permit_add_for_appropriate_care_team() throws Exception {
  final Facility jacobi = context.mock(Facility.class);
    
  context.checking(new Expectations() {{
    one(jacobi).register(with(any(Patient.class)));
    one(jacobi).register(with(any(CareTeam.class)));
  }});
    
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
    
  context.checking(new Expectations() {{
    one(jacobi).getPatients(); will(returnValue(singleton(patient)));
    one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
    
  sut.addMember(patient, careTeam);
}

Now the test finally passes. However, I find it way less readable as a specification. Why does the specification for the addMember() function require expectations on the Facility instance?

Here is the same test where all the test data builders have been replaced by mock objects.

public void should_permit_add_for_appropriate_care_team() throws Exception {
  final Facility jacobi = context.mock(Facility.class);
  final Patient patient = context.mock(Patient.class);
  final CareTeam careTeam = context.mock(CareTeam.class);
  final MembershipCriteria membershipCriteria = context.mock(
                                  MembershipCriteria.class);
    
  context.checking(new Expectations() {{
    one(patient).getId(); will(returnValue("id1"));
    one(careTeam).getId(); will(returnValue("id2"));
    one(careTeam).getFacility(); will(returnValue(jacobi));
    one(patient).isRegisteredAt(jacobi); will(returnValue(true));
    one(careTeam).getMembershipCriteria(); will(returnValue(membershipCriteria));
    one(patient).meets(membershipCriteria); will(returnValue(true));
    one(careTeamMembershipDao).create("id1", "id2");
  }});
    
  sut.addMember(patient, careTeam);
}

This is what my tests looked like when I used mock objects without test data builders.

I find it really hard to separate the expectations that have to do with the system under test from the expectations that have to do with the data setup. I also have to know lots of details about the internals of the depended-on components: patient, careTeam, membershipCriteria, and facility.

So always use mock objects with test data builders and your tests can look like this:

public void should_permit_add_for_appropriate_care_team() {
  final Facility jacobi = aFacility().build();
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
 
  context.checking(new Expectations() {{
 one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
 
  sut.addMember(patient, careTeam);
}

Sweet.

Presentation at XP Toronto

At the March XP Toronto meeting, Jason Cheong-Kee-You and I gave this presentation, Struggling to Create Maintainable Unit Tests?

At the start of the presentation we asked our audience if they where using:

• a unit testing framework
• any Mock Object framework
• Test Data Builders

We also asked if they had read xUnit Test Patterns.

It turned out that essentially everyone was using a unit testing framework and most everyone in the audience had had some exposure to a mocking framework. Very few people had read xUnit Test Patterns. No one was sure about Test Data Builders.

It was the perfect audience for our presentation.

The takeaway from our presentation is that you need to use mock objects with Test Data Builders if you want to create maintainable unit tests. Use mock objects when protocol is important and use Test Data Builders when data is important.

Here is our call to action:

• read xUnit Test Patterns
• read Nat Pryce's blog postings on Test Data Builders
• learn to distinguish between depended-on components that are essentially protocol versus those that are essentially data
• try using mock object with Test Data Builders.

I Might Be Able to Repair Cars After All

I often tell people that in our modern world software is everywhere. I had no idea how much software was in cars. Here are two quotes from Manfred Broy in IEEE Spectrum:

[A] premium-class automobile ... probably contains close to 100 million lines of software.

The cost of software and electronics can reach 35 to 40 percent of the cost of a car ... with software development contributing about 13 to 15 percent of that cost.

When I first met Su, her Grandmother asked me, "What can you do?" I said I was a programmer. She asked me again, "No. No. What can you do with your hands? What can you make?" From her point of view I was pretty useless. Her husband was a tool and die maker and made all the cupboards in her kitchen. When he got older he made himself a wood splitter that we still use. He could certainly make things.

I never imagined that I would be able to repair a car. I might have been wrong.

Useful Habit: Put the Project and the Customer Ahead of Your Personal Interests

In I. M. Wrights's Hard Code columns, Nailing the Nominals and Green Fields Are Full of Maggots, you will find a case to watch out for the changes that are needed in your development approach when your codebase is greater than 100K LOC. I think he is correct. Of course, Eric knows that there is no hard cutoff between programming in the small and programming in the large.

I'm most interested in developing and teaching those habits that are useful all along the programming in the small to the programming in the large continuum.

Here's a habit for software engineers to start with from Nailing the Nominals: put the project and the customer ahead of your personal interests.

You Should Be Using A Static Analysis Tool

In my experience static analysis tools do not provide very much benefit. Nonetheless, you'd be crazy, dare I say unprofessional, not to be using a static analysis tool.

These days I'm coding in Java and I'm using FindBugs as my static analysis tool. There is almost no cost to installing and using FindBugs. Most importantly, FindBugs doesn't have my particular set of blind spots.

Let's see what FindBugs discovered about my current codebase, a codebase that has been in production for 4 years.

Dodgy 1: Method uses the same code for two branches.

selectField.getModel().setSelectedItem(
    theDescriptor.isActive() ? ITEM_ASSESSMENT_YES : ITEM_ASSESSMENT_YES);

I resolved the FindBugs discovery by replacing the dodgy code with:

selectField.getModel().setSelectedItem(
    theDescriptor.isActive() ? ITEM_ASSESSMENT_YES : ITEM_ASSESSMENT_NO);

This is a real bug discovered by FindBugs. In the dodgy code above, a checkbox in the UI is always selected without considering the value returned from the isActive() method. In my current codebase, FindBugs found three occurrences of the same mistake. No doubt from the essential but sometimes dangerous practice of cut-and-paste coding.

Dodgy 2: Using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||).

if (getDiseaseToMeasuresOrder() != null &
    getDiseaseToMeasuresOrder().containsKey(theDisease)) {

I resolved the FindBugs discovery by replacing the dodgy code with:

if (getDiseaseToMeasuresOrder() != null &&
    getDiseaseToMeasuresOrder().containsKey(theDisease)) {

This is a real bug discovered by FindBugs. In the dodgy code above, whenever the getDiseaseToMeasuresOrder() method returns null we'll get a NullPointerException. I'm pretty sure this bug was simply due to a typo.

Dodgy 3: Switch statement found where one case falls through to the next case.

<br />AbstractEvent<EventType> event = (AbstractEvent<EventType>) theEvent;<br />switch (event.getType()) {<br /> case Init:<br /> handleInitEvent((InitEvent) event );<br /> case Indicators:<br /> handleIndicators((IndicatorsEvent) event );<br /> break;<br />}<br />
This code looks like a ClassCastException will be thrown whenever we handle the Init event and then fall through to handle Indicators event.

Now I can either add the missing break;, or I can hunt around to confirm that no one is firing the Init event. I decided to hunt around. As it turns out, the Init event is never fired.

I resolved the FindBugs discovery by replacing the dodgy code with:<br />AbstractEvent<EventType> event = (AbstractEvent<EventType>) theEvent;<br />switch (event.getType()) {<br /> case Indicators:<br /> handleIndicators((IndicatorsEvent) event );<br /> break;<br />}<br />
As an added bonus I can remove the handleInitEvent() method. FindBugs excels at spotting neglected code that should be reworked or eliminated.

What dodgy code is hiding in your codebase? Grab yourself a copy of FindBugs and find out.

Simplicity is Hard

Simplicity is one the values of Extreme Programming. Simplicity is hard.

To achieve simplicity you need to be able recognize the presence or absence of simplicity.

Perhaps more than anything you need a strong desire for simplicity.

Once you have achieved simplicity don't be surprised when you show your code to your colleagues and they react with a Looks right. Why would you do it any other way?

While I find this kind of reaction personally disappointing, it means that I've achieved simplicity.

Refactoring to Simpler Code

Here is some code from my current project:

public boolean isAllowedToAddAdministrators() {
   boolean retVal = false;
   SecurityRole currentUsersSecurityRole =
       getUiContext().getUser().getSecurityRole();
   if (currentUsersSecurityRole.equals(SecurityRole.SYSADMIN)) {
       retVal = true;
   }
   return retVal;
}

I'm going to refactor it to this:

public boolean isAllowedToAddAdministrators() {
   return loggedInUser().isSysAdmin();
}

Did you just think to yourself Looks right. Why would you do it any other way?

Refactoring Steps

Here is that same code from my current project:

public boolean isAllowedToAddAdministrators() {
   boolean retVal = false;
   SecurityRole currentUsersSecurityRole =
       getUiContext().getUser().getSecurityRole();
   if (currentUsersSecurityRole.equals(SecurityRole.SYSADMIN)) {
       retVal = true;
   }
   return retVal;
}

The first thing I notice is that the code seems unnecessarily complicated. It's written using a structured code style that is not warranted in this case. Essentially, isAllowedToAddAdministrators() uses a condition to assign to a boolean return value. Instead, I can just return the condition.

Apply Simplify Boolean Logic.

public boolean isAllowedToAddAdministrators() {
   SecurityRole currentUsersSecurityRole =
       getUiContext().getUser().getSecurityRole();
   return currentUsersSecurityRole.equals(SecurityRole.SYSADMIN);
}

The code doesn't feel right. I often use inlining to gain a fresh perspective.

Apply Inline Temp to currentUsersSecurityRole.

public boolean isAllowedToAddAdministrators() {
   return getUiContext().getUser().getSecurityRole()
       .equals(SecurityRole.SYSADMIN);
}

The code still doesn't feel right, although, you could argue that it is as simple as it could be.

Whenever I have to read code like this I find it hard to tease out the intention: I'm thrashing about in nothing but implementation details. I think I can make it simpler by extracting some little methods.

Apply Extract Method to getUiContext().getUser() to create loggedInUser().

public boolean isAllowedToAddAdministrators() {
   return loggedInUser().getSecurityRole().equals(SecurityRole.SYSADMIN);
}

private IUser loggedInUser() {
   return getUiContext().getUser();
}

Apply Extract Method to loggedInUser().getSecurityRole().equals(SecurityRole.SYSADMIN) to create isSysAdmin().

public boolean isAllowedToAddAdministrators() {
   return isSysAdmin(loggedInUser());
}

private boolean isSysAdmin(IUser loggedInUser) {
   return loggedInUser.getSecurityRole().equals(SecurityRole.SYSADMIN);
}

private IUser loggedInUser() {
   return getUiContext().getUser();
}

Often when I see code of the form someFunction(anObject) I want to transform it to the form anObject.someFunction(). This procedural-style to object-style heuristic is telling me that I should move the isSysAdmin() method to the User class.

Apply Move Method to isSysAdmin().

public class SecurityPolicy {
  
   public boolean isAllowedToAddAdministrators() {
       return loggedInUser().isSysAdmin();
   }

   private IUser loggedInUser() {
       return getUiContext().getUser();
   }

}

public class User implements IUser {
  
   public boolean isSysAdmin() {
       return securityRole.equals(SecurityRole.SYSADMIN);
   }

}

Where did I end up?

I refactored the isAllowedToAddAdministrators() method

public class SecurityPolicy {
  
   public boolean isAllowedToAddAdministrators() {
       boolean retVal = false;
       SecurityRole currentUsersSecurityRole =
           getUiContext().getUser().getSecurityRole();
       if (currentUsersSecurityRole.equals(SecurityRole.SYSADMIN)) {
           retVal = true;
       }
       return retVal;
 }
 
}

first to

public class SecurityPolicy {
  
   public boolean isAllowedToAddAdministrators() {
       return getUiContext().getUser().getSecurityRole()
           .equals(SecurityRole.SYSADMIN);
   }
 
}

and then to

public class SecurityPolicy {
  
   public boolean isAllowedToAddAdministrators() {
       return loggedInUser().isSysAdmin();
   }

   private IUser loggedInUser() {
       return getUiContext().getUser();
   }

}

public class User implements IUser {
  
   public boolean isSysAdmin() {
       return securityRole.equals(SecurityRole.SYSADMIN);
   }

}

So, how exactly is where I ended up simpler? Why didn't I stop the refactoring after the first step?

Two Simplicity Heuristics

Two simplicity heuristics that I use are the Telephone Test and a preference for Little Methods.

To perform the Telephone Test you attempt to read your code to someone over the telephone. If the person on the other end does not understand what you are talking about, then you need to rewrite your code.

Little Methods are short methods, named by what they are intended to do, not how they do it.

The final version of isAllowedToAddAdministrators() is a Little Method that passes the Telephone Test.

public boolean isAllowedToAddAdministrators() {
 return loggedInUser().isSysAdmin();
}

I can imagine this exchange:

Q: When does the application allow administrators to be added?
A: When the logged in user is a SysAdmin.

How does the Telephone Test work for this code?

   public boolean isAllowedToAddAdministrators() {
       return getUiContext().getUser().getSecurityRole()
           .equals(SecurityRole.SYSADMIN);
 }

Q: When does the application allow administrators to be added?
A: When the UI Context user's security role is equal to SysAdmin.

Code readability is so important: my programming speed is way more limited by my ability to read and comprehend code than it is by my ability to type code.

When I created isSysAdmin() on the User class I created a Little Method:

public boolean isSysAdmin() {
 return securityRole.equals(SecurityRole.SYSADMIN);
}

Notice how when I had finished refactoring I'd created 3 single line methods. This style of coding requires well chosen names that reveal intent. Poor names will force the reader to mentally inline each of the Little Methods.

So why do I like to code with lots of well-named little methods? Because in code, as in life: If you do something and you do it well, you will be asked to do it over and over.

Focus Your Resume's Message

I don't think of my resume as a historical document that chronicles my career. My resume is a marketing device that describes my career from the point of view of the work that I want to have now and in the future.

My blog is also a marketing device that should present a point of view that will help me to get the work that I want. My blog's marketing message should agree with my resume's marketing message.

As a way of comparing my resume's and my blog's marketing message, I created a Wordle for my resume and a Wordle for my blog. To focus on the essential message I restricted each Wordle to 25 words.

My Resume's Wordle



My Blog's Wordle



My Take Away

Since I've compared the essential message of my resume to my blog, I'm thinking that my blog should present more posts about design and testing. I'm also thinking that my resume should relate my recent experience with Java and refactoring to application development and architecture.

I find it interesting that C++ showed up for my resume while Java showed up for my blog—perhaps it is time to update my resume.

The Mysterious Case of the Missing BigDecimal Constructor

My current project has recently been experiencing runtime exceptions of the form:

NoSuchMethodError: java.math.BigDecimal.<init>(I)V

These exceptions only showed up when we started creating a codebase that mixes code written for Java 1.4 and code written for Java 1.5 (we wanted our design to use generics and annotations).

Looking at the stack trace it was clear that the source of the runtime exception was construction of a BigDecimal using an int argument, for example, new BigDecimal(5). Looking over the Java API reveals that in Java 1.5 BigDecimal has a constructor that takes a double or an int argument while in Java 1.4 BigDecimal has no constructor that takes an int argument.

Working through all the permutations with a colleague we were able to create the runtime exception when we built our code using a Java 1.5 build path, compiling for Java 1.4, and running the code on a Java 1.4 JVM. It turned out that the key mistake was to use the Java 1.5 build path. This only happened on certain misconfigured developer machines. Once we reconfigured the developer machines the runtime exception was gone.

Still, I was curious and I wanted to really understand the runtime exception. I installed the Bytecode Outline plugin for Eclipse so that I could see the bytecodes generated for new BigDecimal(5). You could also use the javap command line tool. Below are the interesting fragments of bytecode.

Build Path: 1.4.2

NEW java/math/BigDecimal
DUP
LDC 5.0
INVOKESPECIAL java/math/BigDecimal.<init>(D)V

Build Path: 1.5.0

NEW java/math/BigDecimal
DUP
ICONST_5
INVOKESPECIAL java/math/BigDecimal.<init>(I)V

Now it is easy to see what is going on. When the build path is for Java 1.4 the compiler converts the int literal 5 to a double. When the build path is for Java 1.5 the compiler generates a call to a constructor that takes an int argument—a method that does not exist in the 1.4 runtime libraries.

Case closed.

If you want to know the meaning of operands such as DUP and LDC, you can read Chapter 6 of The Java Virtual Machine Specification.