About Tomasz Nurkiewicz

Java EE developer, Scala enthusiast. Enjoying data analysis and visualization. Strongly believes in the power of testing and automation.

Don’t rely on unit tests alone

When you are building a complex system, barely testing components in isolation is not enough. It’s crucial, but not enough. Imagine a car factory that manufactures and imports highest quality parts, but after assembling the vehicle never starts the engine. If your test case suite consists barely of unit tests, you can never be sure that the system as a whole works. Let’s give a contrived example:
 
 
 
 
 
 

public class UserDao {
 
    public List<User> findRecentUsers() {
        try {
            return //run some query
        } catch(EmptyResultDataAccessException ignored) {
            return null;
        }
    }
 
    //...
}

I hope you already spotted an anti-pattern in the catch block (and I don’t mean ignoring the exception, it seems to be expected). Being a good citizen we decide to fix the implementation by returning an empty collection instead of null:

public class UserDao {
 
    public List<User> findRecentUsers() {
        try {
            return //run some query
        } catch(EmptyResultDataAccessException ignored) {
            return Collections.emptyList();
        }
    }
 
    //...
}

The fix is so simple that we almost forget about running unit tests, but just in case we execute them and find the first test case to fail:

public class UserDaoTest {
 
    private UserDao userDao;
 
    @Before
    public void setUp() throws Exception {
        userDao = new UserDao();
    }
 
    @Test
    public void shouldReturnNullWhenNoRecentUsers() throws Exception {
        //given
 
        //when
        final List<User> result = userDao.findRecentUsers();
 
        //then
        assertThat(result).isNull();
    }
 
    @Test
    public void shouldReturnOneRecentUser() throws Exception {
        //given
        final User lastUser = new User();
        userDao.storeLoginEvent(lastUser);
 
        //when
        final List<User> result = userDao.findRecentUsers();
 
        //then
        assertThat(result).containsExactly(lastUser);
    }
 
    @Test
    public void shouldReturnTwoRecentUsers() throws Exception {
        //given
        final User lastUser = new User();
        final User oneButLastUser = new User();
        userDao.storeLoginEvent(oneButLastUser);
        userDao.storeLoginEvent(lastUser);
 
        //when
        final List<User> result = userDao.findRecentUsers();
 
        //then
        assertThat(result).containsExactly(lastUser, oneButLastUser);
    }
 
}

Apparently not only the code was broken (by returning null instead of null-like empty collection), but there was a test verifying this bogus behaviour. I’m pretty sure the test was written after the implementation and it had to somehow deal with reality. No one would ever write a test like that without prior knowledge of the implementation peculiarities. So we fix the test and cheerfully wait for green CI build – which eventually comes. Days later our application breaks with NullPointerException on production. It breaks in a place that is unit-tested thoroughly:

public class StatService {
 
    private final UserDao userDao;
 
    public StatService(UserDao userDao) {
        this.userDao = userDao;
    }
 
    public void welcomeMostRecentUser() {
        final List<User> recentUsers = userDao.findRecentUsers();
        if (recentUsers != null) {
            welcome(recentUsers.get(0));
        }
    }
 
    private void welcome(User user) {
        //...
    }
}

We are surprised because this class is fully covered by unit tests (verification step omitted for clarity):

@RunWith(MockitoJUnitRunner.class)
public class WelcomeServiceTest {
 
    @Mock
    private UserDao userDaoMock;
    private WelcomeService welcomeService;
 
    @Before
    public void setup() {
        welcomeService = new WelcomeService(userDaoMock);
    }
 
    @Test
    public void shouldNotSendWelcomeMessageIfNoRecentUsers() throws Exception {
        //given
        given(userDaoMock.findRecentUsers()).willReturn(null);
 
        //when
        welcomeService.welcomeMostRecentUser();
 
        //then
        //verify no message sent
    }
 
    @Test
    public void shouldSendWelcomeMessageToMostRecentUser() throws Exception {
        //given
        given(userDaoMock.findRecentUsers()).willReturn(asList(new User()));
 
        //when
        welcomeService.welcomeMostRecentUser();
 
        //then
        //verify user welcomed
    }
 
    //...
 
}

You see where the problem lies? We changed the contract of UserDao class while it “looks” the same on the surface. By fixing broken tests we assumed it still works. However WelcomeService was still relying on the old behaviour of UserDao, which was either returning null or a list with at least one element. This behaviour was recorded using mocking framework, so that we were able to unit test WelcomeService in separation. In other words we failed to make sure these two components are still working with each other, we barely tested them alone. Back to our car metaphor – all the pieces still fit together (same contract), but one of them is not behaving internally the same as before. So, what really went wrong? There are at least four problems here, and if any of them was mitigated, none of this would have ever happened.

First of all the author of UserDao failed to recognize that returning a null while an empty list seems much more intuitive. It begs the question: is there a significant difference between null and empty collection? If yes, maybe you are trying to “encode” too much information in a single return value? If no, why make life of your API consumers harder? Iterating over empty collection doesn’t require any extra effort; iterating over collection that might be null needs one extra condition. Author of WelcomeService failed as well by assuming null means an empty collection. He should work around the ugly API rather than relying on it. In this case he could have used CollectionUtils.isNotEmpty() and be a little bit more defensive:

if (CollectionUtils.isNotEmpty(recentUsers)) {

For more comprehensive solution, he could also consider decorating UserDao and replacing null with empty collection. Or even using AOP to fix such APIs globally in whole application. And BTW this applies to Strings as well. In 99% of the cases there is no “business” difference between null, empty string and a string with few white spaces. Use StringUtils.isBlank() or similar by default, unless you really want to distinguish between them.

Finally the person “fixing” UserDao failed to see the big picture. Barely fixing unit tests is not enough. When you are changing the behaviour of a class without changing the API (this is especially important for dynamic languages), chances are you will miss places where this API was used, loosing the context. But the biggest failure was the lack of component/system tests. If we simply had a test case exercising both WelcomeService and UserDao running together, this bug would’ve been discovered. It’s not enough to have 100% code coverage. You test every piece of the jigsaw puzzle but never look at the finished picture. Have at least few larger, smoke tests. Otherwise you no longer have this awesome confidence that when tests are green, code is good to go.
 

Reference: Don’t rely on unit tests alone from our JCG partner Tomasz Nurkiewicz at the NoBlogDefFound blog.

Related Whitepaper:

Functional Programming in Java: Harnessing the Power of Java 8 Lambda Expressions

Get ready to program in a whole new way!

Functional Programming in Java will help you quickly get on top of the new, essential Java 8 language features and the functional style that will change and improve your code. This short, targeted book will help you make the paradigm shift from the old imperative way to a less error-prone, more elegant, and concise coding style that’s also a breeze to parallelize. You’ll explore the syntax and semantics of lambda expressions, method and constructor references, and functional interfaces. You’ll design and write applications better using the new standards in Java 8 and the JDK.

Get it Now!  

Leave a Reply


four − 4 =



Java Code Geeks and all content copyright © 2010-2014, Exelixis Media Ltd | Terms of Use
All trademarks and registered trademarks appearing on Java Code Geeks are the property of their respective owners.
Java is a trademark or registered trademark of Oracle Corporation in the United States and other countries.
Java Code Geeks is not connected to Oracle Corporation and is not sponsored by Oracle Corporation.

Sign up for our Newsletter

15,153 insiders are already enjoying weekly updates and complimentary whitepapers! Join them now to gain exclusive access to the latest news in the Java world, as well as insights about Android, Scala, Groovy and other related technologies.

As an extra bonus, by joining you will get our brand new e-books, published by Java Code Geeks and their JCG partners for your reading pleasure! Enter your info and stay on top of things,

  • Fresh trends
  • Cases and examples
  • Research and insights
  • Two complimentary e-books