It’s been 10 years since I wrote my first unit test. Since then, I can’t remember how many thousands of unit tests I’ve written. To be honest I don’t make any distinction between source code and test code. For me it’s the same thing. Test code is part of the source code. The last 3-4 years, I’ve worked with several development teams and I had the chance to review a lot of test code. In this post I’m summarizing the most common mistakes that in-experienced developers usually do when writing unit tests.
Let’s take a look at the following simple example of a class that collects registration data, validates them and performs a user registration. Clearly the method is extremely simple and its purpose is to demonstrate the common mistakes of unit tests and not to provide a fully functional registration example:
public class RegistrationForm {
private String name,email,pwd,pwdVerification;
// Setters - Getters are ommitted
public boolean register(){
validate();
return doRegister();
}
private void validate () {
check(name, "email");
check(email, "email");
check(pwd, "email");
check(pwdVerification, "email");
if (!email.contains("@")) {
throw new ValidationException(name + " cannot be empty.");
}
if ( !pwd.equals(pwdVerification))
throw new ValidationException("Passwords do not match.");
}
private void check(String value, String name) throws ValidationException {
if ( value == null) {
throw new ValidationException(name + " cannot be empty.");
}
if (value.length() == 0) {
throw new ValidationException(name + " is too short.");
}
}
private boolean doRegister() {
//Do something with the persistent context
return true;
}Here’s a corresponding unit test for the register method to intentionally show the most common mistakes in unit testing. Actually I’ve seen many times very similar test code, so it’s not what I’d call science fiction:
@Test
public void test_register(){
RegistrationForm form = new RegistrationForm();
form.setEmail("Al.Pacino@example.com");
form.setName("Al Pacino");
form.setPwd("GodFather");
form.setPwdVerification("GodFather");
assertNotNull(form.getEmail());
assertNotNull(form.getName());
assertNotNull(form.getPwd());
assertNotNull(form.getPwdVerification());
form.register();
}
Now, this test, obviously will pass, the developer will see the green light so thumbs up! Let’s move to the next method. However this test code has several important issues.
The first one which is in my humble opinion, the biggest misuse of unit tests is that the test code is not adequately testing the register method. Actually it tests only one out of many possible paths. Are we sure that the method will correctly handle null arguments? How the method will behave if the email doesn’t contain the @ character or passwords don’t match? Developers tend to write unit tests only for the successful paths and my experience has shown that most of the bugs discovered in code are not related to the successful paths. A very good rule to remember is that for every method you need N numbers of tests where N equals to the cyclomatic complexity of the method adding the cyclomatic complexity of all private method calls.
Next is the name of the test method. For this one I partially blame all these modern IDEs that auto-generate stupid names for test methods like the one in the example. The test method should be named in such a way that explains to the reader what is going to be tested and under which conditions. In other words it should describe the path under testing. In our case a better name could be : should_register_when_all_registration_data_are_valid. In this article you can find several approaches on naming unit tests but for me the ‘should’ pattern is the closest to the human languages and easier to understand when reading test code.
Now let’s see the meat of the code. There are several assertions and this violates the rule that each test method should assert one and only one thing. This one asserts the state of four(4) RegistrationForm attributes. This makes the test harder to maintain and read (oh yes, test code should be maintainable and readable just like the source code. Remember that for me there’s no distinction between them) and it makes difficult to understand which part of the test fails.
This test code also asserts setters/getters. Is this really necessary? To answer that I will quote Roy Osherove’s saying from his famous book :” The Art of Unit Testing”
Properties (getters/setters in Java) are good examples of code that usually doesn’t contain any logic, and doesn’t require testing. But watch out: once you add any check inside the property, you’ll want to make sure that logic is being tested.
In our case there’s no business logic in our setters/getters so these assertions are completely useless. Moreover they wrong because they don’t even test the correctness of the setter. Imagine that an evil developer changes the code of the getEmail method to always return a constant String instead of the email attribute value. The test will still pass because it asserts that the setter is not null and it doesn’t assert for the expected value. So here’s a rule you might want to remember. Always try to be as much as specific you can when you assert the return value of a method. In other words try to avoid assertIsNull, assertIsNotNull unless you don’t care about the actual return value.
The last but not least problem with the test code we’re looking at is that the actual method (register) that is under test, is never asserted. It’s called inside the test method but we never evaluate its result. A variation of this anti-pattern is even worse. The method under test is not even invoked in the test case. So just keep in mind that you should not only invoke the method under test but you should always assert the expected result, even if it’s just a Boolean value. One might ask : “what about void methods?”. Nice question but this is another discussion – maybe another post, but to give you a couple of tips testing of a void method might hide a bad design or it should be done using a framework that verifies method invocations ( such as Mockito.Verify )
As a bonus here’s a final rule you should remember. Imagine that the doRegister is actually implemented and do some real work with an external database. What will happen if some developer that has no database installed in her local environment tries to run the test. Correct! Everything will fail. Make sure that your test will have the same behavior even if it runs from the dumpiest terminal that has access only to the code and the JDK. No network, no services, no databases, no file system. Nothing!
| Reference: | Common Mistakes Junior Developers Do When Writing Unit Tests from our JCG partner Patroklos Papapetrou at the Only Software matters blog. |
