DaedTech

Stories about Software

By

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<Light> getLightsInRoom(String roomName) {
		
		ArrayList<Light> myLights = new ArrayList<Light>();
		
		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<Light> getLightsInRoom(String roomName) {
		
		ArrayList<Light> myLights = new ArrayList<Light>();
		
		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<BasicDBObject> myLights = new ArrayList<BasicDBObject>();
	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<Light> getLightsInRoom(String roomName) {
		
		ArrayList<Light> myLights = new ArrayList<Light>();
		
		_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.

By

JUnit for C# Developers 7 – Law of Demeter and Temporal Mocking

Last time in this series, I posted about a good reminder of a couple of principles of good object oriented design (respecting the Law of Demeter and avoiding static method invocations as much as possible). Today, I’m going to double back on this consciously a bit to explore some more possibilities in JUnit. Don’t worry – I will fix the design in subsequent posts.

Goals

Today, I’d like to accomplish the following:

  1. Have a mock change with each invocation
  2. Mock a low of demeter violation in as little code as possible

To the Code!

If you’ve followed my last few posts, you’ve noticed that I setup MongoDB. So, logically, the next step is connecting to it with my application, and the next step after that is mocking this connection so that I can unit test the logic (well, since I’m following TDD, technically the mocking comes first). Through trial and error in a throw-away piece of code, I discovered that I could access my database as so:

public class HouseServiceMongoImpl implements HouseService {

	private Mongo _mongoConnection;
	
	private DB _database;
	
	public HouseServiceMongoImpl() {
		try {
			_mongoConnection = new Mongo( "192.168.2.191" , 27017 );
			_database = _mongoConnection.getDB("daedalus");
		} catch (UnknownHostException e) {
			// TODO Auto-generated catch block
			e.printStackTrace();
		} catch (MongoException e) {
			// TODO Auto-generated catch block
			e.printStackTrace();
		}
	}

	@Override
	public House getHouse() {
		DBCollection myCollection = _database.getCollection("house");
		DBCursor myCursor = myCollection.find();
				
		List<String> myStrings = new ArrayList<String>();
		
		while(myCursor.hasNext()) {
			myStrings.add(myCursor.next().get("room").toString());
		}
		return new HouseModel(myStrings);
	}
	
}

Notice here that I’ve inlined instantiation of my dependencies in a form bad for testability. I won’t repeat this mistake in the actual, TDD version. This was just for me to see what actually worked and provide some context for what I’m trying to do. In my real class, RoomServiceMongoImpl, I’m going to be returning a collection of room objects, rather than a house object. So, without further ado, on to the TDD. To get things started, I wrote the following test:

public class RoomServiceMongoImplTest {

	private static RoomServiceMongoImpl BuildTarget(DB database) {
		
		return new RoomServiceMongoImpl(database);
		
	}
		
	public static class getAllRooms {
		
		/*
		 * If the driver gives us back nothing for this collection, return an empty collection to clients
		 */
		@Test
		public void returns_empty_list_when_database_get_collection_returns_null_for_house() {
			
			DB myMockDatabase = mock(DB.class);
			Mockito.when(myMockDatabase.getCollection(Mockito.matches("house"))).thenReturn(null);
			RoomServiceMongoImpl myService = BuildTarget(myMockDatabase);
			
			assertEquals(0, myService.getAllRooms().size());
		}
	}
}

This allowed me to create the room service and inject into it the DB object, since as far as I can tell, I gain nothing by injecting the Mongo object. So, my first test is that we get back a null object in the form of an empty list when the MongoDB collection requested is empty. This makes sense, as it means we have no house (and thus no rooms).

When doing TDD, I’ve gotten in the habit of teasing out any control flow by varying up the return value of the method. So, the first test returns an empty collection. The next test will return count of one. The next will return a bunch. The one after that may go back to zero but in a different set of circumstances. This sort of progression allows me to make progress while covering all my bases. So, here, I’m going to do the setup necessary for a list with a single item to be returned.

But… as it turns out, this isn’t trivial. From my database, I’m getting a collection, which I’m asking for a database cursor, which I’m asking in a loop for an object called “next”, which I’m then asking for my actual room’s string value (its name). This has the Law-of-Demeter violating form db.getCollection().find().next().get(string). Ugh. That’s a lot of mocking, and creating mocks this way is the smell of LOD violations in your code. But, let’s suck it up for now and create the mocks:

@Test
public void returns_list_with_one_item_when_db_returns_matching_cursor() {

	DBObject myMockDatabaseObject = mock(DBObject.class);
	Mockito.when(myMockDatabaseObject.get(Mockito.anyString())).thenReturn("asdf");
	
	DBCursor myMockCursor = mock(DBCursor.class);
	Mockito.when(myMockCursor.next()).thenReturn(myMockDatabaseObject);
			
	DBCollection myMockCollection = PowerMockito.mock(DBCollection.class);
	Mockito.when(myMockCollection.find()).thenReturn(myMockCursor);
	
	DB myMockDatabase = mock(DB.class);
	Mockito.when(myMockDatabase.getCollection(Mockito.matches("house"))).thenReturn(myMockCollection);
	
	RoomServiceMongoImpl myService = BuildTarget(myMockDatabase);
	
	assertEquals(1, myService.getAllRooms().size());
}

This was particularly annoying to write since it cost me a good bit of frustration trying to figure out why I was getting cryptic error messages about the wrong return type. Turns out it was because the collection.find() method is final, requiring me to use PowerMockito (this stack overflow post ended my suffering and I voted the poster up both for the question and his self-answer as my way of saying thanks). But, I was done, and the following class made the test pass:

public class RoomServiceMongoImpl implements RoomService {

	/**
	 * This is the MongoDB database object we'll use for our queries
	 */
	private DB _database;
	
	/**
	 * Dependency injected constructor
	 * @param database
	 */
	public RoomServiceMongoImpl(DB database) {
		_database = database;
	}

	@Override
	public Collection<Room> getAllRooms() {
		Collection<Room> myRooms = new ArrayList<Room>();
		
		if(_database.getCollection("house") != null && _database.getCollection("house").find().next().get("bkag") != null) {
			myRooms.add(new RoomModel("blah", null, 'A'));
		}
		return myRooms;
	}
}

Hideous, with all of those LOD violations, but functional. So now, the next step is to factor the code toward something less obtuse with my TDD, and an excellent way to do this is the case of two strings in the database. But, in order to make that happen, the cursor mock’s next() method has to first return a DB object and then return null. It just so happens that this is goal number one, and it can be achieved as follows:

DBCursor myMockCursor = mock(DBCursor.class);
Mockito.when(myMockCursor.next()).thenReturn(myMockDatabaseObject).thenReturn(myMockDatabaseObject).thenReturn(null);
Mockito.when(myMockCursor.hasNext()).thenReturn(true).thenReturn(true).thenReturn(false);

Note the chained returns calls. I took what I had in the previous test and turned it into this code, and then amended the expected size of the collection returned to be 2 using hasNext() and next(). I also had to go back and add similar code to the previous test for completeness sake. With this in place, the CUT getAllRooms() method became:

@Override
public Collection<Room> getAllRooms() {
	Collection<Room> myRooms = new ArrayList<Room>();
	DBCollection myCollection = _database.getCollection("house");
	if(myCollection != null) {
		DBCursor myCursor = myCollection.find();
		if(myCursor != null) {
			while(myCursor.hasNext()) {
				myRooms.add(new RoomModel(myCursor.next().get("asdf").toString(), null, 'A'));
			}
		}
	}
	return myRooms;
}

Man, that’s ugly. But, it works, and that’s what matters. We’re going to pretty this up and make it more robust later during the “refactor” cycles of red-green-refactor. So, on to goal number two, which is to compact the mock setup code somewhat. Granted, the pain of this setup indicates a design smell and there is a good argument that we should not deoderize these test smells. But, given that this is partially an endeavor for me to learn and blog about the mocking tools available with JUnit, I’m going to make an exception, with a promise to myself that I will pay off this loan of technical debt later by doing what I can to cut down on the database boilerplate I don’t need in this class. I came up with this:

@Test
public void returns_list_with_one_item_when_db_returns_matching_cursor() {

	DBObject myMockDatabaseObject = mock(DBObject.class);
	Mockito.when(myMockDatabaseObject.get(Mockito.anyString())).thenReturn("asdf");
			
	DB myMockDatabase = mock(DB.class);
	
	Mockito.when(myMockDatabase.getCollection(Mockito.matches("house"))).thenReturn(PowerMockito.mock(DBCollection.class));
	Mockito.when(myMockDatabase.getCollection(Mockito.matches("house")).find()).thenReturn(mock(DBCursor.class));
	Mockito.when(myMockDatabase.getCollection(Mockito.matches("house")).find().next()).thenReturn(myMockDatabaseObject).thenReturn(null);
	Mockito.when(myMockDatabase.getCollection(Mockito.matches("house")).find().hasNext()).thenReturn(true).thenReturn(false);
	
	RoomServiceMongoImpl myService = BuildTarget(myMockDatabase);
	
	assertEquals(1, myService.getAllRooms().size());
}

But, it throws a null reference exception on the first find() setup (second Mockito.when() line) and it doesn’t really save any code anyway, so I see no advantage to going this route. My instincts were against it anyway, and since it doesn’t work as fluidly as I hoped it might (I feel like I could probably get this fluent-style to work with enough persistence) and doesn’t save us code, forget it. Nothing ventured, nothing gained. I’ll have to be satisfied that this fluent-chaining appears possible, but might be a stone better left un-turned in favor of going back and cleaning up my code by eliminating LOD violations and generally seeking not to have them.

These posts are getting a little fewer and further between now as I’m less frequently blazing new trails in my work. I will probably check back in on this line of posts when I factor the design of this a bit, as kind of a MongoDB/JUnit fusion post.

By

Configuring Fedora and MongoDB

In my last post, I covered installing MongoDB on Fedora. This is another post as much for my reference as for anything else, and I’ll go over here getting set up so that my application successfully uses the MongoDB server.

When I left off last time, I had successfully configured the server to allow me to create documents using the mongo command line utility. So, I created a collection and a document and was ready to access it from my web application. Following the examples in the MongoDB java tutorial, I created the following implementation of my HouseService interface that I had previously hard coded room values into:

public class HouseServiceMongoImpl implements HouseService {

	private Mongo _mongoConnection;
	
	private DB _database;
	
	public HouseServiceMongoImpl() {
		try {
			_mongoConnection = new Mongo( "192.168.2.191" , 27017 );
			_database = _mongoConnection.getDB("daedalus");
		} catch (UnknownHostException e) {
			// TODO Auto-generated catch block
			e.printStackTrace();
		} catch (MongoException e) {
			// TODO Auto-generated catch block
			e.printStackTrace();
		}
	}

	@Override
	public House getHouse() {
		DBCollection myCollection = _database.getCollection("house");
		DBCursor myCursor = myCollection.find();
				
		List<String> myStrings = new ArrayList<String>();
		
		while(myCursor.hasNext()) {
			myStrings.add(myCursor.next().get("room").toString());
		}
		return new HouseModel(myStrings);
	}
	
}

I felt pretty good about that, and so I fired up the local process and promptly got an error:

java.io.IOException: couldn't connect to [/192.168.2.191:27017] bc:java.net.NoRouteToHostException: No route to host: connect

IP address was resolving correctly, but a cryptic “NoRouteToHoseException”. I deployed my app to the server, “daedalus” and it ran fine, as-is. I was glad to see that my code was working as I expected with Mongo, but I didn’t want to have to deploy to the server every time I wanted to run the application, so I started poking around for a fix. I had no luck connection through the browser either. Something was strange.

At this point, I remembered a similar annoyance in dealing with MySQL, and for some reason “iptables” popped into my head. After poking around a little for syntax particulars, I ran the following commands:

iptables -A INPUT -m state --state NEW -m tcp -p tcp --dport 27017 -j ACCEPT
iptables -A INPUT -m state --state NEW -m tcp -p tcp --dport 28017 -j ACCEPT

Sure enough, that did the trick. Accessing MongoDB through the browser now yielded:

I had been struggling to figure out what I needed to do to the mongo installation, but the problem was with the Linux firewall (or, more specifically, the fact that I hadn’t set it up to allow connections on these ports). To make my changes permanent, I added the following to /etc/sysconfig/iptables

-A INPUT -m state --state NEW -m tcp -p tcp --dport 27017 -j ACCEPT
-A INPUT -m state --state NEW -m tcp -p tcp --dport 28017 -j ACCEPT

I felt good about this, since I saw my handiwork in the same file opening up ports for MySQL and Tomcat. To verify that this bit of goodness stuck around, I rebooted and re-ran the web app on my client desktop…

By

Setting up MongoDB on Fedora

This is one of those posts as much for my own reference as anything else, and if it helps others, then bonus.

I installed MongoDB on my old Fedora server just now, and while it was actually pretty straightforward to do, I figured I’d document the steps. The first thing to remember is that there are two separate installs: the client and the server (this is what tripped me up on the MongoDB site’s documentation). There isn’t some deal where running server install also gives you a client.

So, to set up a smooth, easy install with yum, the first thing you need to do is create the file “/etc/yum.repos.d/10gen.repo”. Once you’ve created that file, open it and add the following:

[10gen]
name=10gen Repository
baseurl=http://downloads-distro.mongodb.org/repo/redhat/os/i686
gpgcheck=0

Now, once you’ve got that in place, run the command, “yum install mongo-10gen mongo-10gen-server”. This will install both the client and the server. When the client and the server are in place, you can start the server by running “/etc/init.d/mongod start”. Finally, if you’re like me, you probably want the server to run automatically. If that’s the case, execute the command “chkconfig mongod on”.

And, you’re set. MongoDB server is installed and running and will also run on your next reboot.

Acknowledgements | Contact | About | Social Media