The problem was with three map methods: keySet(), entrySet() and values(). If you read carefully their API you’ll find out that all these methods should return an "interactive" view backed by the underlying map so that changing the view is automatically reflected in the map that returned that view and vice-versa. For example, if you remove an item from the set returned by keySet(), corresponding map entry with this key value should be also removed. Unfortunately, in my implementation these methods simply returned independent copies holding current state (snapshots) of the map. To make matters worse, those were not immutable collections (see Collections#unmodifiableSet) so when user modified them, no errors were issued but also source map remained untouched, effectively hiding the bug.
As I said, I already have a unit test failing on my Map implementation. I extended the test case and created more complex test set (but still not complete!) Look at the signatures, I hope they are self-describing:
package com.blogspot.nurkiewicz.ehcache;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import org.junit.Before;
import org.junit.Test;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.collection.IsMapContaining.hasEntry;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
public class MapTest {
private Map<String, String> map;
@Before
public void setupTestMap() {
map = //...
}
@Test public void emptyMapShouldReturnEmptyKeySet() {/**/}
@Test public void mapWithSingleEntryShouldReturnKeySetWithSingleItem() {/**/}
@Test public void mapWithMultipleEntriesShouldReturnKeySetWithMultipleItems() {/**/}
@Test public void removingOnlyItemFromKeySetShouldRemoveFromMap() {/**/}
@Test public void removingOnlyItemFromKeySetUsingIteratorShouldRemoveFromMap() {/**/}
@Test public void removingOneOfItemsFromKeySetShouldRemoveFromMap() {/**/}
@Test public void removingOneOfItemsFromKeySetUsingIteratorShouldRemoveFromMap() {/**/}
@Test public void removingNotExistingItemFromKeySetShouldNotChangeMap() {/**/}
@Test public void removingOnlyEntryFromMapShouldRemoveItemFromKeySet() {/**/}
@Test public void removingOnlyEntryFromMapUsingIteratorShouldRemoveFromKeySet() {/**/}
@Test public void removingOneOfEntriesFromMapShouldRemoveFromKeySet() {/**/}
@Test public void removingOneOfEntriesFromMapUsingIteratorShouldRemoveFromKeySet() {/**/}
@Test public void removingNotExistingEntryFromMapShouldNotChangeKeySet() {/**/}
@Test public void addingEntryToMapShouldAddItemToKeySet() {/**/}
}
But before we move on and examine our original implementation against this test let us think for a while how to set up this test case? I could simply write in setupTestMap():
final Ehcache cache = CacheManager.getInstance().getEhcache("com.blogspot.nurkiewicz.ehcache.TEST");
map = new EhcacheMapAdapter<String, String>(cache);But then I decided to test my unit tests (sic!) by running them on standard Java Map implementations: HashMap, TreeMap and ConcurrentHashMap. This is a common scenario, where you have more than one implementation of an interface and you would like to test all the implementations at once. In an ideal world (luckily, this article describes one), unit test should not depend upon particular implementation of the interface, it should rather verify whether the interface contract is satisfied, no matter which implementation is used. We want to write a single test case and pass different implementations to test them one at a time. How to do this in JUnit without copy-pasting the same tests (don’t repeat yourself!) over and over? This is my real setup code:
public abstract class MapTest {
private Map<String, String> map;
@Before
public void setupTestMap() {
map = createTestMap();
assertNotNull(map);
}
protected abstract Map<String, String> createTestMap();
//...
}Have you noticed test case class being abstract? JUnit runners (both maven-surefire-plugin and IntelliJ IDEA built in runner) are smart enough to ignore abstract test cases and run only concrete subclasses. But more importantly, when they run MapTest subclasses they include test methods (annotated with @Test) defined in abstract base class. Don’t get the idea? Look at the rest of the code:
public class HashMapTest extends MapTest {
@Override
public Map<String, String> createTestMap() {
return new HashMap<String, String>();
}
}public class ConcurrentHashMapTest extends MapTest {
@Override
public Map<String, String> createTestMap() {
return new ConcurrentHashMap<String, String>();
}
}public class TreeMapTest extends MapTest {
@Override
public Map<String, String> createTestMap() {
return new TreeMap<String, String>();
}
}public class EhcacheMapAdapterTest extends MapTest {
@Override
public Map<String, String> createTestMap() {
final Ehcache cache = CacheManager.getInstance().getEhcache("com.blogspot.nurkiewicz.ehcache.TEST");
cache.removeAll();
return new EhcacheMapAdapter<String, String>(cache);
}
}Each MapTest subclass inherits test methods from abstract base class, but providing concrete Map implementation. EhcacheMapAdapterTest is the one of our interest. BTW we’ve actually introduced Template Method design pattern! In this pattern the majority of work (algorithm) is implemented in abstract base classes, but some steps are left intentionally as abstract methods. When using this class, almost everything is done already, all you need to provide are (typically simple) implementations of abstract methods. In our case all the logic (unit tests) are gathered in base class MapTest, but the user must subclass it and implement Map factory method createTestMap(). But more on this pattern maybe later.
Going back to bug-fixing. Since we have the tests, lets run them to see where are we starting:
As you can see, all the tests passed in standard JDK implementations, but my EhcacheMapAdapter has lots to be ashamed of...
It is not even test driven development. It is rather test driven bug-fixing – somebody reports you a bug and the first thing to do is to write a unit test (since we probably missed one) that fails because of the bug. That’s the best way to reproduce the bug. When we know what is wrong, we are bug-fixing until that (and all existing) test succeeds. This has another benefit – if few months later some developer sees your code, existing unit test will help him to understand why this bug-fix has been applied and prevent from removing it.
After an hour or two all my tests were green, so I had a good starting point for optimizations or refactorings. Code is good, but making it even better won’t break anything. But I must disappoint you – or rather: give you an opportunity to enrich your test driven experiences! In the first article you have a starting code, below is the full test case source:
public abstract class MapTest {
private Map<String, String> map;
@Before
public void setupTestMap() {
map = createTestMap();
assertNotNull(map);
}
protected abstract Map<String, String> createTestMap();
@Test
public void emptyMapShouldReturnEmptyKeySet() {
//given
//when
final Set<String> set = map.keySet();
//then
assertThat(set.size(), equalTo(0));
}
@Test
public void mapWithSingleEntryShouldReturnKeySetWithSingleItem() {
//given
map.put("zero", "0");
//when
final Set<String> set = map.keySet();
//then
assertThat(set.size(), equalTo(1));
assertThat(set, hasItem("zero"));
}
@Test
public void mapWithMultipleEntriesShouldReturnKeySetWithMultipleItems() {
//given
map.put("zero", "0");
map.put("ten", "10");
map.put("hundred", "100");
//when
final Set<String> set = map.keySet();
//then
assertThat(set.size(), equalTo(3));
assertThat(set, hasItem("zero"));
assertThat(set, hasItem("ten"));
assertThat(set, hasItem("hundred"));
}
@Test
public void removingOnlyItemFromKeySetShouldRemoveFromMap() {
//given
map.put("one", "1");
assertThat(map.size(), equalTo(1));
assertThat(map, hasEntry("one", "1"));
//when
Set<String> set = map.keySet();
final boolean result = set.remove("one");
//then
assertTrue(result);
assertTrue(set.isEmpty());
assertTrue(map.isEmpty());
}
@Test
public void removingOnlyItemFromKeySetUsingIteratorShouldRemoveFromMap() {
//given
map.put("one", "1");
assertThat(map.size(), equalTo(1));
assertThat(map, hasEntry("one", "1"));
//when
Set<String> set = map.keySet();
final Iterator<String> iterator = set.iterator();
iterator.next();
iterator.remove();
//then
assertTrue(set.isEmpty());
assertTrue(map.isEmpty());
}
@Test
public void removingOneOfItemsFromKeySetShouldRemoveFromMap() {
//given
map.put("three", "3");
map.put("two", "2");
assertThat(map.size(), equalTo(2));
assertThat(map, hasEntry("two", "2"));
assertThat(map, hasEntry("three", "3"));
//when
Set<String> set = map.keySet();
final boolean resultOfRemovingOne = set.remove("one");
final boolean resultOfRemovingTwo = set.remove("two");
//then
assertFalse(resultOfRemovingOne);
assertTrue(resultOfRemovingTwo);
assertThat(set.size(), equalTo(1));
assertThat(set, not(hasItem("two")));
assertThat(set, hasItem("three"));
assertThat(map.size(), equalTo(1));
assertThat(map, not(hasEntry("two", "2")));
assertThat(map, hasEntry("three", "3"));
}
@Test
public void removingOneOfItemsFromKeySetUsingIteratorShouldRemoveFromMap() {
//given
map.put("three", "3");
map.put("two", "2");
assertThat(map.size(), equalTo(2));
assertThat(map, hasEntry("two", "2"));
assertThat(map, hasEntry("three", "3"));
//when
Set<String> set = map.keySet();
final Iterator<String> iterator = set.iterator();
final String removedKey = iterator.next();
iterator.remove();
//then
assertThat(set.size(), equalTo(1));
assertThat(set, not(hasItem(removedKey)));
assertThat(map.size(), equalTo(1));
}
@Test
public void removingNotExistingItemFromKeySetShouldNotChangeMap() {
//given
map.put("four", "4");
assertThat(map.size(), equalTo(1));
assertThat(map, hasEntry("four", "4"));
//when
Set<String> set = map.keySet();
final boolean result = set.remove("five");
//then
assertFalse(result);
assertThat(set.size(), equalTo(1));
assertThat(set, hasItem("four"));
assertThat(map.size(), equalTo(1));
assertThat(map, hasEntry("four", "4"));
}
@Test
public void removingOnlyEntryFromMapShouldRemoveItemFromKeySet() {
//given
map.put("one", "1");
assertThat(map.size(), equalTo(1));
assertThat(map, hasEntry("one", "1"));
//when
Set<String> set = map.keySet();
final String previousValue = map.remove("one");
//then
assertThat(previousValue, equalTo("1"));
assertTrue(set.isEmpty());
}
@Test
public void removingOnlyEntryFromMapUsingIteratorShouldRemoveFromKeySet() {
//given
map.put("one", "1");
assertThat(map.size(), equalTo(1));
assertThat(map, hasEntry("one", "1"));
//when
Set<String> set = map.keySet();
final Iterator<Map.Entry<String, String>> iterator = map.entrySet().iterator();
iterator.next();
iterator.remove();
//then
assertTrue(set.isEmpty());
}
@Test
public void removingOneOfEntriesFromMapShouldRemoveFromKeySet() {
//given
map.put("three", "3");
map.put("two", "2");
assertThat(map.size(), equalTo(2));
assertThat(map, hasEntry("two", "2"));
assertThat(map, hasEntry("three", "3"));
//when
Set<String> set = map.keySet();
final String previousValueForKeyOne = map.remove("one");
final String previousValueForKeyTwo = map.remove("two");
//then
assertNull(previousValueForKeyOne);
assertNotNull(previousValueForKeyTwo);
assertThat(set.size(), equalTo(1));
assertThat(set, not(hasItem("two")));
assertThat(set, hasItem("three"));
}
@Test
public void removingOneOfEntriesFromMapUsingIteratorShouldRemoveFromKeySet() {
//given
map.put("three", "3");
map.put("two", "2");
assertThat(map.size(), equalTo(2));
assertThat(map, hasEntry("two", "2"));
assertThat(map, hasEntry("three", "3"));
//when
Set<String> set = map.keySet();
final Iterator<Map.Entry<String, String>> iterator = map.entrySet().iterator();
final Map.Entry<String, String> entry = iterator.next();
iterator.remove();
//then
assertThat(set.size(), equalTo(1));
assertThat(set, not(hasItem(entry.getKey())));
}
@Test
public void removingNotExistingEntryFromMapShouldNotChangeKeySet() {
//given
map.put("four", "4");
assertThat(map.size(), equalTo(1));
assertThat(map, hasEntry("four", "4"));
//when
Set<String> set = map.keySet();
final String previousValueForKeyFive = map.remove("five");
//then
assertNull(previousValueForKeyFive);
assertThat(set.size(), equalTo(1));
assertThat(set, hasItem("four"));
}
@Test
public void addingEntryToMapShouldAddItemToKeySet() {
//given
assertThat(map.size(), equalTo(0));
//when
Set<String> set = map.keySet();
map.put("two", "2");
//then
assertThat(set.size(), equalTo(1));
assertThat(set, hasItem("two"));
}
}Try a little of TDB (test driven bug-fixing) and see how great bug-fixing can be, when unit tests tell you exactly, if you’ve done your job correctly. Happy coding!
