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(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).
No comments:
Post a Comment