Skip to main content

Adapter pattern revised

In my article about adapter pattern I have written an adapter that allows to access Ehcache cache instances through a simple Map interface. The basic idea was to write a Map implementation that actually, behind the scenes, was wrapping and hitting Ehcache. Everything looked great but Zach Bailey found a bug in my implementation – or more precisely – lack of functionality. He even provided a test case proving he is right. And sadly, he was ;-). Thank you Zach!

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!

Comments

  1. Using TestNG's @DataProvider annotation You could get rid of 'templated' subclasses. :)

    @Test( dataProvider = "map-provider" )
    public class MapTest
    {

    @DataProvider( name = "map-provider" )
    public Object[][] mapProvider()
    {
    return new Object[][] { { new HashMap() }, { new ConcurrentHashMap() },
    { new TreeMap() } };
    }

    public void verifySomething( Map map )
    {
    assertNotNull( map );
    }

    public void verifyOtherThing( Map map )
    {
    assertNotNull( map );
    }
    }

    ReplyDelete
  2. Actually, JUnit has a similar feature, which I have already written about (in Polish, sorry). But I must admit TestNG deals with this case much better by injecting arguments directly to test method, very nice!

    Let's just say I used JUnit to have an opportunity to present Template Method pattern :-). Though TestNG looks promising, why I've never used it?

    ReplyDelete
  3. Of course. :) Template method is very easy to adapt pattern.

    Btw.. don't worry, I live actually abroad in Silesia nonetheless Polish isn't foregin to me. ;)

    PS. JUnit's @Parameters have one-to-one eqiuvalent in TestNG using @Factory.

    ReplyDelete
  4. Tomek,

    I recommend you give up hamcrest in your unit tests and check out tiny-project fest-assert[*]. With this library on your classpath you will be able to replace this, not very nice looking and pretty complicated code:

    assertThat(set.size(), equalTo(1));
    assertThat(set, not(hasItem("two")));
    assertThat(set, hasItem("three"));

    to really nice (for me) DSL

    assertThat(set).hasOnly("three");

    [*]http://fest.easytesting.org/assert/wiki/pmwiki.php

    ReplyDelete
  5. also I find it very weird that in every test method you are testing if your "given" block works correctly.

    I have never sow something like that and IMHO if you have assertions in "when" block thats definitely shows you need more tests methods. but you even have those functionalities checked in different tests so all those assertions are just redundant.

    ReplyDelete
  6. Thank you for your comments. I have heard about fest but after not finding it in main m2 repository I forgot about the project. Thanks to your link I found their own repo so I will definitely try it.

    As for assertions in given block: it is a sort of fail-fast mechanism. If your tests are green it's great. If one or two tests fail, it's still great, because you can locate and reproduce bug quickly. But what if, let's say, 50 tests fail? Where to start, which test failure is the root of all evil? As far as I know in TestNG you can set dependencies on tests, so if one test fails, all tests depending on failing functionality aren't even invoked. There is no such mechanism in JUnit. So, for the sake of simplicity, I check whether my setup code worked properly in order to spot mistakes earlier.

    Surely when is not the right place for assertions, but, IMHO, it is better to make this little exception and find the cause of red JUnit bar faster. Even for the price of code duplication. Or, maybe it simply time to give TestNG a chance, as suggested above?

    ReplyDelete
  7. regarding fest-assert - remember that you can always put jars into your local repo - it is really simply and maven prints necessary command for you. Just declare dependency to non existing library in your pom.xml and execute build - in error message you will find almost whole command to do this. It is even simpler when one uses Nexus. (I am pretty sure that you know this ;))

    regarding tests - I disagree with you. In our project that last for 8 months now we have around 1500 unit and integration tests - and we do not "when checking" in any of them. Still we almost always know where the problem is when test is failing - We have junit/fest-assert messages for that. Don't we? But in those rear situations when we do not know - there is debugging.

    Of course I see your arguments, but still I find it little officiousness ;) It is not only redundant but also makes tests more complicated/less readable.

    But still it is great to see that more and more people finds TDD as useful (maybe even inseparable?) programing practice.

    ReplyDelete

Post a Comment