Last week, <a href= http://caueguerra.com/>Cauê</a> and I were refactoring some classes in our systems and faced an interesting situation.

In our application we have a notifications system. It is quite simple; notifications are read from a text file and shown in a small information box in the home page. The MVC controller –it’s a Java application and we use Spring MVC – talks to a <a href=http://martinfowler.com/eaaCatalog/serviceLayer.html >Service Layer</a> and that finds out who should do the dirty job, in this case a class named NotificationTxtFile.

One important thing about notifications is a non-functional requirement saying that we only want to read the text file once per session. Here is the test we found for this class:

@RunWith(MockitoJUnitRunner.class)
public class NotificationServiceTest extends TestCase {
    @Mock
    SessionFacade sessionFacade;
 
    @Mock
    Notification notification;
 
    @Mock
    NotificationTxtFile notificationTxtFile;
 
    @Test
    public void shouldLoadNotificationFromSessionIfNotEmpty(){
        when(sessionFacade.getAttribute("notification")).thenReturn("My Notification");
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        String notificationMessage = notificationService.getNotificationToBeDisplayed();
 
        assertThat(notificationMessage, is("My Notification"));
 
        verify(notificationTxtFile, never()).getNotifications();
        verify(sessionFacade, never()).setAttribute("notification", "My Notification");
    }
 
    @Test
    public void shouldLoadNotificationFromServiceIfNewSession(){
        when(sessionFacade.getAttribute("notification")).thenReturn(null);
        when(notification.getMessage()).thenReturn("My Notification");
        when(notification.getTimeStamp()).thenReturn(new LocalDate().plusDays(1));
        when(notificationTxtFile.getNotifications()).thenReturn(Lists.create(notification));
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        String notificationMessage = notificationService.getNotificationToBeDisplayed();
 
        assertThat(notificationMessage, is("My Notification"));
 
        verify(sessionFacade).setAttribute("notification", "My Notification");
    }
 
    @Test
    public void shouldSetNotificationToBlankStringAtSessionIfThereIsNoNotification(){
        when(sessionFacade.getAttribute("notification")).thenReturn(null);
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        String notificationMessage = notificationService.getNotificationToBeDisplayed();
 
        assertThat(notificationMessage, is(""));
        verify(sessionFacade).setAttribute("notification", "");
    }
 
    @Test
    public void shouldSeeNotificationWhenMultipleNotificationsExistInTheFuture(){
        Notification notificationA = new Notification(tomorrow(), "my notification");
        Notification notificationB = new Notification(nextMonth(), "my other notification");
        when(notificationTxtFile.getNotifications()).thenReturn(create(notificationA, notificationB));
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        String notification = notificationService.getNotificationToBeDisplayed();
 
        assertThat(notification, is(notificationA.getMessage()));
    }
 
    @Test
    public void shouldNotSeeNotificationForPreviousDate(){
        Notification notificationA = new Notification(yesterday(), "my previous notification");
 
        when(notificationTxtFile.getNotifications()).thenReturn(create(notificationA));
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        String notification = notificationService.getNotificationToBeDisplayed();
 
        assertThat(notification, is(""));
    }
 
    @Test
    public void shouldNotSeeNotificationWhenThereIsNoNotificationTxtFile(){
        when(notificationTxtFile.getNotifications()).thenReturn(new ArrayList<Notification>());
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        String notification = notificationService.getNotificationToBeDisplayed();
 
        assertThat(notification, is(""));
   }
 
    private LocalDate yesterday() {
        LocalDate today = new LocalDate();
        return today.minusDays(1);
    }
 
    private LocalDate nextMonth() {
        LocalDate today = new LocalDate();
        return today.plusMonths(1);
    }
 
    private LocalDate tomorrow() {
        LocalDate today = new LocalDate();
        return today.plusDays(1);
    }
}

The tests may look OK, but there is an interesting problem there. The reason we use tests in agile methods is so that we have proper executable specifications about what’s being developed. Those specifications should talk about what a unit does, not about how it performs its task.

As I said before, the notifications file should be read only once. This is a requirement and this is what we care about here. If you look at the test, though, it doesn’t really test if the file is read only once. It tests if the file contents are stored in and retrieved from the (HTTP) session object.

Why should I care about where the file contents are stored? I shouldn’t have to, it’s not relevant to the classes requirements. Storing the file contents in the HTTP session is how our class happens to implement this feature. It can change this implementation detail at any moment and the class would still fulfil its requirements.

When writing a test you should focus only on the expected behaviour, not the current implementation. Using unit tests to verify design and implementation is a terrible thing, <a href=http://philcalcado.com/2008/01/08/please-do-not-break-into/> you break the encapsulation</a> and end up with really brittle tests that have to be changed all the time, just because some implementation detail changed even though the behaviour is the same.

I’ve been to many projects where this kind of highly coupled tests was the norm. They always end up with a test suite that has to be changed so often that developers prefer to only write integration tests, blaming unit testing for being too brittle. I think that they are missing the point, the problem is not with unit testing but with people breaking into their units and depending on its trade secrets.

The way Cauê and I dealt with this issue was to refactor this test to something like this:

@RunWith(MockitoJUnitRunner.class)
public class NotificationServiceTest extends TestCase {
 
    @Mock
    NotificationTxtFile notificationTxtFile;
 
    Notification yesterdayNotification = new Notification(yesterday(), "A long long time ago");
    Notification todayNotification = new Notification(today(), "Tonight is a good good night");
    Notification todayNotificationWithMultipleLines = new Notification(today(), "Jeremy spoke in class today\nClearly I remember\nPickin' on the boy");
    Notification tomorrowNotification = new Notification(tomorrow(), "Where's my hoverboard?");
 
    SessionFacade sessionFacade;
 
    @Before
    public void setup() {
        sessionFacade = new HashMapBackedSessionFacade();
    }
 
    @Test
    public void shouldReturnASingleLineNotificationForToday() {
        when(notificationTxtFile.getNotifications()).thenReturn(asList(yesterdayNotification, todayNotification, tomorrowNotification));
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        assertThat(notificationService.getNotificationToBeDisplayed(), is(todayNotification.getMessage()));
    }
 
    @Test
    public void shouldReturnMultipleLinesNotificationForToday() {
        when(notificationTxtFile.getNotifications()).thenReturn(asList(yesterdayNotification, todayNotificationWithMultipleLines, tomorrowNotification));
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        assertThat(notificationService.getNotificationToBeDisplayed(), is(todayNotificationWithMultipleLines.getMessage()));
    }
 
    @Test
    public void shouldReturnEmptyWhenThereAreNoNotificationForToday() {
        when(notificationTxtFile.getNotifications()).thenReturn(asList(yesterdayNotification));
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        assertThat(notificationService.getNotificationToBeDisplayed(), is(""));
    }
 
    @Test
    public void shouldReturnEmptyWhenThereAreNoNotificationsAtAll() {
        when(notificationTxtFile.getNotifications()).thenReturn(EMPTY_LIST);
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        assertThat(notificationService.getNotificationToBeDisplayed(), is(""));
    }
 
    @Test
    public void shouldNotRetrieveNotificationsMoreThanOnceForTheSameSession() {
        when(notificationTxtFile.getNotifications()).thenReturn(asList(yesterdayNotification, todayNotification, tomorrowNotification));
 
        NotificationService notificationService = new NotificationService(notificationTxtFile, sessionFacade);
        for (int i = 0; i < 10; i++) {
            assertThat(notificationService.getNotificationToBeDisplayed(), is(todayNotification.getMessage()));
        }
        verify(notificationTxtFile, only()).getNotifications();
    }
 
    private LocalDate yesterday() {
        LocalDate today = new LocalDate();
        return today.minusDays(1);
    }
 
    private LocalDate tomorrow() {
        LocalDate today = new LocalDate();
        return today.plusDays(1);
    }
 
    private LocalDate today() {
        return new LocalDate();
    }
 
}

This test is much less coupled to the implementation than the original version. But why is it? Because it only mocks the classes collaborators, not its aggregations and compositions. My general rule is that it is OK to mock any association between classes, except by aggregation and composition:

[aggregation is] a form of association that specifies a whole-part relationship between an aggregate (a whole) and a constituent part. It is also applicable to attributes and parameters.
[…]
There is a stronger form of aggregation, called composition. A composite is an aggregate with the additional constraints that an object may be part of only one composite and that the composite object has responsibility for the disposition of all its parts—that is, for their creation and destruction. An aggregation that is not a composition is called a shared aggregation, because parts may be shared among more than one whole.

Experience says that there are times when you need to mock one of those kinds of relationships; but these are exceptional situations and could probably be replaced by integration tests.

Aggregations and compositions are not just collaborations; they are part of the class internal design and should not be exposed; not even to tests.