Tips for writing readable tests

Following is list of best practices I try to follow when writing tests.

Test method name should describe the tested behavior

//not so good
void searchPosts(){...}

//better
void searchIncludesAllUsersIfNoUserIdsProvided(){...}

Test method name should be as specific as possible

//not so good
void invalidLogin(){...}

//better
void loginFailsWhenPasswordDoesNotMatch(){...}

Test should be short (avoid tests which assert different scenarios)

//not so good
void findHolidaysByUser() {
  ....
  holidays = holidayRepository.findByUser(userId, today, tomorrow)
  assertEquals(...)

  holidays = holidayRepository.findByUser(userId, tomorrow, nextYear)
  assertEquals(...)
}
//better
void findHolidaysWhereStartingDateIsBetweenProvidedDates() {
  holidays = holidayRepository.findByUser(userId, today, tomorrow)
  assertEquals(...)
}

void findHolidaysWhereEndingDateIsBetweenProvidedDates() {
  holidays = holidayRepository.findByUser(userId, tomorrow, nextYear);
  assertEquals(...);
}

Note that these test methods actually tell what is the desired behavior while single test method gives no information about the tested behavior.

It’s OK to have long test names

Although sometimes when test names get too long it is good indicator that test class should be split into multiple smaller test classes (and possibly class under test should be split too :))

Avoid ANY logic inside tests

//bad sample
void searchOrdersResultsByStartingDate() {
  results = repository.findHolidays(...)

  for (Holiday holiday : holidays) {
    if (last != null) {
      assertTrue(last.getStartingDate().after(activity.getStartingDate()))
    }
    last = activity;
  }
}  
//better sample
void searchOrdersResultsByStartingDate() {
  save(holiday1, holiday2, holiday3)

  results = repository.findHolidays(...)

  assertEquals(holiday1, results.get(0))
  assertEquals(holiday3, results.get(1))
  assertEquals(holiday2, results.get(2))
}

In this sample we actually know the correct order of holidays since we inserted them in the same test.

Never assume database state

//bad sample
testProject = projectRepository.findById(6);

Try to minimize database traffic when testing DAOs/Repositories

//bad sample
void setUpTestData() {
  testUser = userRepository.findAllUsers().get(0);
}

//better
void setUpTestData() {
  User testUser = new User();
  save(testUser);
}

Clean database before running your test (in setUp)

Although well written tests should clean up system state after their are executed it is still possible that some legacy tests don’t do this or someone manually inserted some data.

Avoid mocking simple objects

Mocks created by some special mocking library like EasyMock or Mockito make test code more complicated than setting up simple objects. I have seen that people who are new to using mocking libraries start mocking every little dependency they have. In general I find that such dynamic mocks always make the test more complex.

Avoid tests with extensive mocking

If your test looks something like below then you should probably either rewrite it as integration test without mocks or simply delete it. Sooner or later you will do something that causes this test to fail and you will spend lot of time understanding what exactly does it means that this test fails now.

...
EasyMock.expect(projectService.getCurrentUserProjects()).andReturn(projects);
EasyMock.expect(projectModelConverter.convertModel(p1)).andReturn(pdto1);
EasyMock.expect(projectModelConverter.convertModel(p2)).andReturn(pdto2);
EasyMock.expect(session.getUser()).andStubReturn(user);
EasyMock.expect(userProjectGroupService.getByUser(user.getId())).andReturn(projectGroups);
EasyMock.expect(projectService.getJiraInstances()).andReturn(jiras);
EasyMock.expect(userService.getAllUsers(true)).andReturn(userList);
EasyMock.expect(listItemDtoConverter.convert(u1)).andReturn(userItem);

...
    
InitialDataRpcFacade facade = ...
InitialDataDto data = facade.loadInitialData();
  
//assertions

EasyMock.verify(session, userService, projectService);
EasyMock.verify(userProjectGroupService, listItemDtoConverter, projectModelConverter);

Always mock DAOs/Repositories when testing higher layers e.g services or facades

Unless you are doing higher level testing of course.

Avoid using dependency injection container (e.g Spring) for unit tests

Spring should only be used if we for example want to test Spring XML bean definitions.
For testing DAOs we only need minimal set of beans that can be set up manually.

When asserting if collection returned contains the right items use the toString

This trick I learned from my colleague Risto.

List result = service.doSomething();
assertEquals("[product1, product2]", result.toString());

Implement toString() so that it will be easy to use in tests (also see previous tip)

//bad sample}
String toString() {
  return new ToStringBuilder(this).append(field1).append(field2).append(id).toString();
}
//better
String toString() {
  return id;
}

Now in some cases you may want to use toString also for debugging info in logs. I suggest to use something else for this purpose. Probably you don’t want this to be turned on all the time in production anyway so one option is to use some reflection based solution e.g BeanUtils.

Don’t implement extensive equals for extensive testing

//not so good
boolean equals(Object other) {
  ...
  User user = (User) other
  return new EqualsBuilder()
        .append(getName(), other.getName())
        .append(getPersonalCode(), other.getPersonalCode())
        .append(getDateOfBirth(), other.getDateOfBirth())
        ...
        .isEquals()
}

Sometimes this is done to test if persistence works correctly. However, I find that this kind of changes in production code are not justified even if it helps testing certain cases. Better alternative is to implement custom assert that checks all fields and leave equals to check only the business identity.

//better
boolean equals(Object other) {
  ...
  User user = (User) other
  return new EqualsBuilder()
        .append(getPersonalCode(), other.getPersonalCode()).isEquals()
}


void assertEquals(User expected, User actual) {
  assertEquals(expected.getName(), actual.getName())
  assertEquals(expected.getPersonalCode(), actual.getPersonalCode())
  assertEquals(expected.getDateOfBirth(), actual.getDateOfBirth())
}
Advertisements

5 thoughts on “Tips for writing readable tests

  1. svenfila

    Very good tips!

    Regarding toString() and equals()/hashCode(), I’d argue that always implement them keeping in mind production code only. In tests, assert the results as you need. This may make asserting in tests a little longer, but I think it’s worth it – production code is free of any test related stuff and tests are more explicit.
    The trick shown by Risto regarding toString() is a really good one.

    Sven

    Reply
    1. Urgo Post author

      Thanks!
      I agree on equals and hashCode part. Regarding to toString I would in most cases use it for testing only. For extensive debugging and logging in production I would use something that uses reflection or implement other method specially for that purpose. For example Groovy has built-in support for that via Object.dump()

      Reply
  2. Ürgo Ringo Post author

    Reblogged this on Ürgo Ringo's blob and commented:

    I wrote this post some time ago. Decided to update it primarily due to inspiration got from Growing Object-Oriented Software, Guided by Tests book

    Reply
  3. Pingback: Ignite OÜ  —  Tips for writing readable tests

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s