Smelly Code: Eager Test

This post shows a common code smell in unit tests also mentioned in the xunit test patterns book, namely "Eager tests".

Original, smelly code

public void testSaveSession(){
 MockSession mockSession = new MockSession("Test");
 mockSession.addParticipant(devices.get(0));
 mockSession.setSessionStart(DateUtils.createDateInstance("15.05.2010 12:00:00"));

 assertTrue(dao.saveSession(mockSession));
 assertTrue("The id should have been assigned", mockSession.getId() > 0); 

 mockSession.setSessionEnd(DateUtils.createDateInstance("15.05.2010 13:00"));
 assertTrue(dao.saveSession(mockSession));
 assertTrue("The id should have been assigned", mockSession.getId() > 0);
 assertNotNull("The date should have been assigned", mockSession.getSessionStart());
 
 Cursor participantCursor = mockContentResolver.query(Participant.CONTENT_URI, null, Participant.DEVICES_ID + "=? AND " + Participant.SESSIONS_ID + "=?", new String[]{String.valueOf(devices.get(0).getId()), String.valueOf(mockSession.getId())}, null);

 assertNotNull(participantCursor);
 assertTrue(participantCursor.moveToFirst());
 assertEquals("There should be 1 session", 1, participantCursor.getCount());
}

Objections and Comments

The unit test shown above is a typical example of an eager test. The problem is that the test case does not only cover a single functionality, but several ones.

Line 9, 10 - The code modifies the originally stored object and sets another property and persists everything again to the DB. In line 12 a verification on the correct storing/retrieving of that value is being done. This actually verifies an update and is therefore out of scope of the "saveSession" test. It should be extracted to another one.

Line 12-18 -  The ContentProvider is queried on whether the participants associated to the session in line 3 have been persisted properly. Again, this should not be done in this test case.

Proposed Refactoring

A possible refactoring is to split the test case into different ones, by using the Extract Method refactoring. I came up with three of them.

The first is for testing just the save operation. All it needs to do is to verify whether the object got the according id associated from the DB.

public void testSaveSession(){
 //setup
 MockSession mockSession = new MockSession("Test");
 mockSession.setType(SessionType.MEETING);
 mockSession.setSessionStart(DateUtils.createDateInstance("15.05.2010 12:00:00"));
 mockSession.setSessionEnd(DateUtils.createDateInstance("15.05.2010 12:10:00"));
 mockSession.setDurationSeconds(5000);

 //exercise
 assertTrue(dao.saveSession(mockSession));
 
 //verify
 assertTrue("The id should have been assigned", mockSession.getId() > 0);
}

The next one is responsible for verifying the update operations. It checks whether the object is being updated correctly and - for instance - it doesn't create a new record in the DB.

public void testUpdateSession(){
 //setup (by inserting a session which can be updated subsequently)
 MockSession mockSession = new MockSession("Test");
 mockSession.setType(SessionType.MEETING);
 mockSession.setSessionStart(DateUtils.createDateInstance("15.05.2010 12:00:00"));
 dao.saveSession(mockSession);
 
 //exercise (do the update operation)
 mockSession.setSessionEnd(DateUtils.createDateInstance("15.05.2010 12:10:00"));
 dao.saveSession(mockSession);
 
 //verify
 Session updatedSession = dao.readSessionById(mockSession.getId());
 assertSessionEquals(mockSession, updatedSession);
}

Note, in line 14 I've factored out the comparison of the session object into a custom helper method which looks as follows

/*
 * Utility method for comparing session objects
 */
private void assertSessionEquals(Session expected, Session actual){
 assertEquals("The session id should match", expected.getId(), actual.getId());
 assertEquals("The session start date should match", expected.getSessionStart(), actual.getSessionStart());
 assertEquals("The session end date should match", expected.getSessionEnd(), actual.getSessionEnd());
 assertEquals("The session duration should match", expected.getDurationSeconds(), actual.getDurationSeconds());
 assertEquals("The session type should match", expected.getType(), actual.getType());
 assertEquals("The number of participants should match", expected.getParticipants().size(), actual.getParticipants().size());
}

Finally, the last one tests the persisting of a session with participants.

public void testSaveSession_WithParticipants(){
 //setup
 MockSession mockSession = new MockSession("Test");
 mockSession.addParticipant(devices.get(0));
 mockSession.addParticipant(devices.get(1));
 
 //exercise
 dao.saveSession(mockSession);
 
 //verify (by bypassing the Dao and directly querying the ContentProvider/DB
 Cursor participantCursor = mockContentResolver.query(Participant.CONTENT_URI, null, Participant.SESSIONS_ID + "=?", new String[]{ String.valueOf(mockSession.getId())}, null);
 assertTrue("The participant cursor should contain some participants", participantCursor.moveToFirst());
 assertEquals("There should be exactly 2 participants", 2, participantCursor.getCount());
}

The resulting code is much better organized, readable and consequently also more maintainable.

This is one of a series of "smelly code" posts. Read more here or find them on Twitter.

---

Questions? Thoughts? Hit me up on Twitter