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:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
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():
1
2
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:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
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:
1
2
3
4
5
6
7
public class HashMapTest extends MapTest {
@Override
public Map<String, String> createTestMap() {
return new HashMap<String, String>();
}
}
1
2
3
4
5
6
7
public class ConcurrentHashMapTest extends MapTest {
@Override
public Map<String, String> createTestMap() {
return new ConcurrentHashMap<String, String>();
}
}
1
2
3
4
5
6
7
public class TreeMapTest extends MapTest {
@Override
public Map<String, String> createTestMap() {
return new TreeMap<String, String>();
}
}
1
2
3
4
5
6
7
8
9
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:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
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!