My former colleague David just posted an example of verbose test code on his blog (the parts in French have been translated to English by myself):
/**
*
*/
@Test
public void testGetCustomerOK() {
LOGGER.info("======= testGetCustomerOK starting...");
try {
CustomerDTO targetDTO = this.serviceImpl.getCustomer("ABC99");
// Check.
Assert.assertNotNull("Extra not found.", targetDTO);
Assert.assertEquals("accountIds must be the same.", "ABC99", targetDTO.getAccountId());
} catch (CustomerBusinessException exception) {
LOGGER.error("CustomerBusinessException : {}",
exception.getCause());
Assert.fail(exception.getMessage());
} catch (UnavailableResourceException exception) {
LOGGER.error("UnavailableResourceException : {}",
exception.getMessage());
Assert.fail(exception.getMessage());
} catch (UnexpectedException exception) {
LOGGER.error("UnexpectedException : {}" +
exception.getMessage());
Assert.fail(exception.getMessage());
} catch (Exception exception) {
LOGGER.error("CRASH : " + exception.getMessage());
Assert.fail(exception.getMessage());
}
LOGGER.info("======= testGetCustomerOK done.");
}
Let’s see what I’d do with that.
Facing this code, the first thing I would do is remove the logs in the catch clauses, which are clearly obscuring the code the most. They do not provide actual value, since the exception message is used in Assert.fail() calls.
/**
*
*/
@Test
public void testGetCustomerOK() {
LOGGER.info("======= testGetCustomerOK starting...");
try {
CustomerDTO targetDTO = this.serviceImpl.getCustomer("ABC99");
// Check.
Assert.assertNotNull("Extra not found.", targetDTO);
Assert.assertEquals("accountIds must be the same.", "ABC99", targetDTO.getAccountId());
} catch (CustomerBusinessException exception) {
Assert.fail(exception.getMessage());
} catch (UnavailableResourceException exception) {
Assert.fail(exception.getMessage());
} catch (UnexpectedException exception) {
Assert.fail(exception.getMessage());
} catch (Exception exception) {
Assert.fail(exception.getMessage());
}
LOGGER.info("======= testGetCustomerOK done.");
}
Next, the catching of exceptions and calling Assert.fail() is in fact the same work that the test runner actually does. We can let the test runner handle those cases:
/**
*
*/
@Test
public void testGetCustomerOK() throws Exception {
LOGGER.info("======= testGetCustomerOK starting...");
CustomerDTO targetDTO = this.serviceImpl.getCustomer("ABC99");
// Check.
Assert.assertNotNull("Extra not found.", targetDTO);
Assert.assertEquals("accountIds must be the same.", "ABC99", targetDTO.getAccountId());
LOGGER.info("======= testGetCustomerOK done.");
}
The remaining logs are actually information that can already be known from the test runner, since test results are presented by methods. My guess is that they were added to make the previous logs more readable. They can go now:
/**
*
*/
@Test
public void testGetCustomerOK() throws Exception {
CustomerDTO targetDTO = this.serviceImpl.getCustomer("ABC99");
// Check.
Assert.assertNotNull("Extra not found.", targetDTO);
Assert.assertEquals("accountIds must be the same.", "ABC99", targetDTO.getAccountId());
}
The “check” comment serves no useful purpose. Also, there is an empty comment that obviously adds no value:
@Test
public void testGetCustomerOK() throws Exception {
CustomerDTO targetDTO = this.serviceImpl.getCustomer("ABC99");
Assert.assertNotNull("Extra not found.", targetDTO);
Assert.assertEquals("accountIds must be the same.", "ABC99", targetDTO.getAccountId());
}
We are down to 6 lines of code. The first assert does a check that is also done implicitely by the following one. That seems redundant. Also, it is clearly a situation that should not happen in a normal test run. That can go:
@Test
public void testGetCustomerOK() throws Exception {
CustomerDTO targetDTO = this.serviceImpl.getCustomer("ABC99");
Assert.assertEquals("accountIds must be the same.", "ABC99", targetDTO.getAccountId());
}
The remaining assertion has a comment that says what the test does. In some way, there is some value in this. However, in most (all?) cases, the person seeing the error would open the test class anyway and read the code. The test comment seems redundant. At the very least, its value is far from clear. In those situations, I remove the comments:
@Test
public void testGetCustomerOK() throws Exception {
CustomerDTO targetDTO = this.serviceImpl.getCustomer("ABC99");
Assert.assertEquals("ABC99", targetDTO.getAccountId());
}
OK, a little inlining can now be done:
@Test
public void testGetCustomerOK() throws Exception {
Assert.assertEquals("ABC99", this.serviceImpl.getCustomer("ABC99").getAccountId());
}
The Assert.assertEquals method can also be statically imported:
@Test
public void testGetCustomerOK() throws Exception {
assertEquals("ABC99", this.serviceImpl.getCustomer("ABC99").getAccountId());
}
And the test could use a renaming:
@Test
public void can_obtain_a_customer_by_account_id() throws Exception {
assertEquals("ABC99", this.serviceImpl.getCustomer("ABC99").getAccountId());
}
Hm… the “this.” clause is not really necessary:
@Test
public void can_obtain_a_customer_by_account_id() throws Exception {
assertEquals("ABC99", serviceImpl.getCustomer("ABC99").getAccountId());
}
And… that’s it. I’m down to 4 lines of code. There might be fewer comments, but I believe the result is a lot more readable.
I’ve found it useful to separate the Arrange, Act, and Assert parts
@Test
public void can_obtain_a_customer_by_account_id() throws Exception {
obtainedAccountId = serviceImpl.getCustomer(“ABC99”).getAccountId()
assertEquals(“ABC99”, obtainedAccountId);
}
here, the Arrange part is in the setUp(Fixture), thus implicit. I know you don’t like local variables, but it helps when there are multiple asserts (which might be of course a single logical assert) and you want to use plain JUnit and not introduce a new framework that has Facts/Observations).
@Peter yes, I agree separating the setup from the assertion is useful… when there are several asserts. Until then, I’d rather keep my tests compact.
Also, I’ve noticed that I shy away from having multiple asserts of that form. Often seems like having too many things tested at the same time (I do still have this kind of tests, but I am not happy with them).
I don’t have a problem with having multiple simple asserts with inlined values, though.
So, for me, this is rather good:
public void can_obtain_a_customer_by_account_id() throws Exception {
assertEquals(“ABC99", serviceImpl.getCustomer(“ABC99").getAccountId());
assertEquals(“X", serviceImpl.getCustomer(“X").getAccountId());
assertEquals(“1", serviceImpl.getCustomer(“1").getAccountId());
}
While this is (a little) bad:
public void can_obtain_a_customer_by_account_id() throws Exception {
Customer customer = serviceImpl.getCustomer(“ABC99");
assertEquals(“ABC99", customer.getAccountId());
assertEquals(“John", customer.getName());
}
Seems we have a somewhat different style for writing tests :)
In the first case, when I check the same thing for different input, I would either create a helper method
def assert_obtained_account_is_the_same_as_we_searched_for(self, account_number):
self.assertEquals(account_number, self.service.get_customer_for(account_number).account_number)
and use that inside the test – simply to respect the single responsibility principle, and to remove the how from the test, so they can focus on the what
Could you elaborate why you consider the second case bad? E.g.: when asserting against an object’s default values, I would write such tests and I don’t consider it bad
P.S.: and sorry for accidentally writing the code in Python instead of Java :)