Software craftsman, author of Software Craftsmanship: Professionalism Pragmatism Pride (http://leanpub.com/socra) and founder of the London Software Craftsmanship Community (LSCC). Sandro started coding at a very young age but just started his professional career years later, in 1996. He has worked for startups, software houses, product companies and a few international consultancy companies. Having worked as a consultant for the majority of his career, he had the opportunity to work in a good variety of projects, different languages, technologies and industries. Currently he is a director and a software craftsman at UBS Investment Bank, where he writes code, look after the quality of the applications, mentor developers and help teams around the world to get better at delivering quality software. Sandro is a DZone MVB and is not an employee of DZone and has posted 34 posts at DZone. You can read more from them at their website. View Full User Profile

Readable Tests: Separating intent from implementation

07.16.2011
| 4145 views |
  • submit to reddit

Very recently, I was working on a test class like this:

public class AnalyticsExpirationDateManagerTest extends TestCase {
 
 private static final long ONE_HOUR_TIMEOUT = 1000 * 60 * 60;
 private static final long TWO_HOUR_TIMEOUT = ONE_HOUR_TIMEOUT * 2;
  
 private Map<Parameter, Long> analyticsToTimeout;
 private long defaultTimeout;
  
 private Parameter minTimeoutParam;
 @Mock private CacheKeyImpl<Parameter> cacheKey;
 
    @Override
    protected void setUp() throws Exception {
     MockitoAnnotations.initMocks(this);
      
     this.minTimeoutParam = new Parameter("minTimeout", "type");
      
     when(cacheKey.getFirstKey()).thenReturn(minTimeoutParam);
      
     this.analyticsToTimeout = new HashMap<Parameter, Long>();
     this.defaultTimeout = 0;
    }
  
 public void
 testGetExpirationDateWhenAnalyticsToTimeoutsAndCacheKeyAreEmpty() {
  AnalyticsExpirationDateManager<Long> manager =
    new AnalyticsExpirationDateManager<Long>(analyticsToTimeout, defaultTimeout);
  Date date = manager.getExpirationDate(cacheKey, 0L);
  assertNotNull(date);
 }
  
 public void
 testGetExpirationDateWithMinimunTimeoutOfOneHour() {
  this.analyticsToTimeout.put(this.minTimeoutParam, ONE_HOUR_TIMEOUT);
  Collection<Parameter> cacheKeysWithMinTimeoutParam = new ArrayList<Parameter>();
  cacheKeysWithMinTimeoutParam.add(this.minTimeoutParam);
  when(this.cacheKey.getKeys()).thenReturn(cacheKeysWithMinTimeoutParam);
   
  AnalyticsExpirationDateManager<Long> manager =
   new AnalyticsExpirationDateManager<Long>(analyticsToTimeout, defaultTimeout);
  Date date = manager.getExpirationDate(cacheKey, 0L);
 
  assertNotNull(date);
  Calendar expirationDate = Calendar.getInstance();
  expirationDate.setTime(date);
   
  Calendar currentDate = Calendar.getInstance();
   
  // Check if expiration date is one hour ahead current date.
  int expirationDateHour = expirationDate.get(Calendar.HOUR_OF_DAY);
  int currentDateHour = currentDate.get(Calendar.HOUR_OF_DAY);
  assertTrue(expirationDateHour - currentDateHour == 1);
 }
  
 public void
 testGetExpirationDateWhenCacheKeyIsNullAndDefaultTimeoutIsOneHour() {
  CacheKeyImpl<Parameter> NULL_CACHEKEY = null;
  AnalyticsExpirationDateManager<Long> manager =
   new AnalyticsExpirationDateManager<Long>(analyticsToTimeout, ONE_HOUR_TIMEOUT);
  Date date = manager.getExpirationDate(NULL_CACHEKEY, 0L);
   
  assertNotNull(date);
  Calendar expirationDate = Calendar.getInstance();
  expirationDate.setTime(date);
   
  Calendar currentDate = Calendar.getInstance();
   
  // Check if expiration date hour is the same of current date hour.
  // When cache key is null, system date and time is returned and default timeout is not used.
  int expirationDateHour = expirationDate.get(Calendar.HOUR_OF_DAY);
  int currentDateHour = currentDate.get(Calendar.HOUR_OF_DAY);
  assertTrue(expirationDateHour - currentDateHour == 0);
 }
  
 public void
 testGetExpirationDateWithDefaultTimeout() {
  // Default timeout is used when no time out is specified.
  Collection<Parameter> cacheKeysWithoutTimeoutParam = new ArrayList<Parameter>();
  cacheKeysWithoutTimeoutParam.add(new Parameter("name", "type"));
  when(this.cacheKey.getKeys()).thenReturn(cacheKeysWithoutTimeoutParam);
 
  AnalyticsExpirationDateManager<Long> manager =
   new AnalyticsExpirationDateManager<Long>(analyticsToTimeout, ONE_HOUR_TIMEOUT);
  Date date = manager.getExpirationDate(cacheKey, 0L);
   
  assertNotNull(date);
  Calendar expirationDate = Calendar.getInstance();
  expirationDate.setTime(date);
   
  Calendar currentDate = Calendar.getInstance();
   
  // Check if expiration date is one hour ahead current date.
  int expirationDateHour = expirationDate.get(Calendar.HOUR_OF_DAY);
  int currentDateHour = currentDate.get(Calendar.HOUR_OF_DAY);
  assertTrue(expirationDateHour - currentDateHour == 1);
 }
  
 public void
 testGetExpirationDateWhenMinTimeoutIsSetAfterCreation() {
  AnalyticsExpirationDateManager<Long> manager =
   new AnalyticsExpirationDateManager<Long>(analyticsToTimeout, ONE_HOUR_TIMEOUT);
  manager.setExpirationTimeout(this.minTimeoutParam.getName(), TWO_HOUR_TIMEOUT);
   
  Date date = manager.getExpirationDate(cacheKey, 0L);
   
  assertNotNull(date);
  Calendar expirationDate = Calendar.getInstance();
  expirationDate.setTime(date);
   
  Calendar currentDate = Calendar.getInstance();
   
  // Check if expiration date is two hour ahead current date.
  int expirationDateHour = expirationDate.get(Calendar.HOUR_OF_DAY);
  int currentDateHour = currentDate.get(Calendar.HOUR_OF_DAY);
  assertTrue("Error", expirationDateHour - currentDateHour == 2);
 }
  
}

Quite frightening, isn't it? Very difficult to understand what's going on there.

The class above covers 100% of the class under test and all the tests are valid tests, in terms of what is being tested.

Problems

There are quite a few problems here:
- The intent (what) and implementation (how) are mixed, making the tests very hard to read;
- There is quite a lot of duplication among the test methods;
- There is also a bug in the test methods when comparing dates, trying to figure out how many hours one date is ahead of the other. When running these tests in the middle of the day, they work fine. If running them between 22:00hs and 00:00hs, they break. The reason is that the hour calculation does not take into consideration the day.

Making the tests more readable

Besides testing the software, tests should also be seen as documentation, where business rules are clearly specified. Since the tests here are quite messy, understanding the intention and detecting bugs can be quite difficult.

I've done quite a few refactorings to this code in order to make it more readable, always working in small steps and constantly re-running the tests after each change. I'll try to summarise my steps for clarity and brevity.

1. Fixing the hour calculation bug

One of the first things that I had to do was to fix the hour calculation bug. In order to fix the bug across all test methods, I decided to extract the hour calculation into a separate class, removing all the duplication from the test methods. Using small steps, I took the opportunity to construct this new class called DateComparator (yes, I know I suck naming classes) using some internal Domain Specific Language (DSL) techniques.

public class DateComparator {
  
 private Date origin;
 private Date target;
 private long milliseconds;
 private long unitsOfTime;
  
 private DateComparator(Date origin) {
  this.origin = origin;
 }
  
 public static DateComparator date(Date origin) {
  return new DateComparator(origin);
 }
  
 public DateComparator is(long unitsOfTime) {
  this.unitsOfTime = unitsOfTime;
  return this;
 }
  
 public DateComparator hoursAhead() {
  this.milliseconds = unitsOfTime * 60 * 60 * 1000;
  return this;
 }
  
 public static long hours(int hours) {
  return hoursInMillis(hours);
 }
  
 private static long hoursInMillis(int hours) {
  return hours * 60 * 60 * 1000;
 }
  
 public boolean from(Date date) {
  this.target = date;
  return this.checkDifference();
 }
  
 private boolean checkDifference() {
  return (origin.getTime() - target.getTime() >= this.milliseconds);
 }
}

So now, I can use it to replace the test logic in the test methods.

2. Extracting details into a super class

This step may seem a bit controversial at first, but can be an interesting approach for separating the what from how. The idea is to move tests set up, field declarations, initialisation logic, everything that is related to the test implementation (how) to a super class, leaving the test class just with the test methods (what).

Although this many not be a good OO application of the IS-A rule, I think this is a good compromise in order to achieve better readability in the test class.

NOTE: Logic can be moved to a super class, external classes (helpers, builders, etc) or both.

Here is the super class code:

public abstract class BaseTestForAnalyticsExperationDateManager extends TestCase {
 
 protected Parameter minTimeoutParam;
 @Mock protected CacheKeyImpl<Parameter> cacheKey;
 protected Date systemDate;
 protected CacheKeyImpl<Parameter> NULL_CACHEKEY = null;
 protected AnalyticsExpirationDateManager<Long> manager;
 
 @Override
 protected void setUp() throws Exception {
  MockitoAnnotations.initMocks(this);
  this.minTimeoutParam = new Parameter("minTimeout", "type");
  when(cacheKey.getFirstKey()).thenReturn(minTimeoutParam);
  this.systemDate = new Date();
 }
 
 protected void assertThat(boolean condition) {
  assertTrue(condition);
 }
  
 protected void addMinimunTimeoutToCache() {
  this.configureCacheResponse(this.minTimeoutParam);
 }
  
 protected void doNotIncludeMinimunTimeoutInCache() {
  this.configureCacheResponse(new Parameter("name", "type"));
 }
  
 private void configureCacheResponse(Parameter parameter) {
  Collection<Parameter> cacheKeysWithMinTimeoutParam = new ArrayList<Parameter>();
  cacheKeysWithMinTimeoutParam.add(parameter);
  when(this.cacheKey.getKeys()).thenReturn(cacheKeysWithMinTimeoutParam);
 }
}

 

3. Move creation and configuration of the object under test to a builder class

The construction and configuration of the AnalyticsExpirationDateManager is quite verbose and adds a lot of noise to the test. Once again I'll be using a builder class in order to make the code more readable and segregate responsibilities. Here is the builder class:

public class AnalyticsExpirationDateManagerBuilder {
  
 protected static final long ONE_HOUR = 1000 * 60 * 60;
 
 protected Parameter minTimeoutParam;
 private AnalyticsExpirationDateManager<Long> manager;
 private Map<Parameter, Long> analyticsToTimeouts = new HashMap<Parameter, Long>();
 protected long defaultTimeout = 0;
 private Long expirationTimeout;
 private Long minimunTimeout;
 
 private AnalyticsExpirationDateManagerBuilder() {
  this.minTimeoutParam = new Parameter("minTimeout", "type");
 }
  
 public static AnalyticsExpirationDateManagerBuilder aExpirationDateManager() {
  return new AnalyticsExpirationDateManagerBuilder();
 }
  
 public static long hours(int quantity) {
  return quantity * ONE_HOUR;
 }
  
 public AnalyticsExpirationDateManagerBuilder withDefaultTimeout(long milliseconds) {
  this.defaultTimeout = milliseconds;
  return this;
 }
  
 public AnalyticsExpirationDateManagerBuilder withExpirationTimeout(long milliseconds) {
  this.expirationTimeout = new Long(milliseconds);
  return this;
 }
  
 public AnalyticsExpirationDateManagerBuilder withMinimunTimeout(long milliseconds) {
  this.minimunTimeout = new Long(milliseconds);
  return this;
 }
  
 public AnalyticsExpirationDateManager<Long> build() {
  if (this.minimunTimeout != null) {
   analyticsToTimeouts.put(minTimeoutParam, minimunTimeout);
  }
  this.manager = new AnalyticsExpirationDateManager<long>(analyticsToTimeouts, defaultTimeout);
  if (this.expirationTimeout != null) {
   this.manager.setExpirationTimeout(minTimeoutParam.getName(), expirationTimeout);
  }
  return this.manager;
 }
 
}

The final version of the test class

After many small steps, that's how the test class looks like. I took the opportunity to rename the test methods as well.

import static com.mycompany.AnalyticsExpirationDateManagerBuilder.*;
import static com.mycompany.DateComparator.*;
 
public class AnalyticsExpirationDateManagerTest extends BaseTestForAnalyticsExperationDateManager {
 
 public void
 testExpirationTimeWithJustDefaultValues() {
  manager = aExpirationDateManager().build();
  Date cacheExpiration = manager.getExpirationDate(cacheKey, 0L);
  assertThat(dateOf(cacheExpiration).is(0).hoursAhead().from(systemDate));
 }
  
 public void
 testExpirationTimeWithMinimunTimeoutOfOneHour() {
     addMinimunTimeoutToCache(); 
  manager = aExpirationDateManager()
      .withMinimunTimeout(hours(1))
      .build();
  Date cacheExpiration = manager.getExpirationDate(cacheKey, 0L);
  assertThat(dateOf(cacheExpiration).is(1).hoursAhead().from(systemDate));
 }
  
 public void
 testExpirationTimeWhenCacheKeyIsNullAndDefaultTimeoutIsOneHour() {
  manager = aExpirationDateManager()
      .withDefaultTimeout(hours(1))
      .build();
  Date cacheExpiration = manager.getExpirationDate(NULL_CACHEKEY, 0L);
  // When cache key is null, system date and time is returned and default timeout is not used.
  assertThat(dateOf(cacheExpiration).is(0).hoursAhead().from(systemDate));
 }
  
 public void
 testExpirationTimeWithDefaultTimeout() {
  doNotIncludeMinimunTimeoutInCache();
  manager = aExpirationDateManager()
      .withDefaultTimeout(hours(1))
      .build();
  Date cacheExpiration = manager.getExpirationDate(cacheKey, 0L);
  assertThat(dateOf(cacheExpiration).is(1).hoursAhead().from(systemDate));
 }
  
 public void
 testExpirationTimeWhenExpirationTimeoutIsSet() {
  manager = aExpirationDateManager()
      .withDefaultTimeout(hours(1))
      .withExpirationTimeout(hours(2))
      .build();
  Date cacheExpiration = manager.getExpirationDate(cacheKey, 0L);
  // Expiration timeout has precedence over default timeout.
  assertThat(dateOf(cacheExpiration).is(2).hoursAhead().from(systemDate));
 }
  
}

Conclusion

Test classes should be easy to read. They should express intention, system behaviour, business rules. Test classes should express how the system works. They are executable requirements and specifications and should be a great source of information for any developer joining the project.

In order to achieve that, we need to try to keep our test methods divided in just 3 simple instructions.

1. Context: The state of the object being tested. Here is where we set all the attributes and mock dependencies. Using variations of the Builder pattern can greatly enhance readability.

	
manager = aExpirationDateManager()
                .withDefaultTimeout(hours(1))
                .withExpirationTimeout(hours(2))
                .build();

2. Operation: The operation being tested. Here is where the operation is invoked.
Date cacheExpiration = manager.getExpirationDate(cacheKey, 0L);

3. Assertion: Here is where you specify the behaviour expected. The more readable this part is, the better. Using DSL-style code is probably the best way to express the intent of the test.
assertThat(dateOf(cacheExpiration).is(2).hoursAhead().from(systemDate));

In this post I went backwards. I've started from a messy test class and refactored it to a more readable implementation. As many people now are doing TDD, I wanted to show how we can improve an existing test. For new tests, I would suggest that you start writing the tests following the Context >> Operation >> Assertion approach. Try writing the test code in plain English. Once the test intent is clear, start replacing the plain English text with Java internal DSL code, keeping the implementation out of the test class.

PS: The ideas for this blog post came from a few discussions I had during the Software Craftsmanship Round-table meetings promoted by the London Software Craftsmanship Community (LSCC).

From http://craftedsw.blogspot.com/2010/12/readable-tests-separating-intent-from.html

Published at DZone with permission of Sandro Mancuso, author and DZone MVB.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Tags:

Comments

John Ferguson Smart replied on Sun, 2011/07/17 - 4:49am

Nicely done. Here are a few more ideas you could consider: using JUnit 4, you might also work with the names of the tests and test classes to make them more readable too, e.g. "testExpirationTimeWhenCacheKeyIsNullAndDefaultTimeoutIsOneHour()" could be "expirationTimeWhenCacheKeyIsNullAndDefaultTimeoutShouldBeOneHour()" or even "expiration_time_when_cache_key_is_null_and_default_timeout_should_be_one_hour()", to put the focus more on the expected outcomes. The test case itself could also be renamed to give a little context, e.g. "AnalyticsExpirationDateManagerTest" could be "WhenManagingAnayticsExpirationDates". Finally, if you implemented your custom testing DSL using custom Hamcrest matchers, you would get both readable test code and readable error messages.

Sandro Mancuso replied on Mon, 2011/07/18 - 6:10pm

Hi John,

I wrote this post last year and since then I changed slightly how I'm writing my tests and I'm already following some of your suggestions (you will see in more recent posts). I've started using Hamcrest as well and I really like the way it reads.

I also liked your suggestion related to the use of underscores in the name of the tests. I saw few guys using it before. First I found it a bit weird, I mean, too different from the Java "conventions", but bit by bit I'm finding it quite nice. It definitely reads well, no doubt. Still haven't adopted it yet though. 

Thank you very much for the great tips. 

Erwin Mueller replied on Tue, 2011/07/19 - 4:19pm

How about using Groovy or any other language for tests? I'm using it for my projects and it makes the tests less verbose and more readable. For example you can name the test methods without the _:

def "expiration time when cache key is null and default timeout should be one hour"() {
}

And you can write DSL like tests, and no need to write the type of the variables anymore, etc.

Claus Polanka replied on Wed, 2011/07/27 - 10:34am

Nice article.

Where does the dateOf method come from?

 Cheers

Sirikant Noori replied on Fri, 2012/03/30 - 1:20pm

I would also say avoid usin' variable or method names that are too long

THIS:
testGetExpirationDateWhenAnalyticsToTimeoutsAndCacheKeyAreEmpty

makes me nervous. This is code, not an exercise in comprehension!

Java Exam

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.