Recognizing Test Smells
Tonight, I was plodding through the process of getting to know MongoDB and its java driver, TDD style. This is an interesting experience, as most of one’s development is going to involve technologies with which one is familiar and working out designs. This is classic problem isolation — all of your tools are familiar, and you have only to work out the design itself. In this case, I have two problems – the technology and the design aren’t familiar. In situations like this, it’s even more important to follow your nose when it comes to smells in code or tests, as you’re really flailing around in the dark. These may be the only things that guide a technology initiate to good practice short of taking a class or reading some books (and even then, they’re important, because you can only learn so much without getting your hands dirty.
So, I found myself with the following code during TDD (not a finished product):
public class LightServiceMongoImpl implements LightService {
public static final String ROOM_KEY = "room";
public static final String EQUALS_KEY = "$eq";
private DBCollection _collection;
public LightServiceMongoImpl(DBCollection collection) {
if(collection == null)
throw new IllegalArgumentException("Collection cannot be null.");
_collection = collection;
}
@Override
public Collection getLightsInRoom(String roomName) {
ArrayList myLights = new ArrayList();
BasicDBObject myQuery = new BasicDBObject();
myQuery.put(ROOM_KEY, new BasicDBObject(EQUALS_KEY, roomName));
DBCursor myCursor = _collection.find(myQuery);
while(myCursor != null && myCursor.hasNext()) {
myLights.add(new Light("asdf", "1"));
}
return myLights;
}
}
I’m in the middle of developing a query that will take the following document structure and parse it into a series of Light objects:
{ "_id" : ObjectId("4f7d2ba6fd5a306d82687d48"), "room" : "Den" }
{ "_id" : ObjectId("4f7d2baafd5a306d82687d49"), "room" : "Foyer" }
{ "_id" : ObjectId("4f7d2fdcfd5a306d82687d4a"), "room" : "Master Bedroom" }
{ "_id" : ObjectId("4f7d301afd5a306d82687d4b"), "room" : "Guest Bedroom" }
{ "_id" : ObjectId("4f7d2b98fd5a306d82687d47"), "code" : "A", "lights" : [ { "name" : "Overhead", "code" : "1" } ], "room" : "Kitchen" }
Each document corresponds to a room with a room name, a room code, and 0 to n lights, each of which are sub-documents. This may not be the ideal structure for what I’m trying to accomplish, but it’s a working structure, and these are growing pains.
Clearly, my code doesn’t yet work, but all tests so far pass. I looked at this class critically and decided I’d been a bit lax in the “refactor” portion of “red-green-refactor” and getLightsInRoom() was getting a little unwieldy. So, I refactored to this:
public class LightServiceMongoImpl implements LightService {
public static final String ROOM_KEY = "room";
public static final String EQUALS_KEY = "$eq";
private DBCollection _collection;
public LightServiceMongoImpl(DBCollection collection) {
if(collection == null)
throw new IllegalArgumentException("Collection cannot be null.");
_collection = collection;
}
@Override
public Collection getLightsInRoom(String roomName) {
ArrayList myLights = new ArrayList();
DBCursor myCursor = getCursorForRoomName(roomName);
while(myCursor != null && myCursor.hasNext()) {
myLights.add(new Light("asdf", "1"));
}
return myLights;
}
/**
* Given a room name, get a cursor that matches
* @param roomName - Name of the room for which to get a cursor
* @return The cursor retrieved
*/
private DBCursor getCursorForRoomName(String roomName) {
BasicDBObject myQuery = buildRoomNameQuery(roomName);
DBCursor myCursor = _collection.find(myQuery);
return myCursor;
}
/**
* Construct the query for a given room name
* @param roomName - room name for which to build the query
* @return - query object in question
*/
private BasicDBObject buildRoomNameQuery(String roomName) {
BasicDBObject myQuery = new BasicDBObject();
myQuery.put(ROOM_KEY, new BasicDBObject(EQUALS_KEY, roomName));
return myQuery;
}
}
Ah, much better. Again, I don’t know any design practices, but separating the parsing of the returned document from the construction of the query from the retrieval of the cursor seems like it is likely to come in handy later if there’s a need to divvy up responsibilities. So, I turned to my most recent test (it’s a passing test):
@Test
public void returns_collection_with_count_of_one_when_matching_record_has_lights_collection_with_one_element() {
BasicDBObject myLight = new BasicDBObject();
myLight.put("name", "overhead");
myLight.put("light", "1");
ArrayList myLights = new ArrayList();
myLights.add(myLight);
BasicDBObject myRoom = new BasicDBObject();
myRoom.put("lights", myLights);
DBObject myMockDatabaseObject = mock(DBObject.class);
Mockito.when(myMockDatabaseObject.get(RoomServiceMongoImpl.ROOM_NAME_KEY)).thenReturn(myRoom);
DBCursor myMockCursor = mock(DBCursor.class);
Mockito.when(myMockCursor.next()).thenReturn(myMockDatabaseObject).thenReturn(myMockDatabaseObject).thenReturn(null);
Mockito.when(myMockCursor.hasNext()).thenReturn(true).thenReturn(false);
DBCollection myMockCollection = PowerMockito.mock(DBCollection.class);
Mockito.when(myMockCollection.find(any(DBObject.class))).thenReturn(myMockCursor);
LightServiceMongoImpl myService = buildTarget(myMockCollection);
assertEquals(1, myService.getLightsInRoom("asdf").size());
}
Yeesh. I had been so focused on figuring out the MongoDB API that I had allowed myself to be the proverbial boiled frog. That’s a hideous test, and I see that I’m starting to duplicate logic like that in a few of my tests. I had written this off as beyond my control because the Mongo API is such that I need a collection in order to get a cursor, which will give me a database object. If these data access classes (called services temporarily until I set up a proper domain) don’t retain a reference to the collection, they can’t requery the database.
But, that reasoning is no good. The fact that some API works in a certain way is no excuse for me to violate the Law of Demeter. I’m storing a reference to a collection so that I can ask it for a cursor, which I then ask for a DB object. What I really want is a DBObject to parse, and that’s it. It’s time for me to introduce some other class that encapsulates all of that and returns DBObjects to me, which I will parse into my DTOs. I have a sneaking suspicion that doing this will make my test setup much less ugly.
So, on a hunch, I’m going to split responsibilities here such that my service invokes an API that takes a query as an argument and returns a series of DBObject:
public interface MongoQueryService {
void ExecuteQuery(BasicDBObject query);
Boolean HasNext();
DBObject getNext();
}
Now, I bet I can use this to make the class under test a bit more focused and my test setup a lot less painful. Here is my refactored class:
public class LightServiceMongoImpl implements LightService {
public static final String ROOM_KEY = "room";
public static final String EQUALS_KEY = "$eq";
private MongoQueryService _service;
public LightServiceMongoImpl(MongoQueryService service) {
if(service == null)
throw new IllegalArgumentException("Collection cannot be null.");
_service = service;
}
@Override
public Collection getLightsInRoom(String roomName) {
ArrayList myLights = new ArrayList();
_service.executeQuery(buildRoomNameQuery(roomName));
while(_service.hasNext()) {
myLights.add(new Light("asdf", "1"));
}
return myLights;
}
/**
* Construct the query for a given room name
* @param roomName - room name for which to build the query
* @return - query object in question
*/
private BasicDBObject buildRoomNameQuery(String roomName) {
BasicDBObject myQuery = new BasicDBObject();
myQuery.put(ROOM_KEY, new BasicDBObject(EQUALS_KEY, roomName));
return myQuery;
}
}
The collection is gone, as is the intermediate cursor object. In its stead I have the new query service interface that I’ve defined (I’m going to sort out the naming and package structure when I have a better grasp on things). Now, let’s take a look at the refactored test:
@Test
public void returns_collection_with_count_of_one_when_service_hasNext_is_true() {
MongoQueryService myMockService = mock(MongoQueryService.class);
Mockito.when(myMockService.hasNext()).thenReturn(true).thenReturn(false);
LightServiceMongoImpl myService = buildTarget(myMockService);
assertEquals(1, myService.getLightsInRoom("asdf").size());
}
Ah… I love the smell of deleted code in the (very early) morning. Smells like… victory! Now, granted, the implementation is still obtuse, and at some point, I’m going to have to deal with the fact that I probably won’t build a light for a null return value from the query service, which might mean revisiting this test, but I’d much rather start simple and add complexity than start complex and add complexity or, worse yet, start complex and have unnecessary complexity.
This new class is substantially easier to reason about. When clients call it, I build a query object to get what I want, tell some API to execute the query, and then iterate through the results. I’m no longer managing mixed levels of abstraction, handling a database collection, a database object iterator, and database object parsing in one flat class. Instead, I handle query construction and the parsing of query results, which is the same level of abstraction. And, for my good faith refactoring effort, I’m rewarded with a wonderfully simple unit test to run. Now, I can write tests about the query I’m constructing and handling the results of the query (and, if I decide I need to decouple query construction and result processing, which seems like a wise step to do at some point, it will be much easier).
The lesson here is to keep your nose open not just for smells in your code, but for smells in your tests. In my code, I had a law of demeter violation (and will probably have another one when I implement the query service, but I’ll deal with that during another refactor cycle). In my test, I had burgeoning test setup. In general, this can’t be ignored, but it’s especially important not to ignore this when you don’t really know what you’re doing. I don’t know enough about MongoDB, Mongo’s Java driver, JUnit, Mockito, etc to have the luxury of knowing when a smell is indicative of a problem and when it’s not. In this scenario, I have to be very careful to avoid smells if I’m going to have any hope of creating a good design on the fly while teaching myself multiple technologies.
In a similar situation, I’d advise you to take the same approach. My two cents, anyway.
