«    »

How to Write Good Unit Tests

My previous post on How to Create Maintainable Software talked about the importance of automated testing. Working with automated test suites over the years, I've found that creating good tests is a skill that needs to be developed. I've seen many cases of unit tests that I'd consider poor or bad due to a number of reasons: tests that exercise simple code unlikely to break, tests that are brittle and fail due to the most minor of change, such as different data in the database or a minor user interface change, or the tests depend on data in the database that is poorly managed. In this article I will provide examples of such bad tests and show how they can be improved.

This article assumes you are familiar with unit testing in Java using JUnit. If not, check out the documentation on the JUnit website. A good introductory book on the topic is Pragmatic Unit Testing in Java with JUnit by Andrew Hunt and Dave Thomas. The back of the book has a summary page that does a great job highlighting the main concepts explained in the book, and is worth looking at even if you have experience with unit testing.

In each of the following sections I discuss a particular category of bad or poor tests, provide an example, and explain how such tests can be improved.

Testing trivial code

Unit tests should test those segments of code that are likely to have defects when first developed, or are likely to have defects introduced when changed. Like all software development activities, there is a cost benefit analysis that can be applied to writing unit tests. For normal enterprise business software, it is not worthwhile to test trivial code.

Typical examples of trivial code in Java include simple getter and setter methods for properties and simple constructors.

class User {
  private String name;

  public String getName() {
    return name;
  }

  public void setName(String newName) {
    name = newName;
  }
}

class UserTest extends TestSuite {
  public void testNameProperty() {
    User user = new User();
    assertNull(user.getName());

    String testName = "test";
    user.setName(testName);
    assertEquals(testName, user.getName());
  }
}

When I see tests such as these, I simply delete them. It is unlikely for simple getter/setter methods to change in the future. The test doesn't improve my understanding of the code and just takes up space, hiding the more useful tests.

Using hardcoded values when checking test results

I sometimes see tests that hardcode a 'magic' value when checking the results of some operation. This magic value is often separately hardcoded in the application code being tested. When the value is changed in the application code, the test is guaranteed to fail.

class CustomerWebController {

  public String doOperationReturningNextPage(UserInput input) {
    // some random logic...
    return "newCustomer.jsp"; 
  }
}

class CustomerWebControllerTest extends TestSuite {
  public void testDoOperation() {
    CustomerWebController controller = new CustomerWebController();
    UserInput input = getInputForNewCustomer();
    String result = controller.doOperationReturningNextPage(input);
    assertEquals("newCustomer.jsp", result);
  }
}

The fix for this is simply the application of the DRY principle: Don't Repeat Yourself (from the book The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas). When you go to use a magic value more than once, define it as a constant (or method) and then refer to that constant (or method).

To improve the example, we define a constant for the "newCustomer.jsp" value.

class CustomerWebController {

  public static final NEW_CUSTOMER_PAGE = "newCustomer.jsp";

  public String doOperationReturningNextPage(UserInput input) {
    // some random logic...
    return NEW_CUSTOMER_PAGE;
  }
}

class CustomerWebControllerTest extends TestSuite {
  public void testDoOperation() {
    CustomerWebController controller = new CustomerWebController();
    UserInput input = getInputForNewCustomer();
    String result = controller.doOperationReturningNextPage(input);
    assertEquals(CustomerWebController.NEW_CUSTOMER_PAGE, result);
  }
}

Being too dependent on specific test data

There is some debate whether unit tests should involve the database (purists tend to argue not), but in practise this is quite common and serves a useful purpose. However, problems often occur because such tests are overly dependent on specific data in the database. As new tests are written and new test data is added, this can cause existing, unrelated tests to fail.

class CustomerDataAccess {
  public List findCustomers(CustomerCriteria criteria) {
    // Logic to query database based on criteria
    return customersFound;
  }

class CustomerDataAccessTest extends TestSuite {
  public void testFindCustomers {
    CustomerDataAccess customerDataAccess = new CustomerDataAccess();
    CustomerCriteria criteria = new CustomerCriteria();
    String firstNameToFind = "Bob";
    criteria.firstNameEquals(firstNameToFind);
    List results = customerDataAcess.findCustomers(criteria);
    assertEquals(2, results.size());
  }
}

This example is based on real code that I came across. The problem is that the test expects there to be only two customers with a first name of 'Bob', which must have been the case when the test was first written and executed. However at some later date, working on some unrelated piece of code, a developer could add another customer named Bob, and suddenly this test will fail. To fix this type of situation in general, you need to minimize the number of assumptions you make about the test data. This reduces the level of coupling between the test and the data, which makes either easier to change independent of the other.

To improve this particular example we simply need to change the test to check that each result matches the criteria we specified.

class CustomerDataAccessTest extends TestSuite {
  public void testFindCustomers {
    CustomerDataAccess customerDataAccess = new CustomerDataAccess();
    CustomerCriteria criteria = new CustomerCriteria();
    String firstNameToFind = "Bob";
    criteria.firstNameEquals(firstNameToFind);
    List results = customerDataAcess.findCustomers(criteria);
    for (Customer customer : results) {
      assertEquals(firstNameToFind, customer.getFirstName());
    }
  }
}

Slowly executing tests

One problem with having unit tests involve the database is that such tests tend to run slower than in-memory tests. As the suite of tests grow, it is important to minimize the time required to run the entire suite. Ideally you should execute the entire test suite before checking changes into the version control repository. The longer it takes to run the tests, the less likely you are to do this, and the more likely you are to check in changes that break the tests, which defeats the entire purpose of having them. A rule of thumb I use is that all tests should execute within about five minutes.

When I joined one project, the unit tests took almost two hours to run. When I had the opportunity I analyzed the performance of the tests to determine which tests were taking the longest and why. I discovered several issues, all dealing with database access. The most significant issue was the setup and teardown being done for the persistence tests. Due to the design of the application, any creating or updating of records required a User object to be supplied, and in order to meet foreign key constraints, this User object must be defined in the database. Each persistence test created a new user object (plus all associated objects, when needed) in the database in the setup phase, and deleted all these new objects from the database in the teardown phase. Since in JUnit the setup and teardown are performed before and after each test method, this leads to a lot of database calls.

class UserTest extends TestSuite {
  public User createTestUser() {
    // Create test user in database.
    return user;
  }

  public void deleteTestUser(User user) {
    // Delete test user from database.
  }
}

class CustomerTest extends TestSuite {
  private User user;
  public void setUp() {
    user = UserTest.createTestUser();
  }

  public void tearDown() {
    UserTest.deleteTestUser(user);
  }

  public void testCreating() {
    // Test code ...

    // Whether or not this method needs the user object, 
    // it is created before the method is called, and deleted afterwards.
  }

  public void testUpdating() {
    // Test code ...

    // Whether or not this method needs the user object, 
    // it is created before the method is called, and deleted afterwards.
  }

  public void testNameValidation() {
    // Test code ...

    // Whether or not this method needs the user object, 
    // it is created before the method is called, and deleted afterwards.
  }

}

To improve the situation, I introduced special cached test users to use for these tests, which would only need to be loaded once for all the tests. This vastly improved the performance of the persistence tests, but did have a cost: these tests were now more dependent on specific test data, which I specifically mentioned already as something to avoid as much as possible. The general principle I use to minimize the risk of working with shared data is to have shared data be read-only. Tests using shared data are not allowed to modify that data. If a test needs to modify such data, it has to create its own copy.

You cannot easily delete the cached test user from the database within any specific test, since a subsequent test may want to make use of it. This means that test users will continually be created in the database. To avoid this, the initial creation of the test user can look for the existence of a special test user (i.e. via a special name), and if found use that instead of creating a new instance, but this further increases the risk of that user record being modified in the database and breaking tests as a result. A better option would be to perform a special setup and teardown operation once for a set of tests, but JUnit does not easily provide this functionality. (TestNG is a newer Java testing framework with features such as this.)

After I finished my performance optimizations, the tests for this project executed in about twelve minutes - still not great, but a big improvement from two hours.

class UserTest extends TestSuite {

  private static User cachedTestUser;

  public static synchronized User getCachedTestUser() {
    if (cachedTestUser == null) {
      cachedTestUser = loadTestUserCreatingIfNotExist();
    }
    return cachedTestUser;
  }

}

class CustomerTest extends TestSuite {
  private User user;
  public void setUp() {
    user = UserTest.getCachedTestUser();
  }

  public void testCreating() {
    // Test code ...
  }

  public void testUpdating() {
    // Test code ...
  }

  public void testNameValidation() {
    // Test code ...
  }
}

Like any skill, the ability to write good unit tests takes effort to develop. I try to hone my test-writing skill by paying attention to a couple situations:

  1. When I encounter a defect and identify the cause, I always aim to write a test to prevent the defect from reoccuring. I also ask myself why that test didn't exist in the first place, or why it missed the defect.
  2. When I encounter brittle tests (tests that unexpectedly fail due to unrelated changes), I ask myself how those tests could have been written differently to be more robust.

I hope this proves helpful in improving your ability to write unit tests. If you come across other examples of bad tests, I'd be interested in hearing about them.

If you find this article helpful, please make a donation.

15 Comments on “How to Write Good Unit Tests”

  1. Kola says:

    Some really good advice here. I’m curious if/how you test private methods…

  2. I sometimes test private methods if they have non-trivial logic. To test them, I change them from private to package (default) access so that my test class in the same package can access them. I usually add a comment above the method to indicate that it is non-private in order to test it.

  3. [...] year, Basil Vandegriend put out a concise and helpful post on writing good unit tests. Most people agree tests are important, but many do not know precisely how to make them work. Basil [...]

  4. Michal says:

    Read about private field/method accessor. There is a possibility in Java to change visibility and use private methods and fields.

    import java.lang.reflect.Field;
    import junit.framework.Assert;

    /**
    * Provides access to private members in the application classes.
    */
    public class PrivateFieldAccessor {

    public static Object getPrivateField (Object o, String fieldName) {

    /* Check we have valid arguments */
    Assert.assertNotNull(o);
    Assert.assertNotNull(fieldName);

    /* Go and find the private field… */
    final Field fields[] = o.getClass().getDeclaredFields();

    for (int i = 0; i

  5. Deepak says:

    Information provided is useful to some extent, but it would be helpful if you clearly specify how to write good junit test cases with examples. Right now you have explained what not to do.

  6. @Deepak, thanks for your comment. The article does provide guidance on how to transform the ‘what-not-to-do’ examples into better tests. I explicitly wrote the article this way to provide some concrete guidance to people to write better tests, but you are right that I do not provide instructions on the basics of writing a good test case. I am tentatively planning to write some more articles on unit testing in the near future, so stay tuned.

  7. Rick Pingry says:

    I would like to know more about writing tests for trivial code. When is the code NOT trivial? Where is the line between trivial code and non-trivial code? Can anyone provide some solid examples?

  8. @Rick, there are a few guidelines I can provide about when the code is not trivial. The two solid examples I provided in the article where getter & setter methods with no logic and simple constructors (that just populate fields from parameters passed in). Another example would be delegate methods that simnply delegate execution to another class. I.e.
    private Bar delegate;

    public void doStuff(Foo foo) {
    delegate.doStuff(foo);
    }

    The general principle is this: how confident are you that the code is correct? If you are 100% confident, then this is trivial code that you do not need to test. If you have doubts or uncertainties, then you should write a test.

  9. tinca says:

    Would you write cases that test contracts? For instance: when null is passed IllegalArgumentException must be thrown? It may seem a trivial one, but on the long run it can protect again accidental changes.

  10. @tinca, I often do write tests for a method based on its contract (preconditions & postconditions). However, I seldom test for the negative case (e.g. that the method fails with an exception if a precondition is violated). Instead, I focus on testing that the method behaves properly when the preconditions are met. E.g. if an argument can be null, then I verify that the method accepts a null as well as a non-null value.

  11. Aleksei says:

    In fact, quite unuseful article… Most of thing are too obvious but the principles of designing test cases are not covered.

  12. Shashank says:

    Basil, I am looking for guidance on writing a test suite for set of java files doing either persistance of xml to DB or doing manipulation, validation of Data in DB. any references doing the same would be of great help.

    Thanks.

  13. @Shashhank, there are many options for testing database interactions. If you need to do lots of data setup, I suggest checking out DBUnit. My preferred approach, assuming you are using Spring for transaction management, is to use Spring’s test framework so that each test involving the database executes as a single transaction that is rolled back, thus minimizing the problems from having tests change data in the database.

«    »