Besides code review, the code coverage (line or/and branch) is also often considered as a medium for measuring the quality of unit tests. From my experience, this leads to developers adding some tests (let’s say it like useless tests) just to be sure their coverage result is enough. Even the tests which look cool and feel right, could be somehow stupid or not clever enough.
I do always thought though that i write reasonable tests, which do not just technically cover %100 of my code, but also cover all the functional possibilities; until last week when i realized just the opposite. As it is always better to explain with an example, here is a dummy part of my method to be tested:
protected void myMethod(MyObject o) {
o.setComment("comment");
MyObjectDAO.updateMyObject(o);
}
Here is a part of my JUnit, and this is the best case in our project by the way, where we always try to have %100 coverage; best case in terms of test quality, functional coverage etc.
@Mock
private MyObjectDAO myObjectDaoMock;
@Test
public void testMyMethod() {
MyObject o = new MyObject();
// Run Test
x.myMethod(o);
// Control
assertEquals("comment", o.getComment());
verify(myObjectDaoMock).updateMyObject(o);
}
It looks actually promising. We have two lines of code in our method, and the test is trying to test both lines. It could also have been like this, where we also have %100 coverage:
...
// Control
verify(myObjectDaoMock).updateMyObject(o);
}
Anyway, the above test is green; we are now testing whether the comment attribute of myObject is set, and the update method is called with myObject. But not whether the update method is called with the updated attribute. When we change the position of two lines in myMethod (first updating the object, then setting its attribute), our test is still green. Yet a good test should fail with any functional change!
I thought that the ArgumentCaptor can be useful here. So i made this one:
...
// Control
ArgumentCaptor<MyObject>; argumentCaptor = ArgumentCaptor.forClass(MyObject.class);
verify(myObjectDaoMock).updateMyObject(argumentCaptor.capture());
MyObject oActual = argumentCaptor.getValue();
assertEquals("comment", oActual.getComment());
}
… hoping that the ArgumentCaptor will capture that state of the object, by which the update method is called, so i can be sure that the update method is called with the updated comment attribute. The test is green again. But it still does not test cleverly. When we change the position of two lines in myMethod again (first updating the object, then setting its attribute), our test is still green.
I understand that, the ArgumentCaptor does not create another attribute for himself (argumentCaptor.getValue()), it is the reference of the original object. So since java passes the reference by value, for JUnit it does not make any difference whether i update the object before or after, as long as the objectIds are same.
So how can I actually test that the updateObject method is called with the updated value of myObject?
There are two solutions we can look at:
1- We can create an inner class in our test, extending the myObject type, so that we can override its equals and hashcode methods. In equals method we can add the comment attribute of the object as a requirement for objects’ equality. In that case, when our update method is not called with the intended value of comment, while comparing expect vs. actual Mockito will see another object since the objectIds will be different; and our test will fail with the changes stated above.
This solution was not an appropriate one for us; firstly since we have many other tests, which also need the original equals method of myObject type; secondly i would simply not prefer to extend a domain object in a test class.
2- (which i personally prefer) To have mocked both MyObject and MyObjectDAO. In that case we can verify whether the setter was called on MyObject and also the order of calls.
InOrder inOrder = inOrder(myObjectMock, myObjectDAOMock);
The above line assures that the dao will always be called after the object interaction (getter/setter). So the test for our modified method would fail. With this extension our test would look like this:
@Mock
private MyObjectDAO myObjectDaoMock;
@Mock // we can also use @Spy here, depending on our needs
private MyObject myObjectMock;
@Test
public void testMyMethod() {
// Run Test
x.myMethod(o);
// Control
InOrder inOrder = inOrder(myObjectMock, myObjectDAOMock);
inOrder.verify(myObjectMock).setComment("comment");
inOrder.verify(myObjectDAOMock).updateMyObject(o);
}
We have moved again one step further. The method above looks better since we now assure the order, the calls to our objects/mocks, the parameters which are passed by those calls. Yet we still miss something, the connection. When we think those cases we covered, one by one, all of them make sense. But we still do not test whether the update method is really called with the intended value “comment”.
Let’s change our method one more time, and see if the test would fail:
protected void myMethod(MyObject o) {
o.setComment("comment");
o.setComment("xxx");
MyObjectDAO.updateMyObject(o);
}
The above test would still pass through since the ordering is correct, comment attribute of myObject is set to “comment”, and my dao is called with myObject. But at the end our object will be saved with an “xxx” in its comment attribute. Again the same problem, we still do not test whether the update method is really called with the intended value “comment”. We can extend our test one more time:
...
// Control
InOrder inOrder = inOrder(myObjectMock, myObjectDAOMock);
inOrder.verify(myObjectMock).setComment("comment");
verifyNoMoreInteractions(myObjectMock);
inOrder.verify(myObjectDAOMock).updateMyObject(o);
}
Now this test would fail for the method above. Since verifyNoMoreInteractions prevents all the interactions with myObject, we can be sure that our intended value “comment” is set, and update method is called. But again we still miss here something. As i also wrote above “Yet a good test should fail with any functional change!” (which i think the most important sentence of this post), a good test also should not fail with any change! Actually i want to rewrite that sentence again: A good test should fail if, and only if, there is a functional change! Let’s modify our method last time:
protected void myMethod(MyObject o) {
o.setComment("comment");
x = o.getComment();
MyObjectDAO.updateMyObject(o);
}
Here, as we just call a getter, which do not have any effect on the functionality of my method, my test should NOT fail; since we always would like to have the chance of updating our methods as our requirements grow, but only then update their tests, when really needed. But our test above would again fail, since verifyNoMoreInteractions does not want us to use myObject anymore.
As you see, we just have a stupid method which has 2 lines of code, we still could not cover it %100. Writing unit test is really not that simple, should also not be. Let me say so; when we needed less time for writing tests for a specific method, than the time we needed to write the method itself, we should review that test again.
This is the last modification for our test:
...
// Control
InOrder inOrder = inOrder(myObjectMock, myObjectDAOMock);
inOrder.verify(myObjectMock).setComment("comment");
inOrder.verify(myObjectMock, times(0)).setComment(anyString());
inOrder.verify(myObjectDAOMock).updateMyObject(o);
}
Here, instead of preventing all the calls to myObject, we simply say “comment attribute of my object should not be set any more!”. Now we can modify our method as many time as we want. We really test whether our update method is called with the intended value of our object.
I have recently learned that the idea behind my way of thinking has actually a name: Mutation Testing. I think an old concept which is not known very well. I may post another entry about this topic someday. Until that time, happy clever test writing!
A Developer's Blog 