Keep Test Code inside the Test

2019-11-24 by Tomas Tulka

Noone wants to write one thing twice. Reducing duplicates makes code shorter and clearer. How much this applies for test code?

A few days ago I was pair-programming with another developer in the Test-Driven Development manner. TDD and pair-programming is the best fit. I usually apply TDD after having understood the problem and designed the API, in the implementation phase. This approach helps me reduce the need for later refactoring and avoid testing of trivial code.

Having the requirement reflected in the API, we wrote a test:

public interface Account {

  boolean canLogin(String password);
}

@Test
void user_can_login_with_a_valid_password() {
  Account account = new UserAccount("test", "test@example.com", "pwd1");

  assertThat(account.canLogin("pwd1")).isTrue();
}

Then we implemented the feature and wrote the next test:

@Test
void user_cannot_login_with_an_invalid_password() {
  Account account = new UserAccount("test", "test@example.com", "pwd1");

  assertThat(account.canLogin("xxx")).isFalse();
}

After implementing this we continued:

public interface Account {

  boolean canLogin(String password);

  void changePassword(String newPassword);
}

@Test
void password_is_changed() {
  Account account = new UserAccount("test", "test@example.com", "pwd1");

  account.changePassword("updated");

  assertThat(account.canLogin("updated")).isTrue();
}

At that moment my partner proposed to move the first line of the tests into a separate method, like this:

private Account account;

@BeforeEach
void setupAccount() {
  this.account = new UserAccount("test", "test@example.com", "pwd1");
}

It does follow the Don't Repeat Yourself principle and makes the test methods shorter, right? Look:

@Test
void user_can_login_with_a_valid_password() {
  assertThat(account.canLogin("pwd1")).isTrue();
}

Sounds well first, however, there are several issues.

Don't Share Code among Tests

Sharing code among different requirements is a bad idea, because business is volatile and the rules might change independently. Doing so comes with risks and drawbacks:

1. It's not obvious how the test is set up

Looking at the test only, its not clear what is the arrangement of the test, how is the unit initialized and set up. To understand this, we have to scroll up to the setup method.

2. Tests are coupled

It is impossible to have different setups for different tests, although this is often desired. Consider the following test:

@Test
void user_with_no_email_cannot_login() {
  Account account = new UserAccount("test", null, "pwd1");

  assertThat(account.canLogin("pwd1")).isFalse();
}

Such a dilemma often leads to a complex setup method trying to do more than one thing, which makes the test suite difficult to understand and maintain.

3. Tests are not well isolated

Tests must run in isolation from each other. Tests must never be meant to run in a particular order. Consider the following tests:

@Test
void user_can_login_with_a_valid_password() {
  assertThat(account.canLogin("pwd1")).isTrue();
}

@Test
void password_is_changed() {
  account.changePassword("updated");

  assertThat(account.canLogin("updated")).isTrue();
}

When the second test runs before the first one, it fails.

Even not frequent in practice, it should always be possible to run the tests in parallel. Bearing this in mind helps you to write well isolated tests.

4. Unnecessary work

Consider another test in the test suite:

@Test
void empty_password_raises_an_error() {
  assertThrows(InvalidPasswordException.class, () ->
      new UserAccount("test", "test@example.com", ""));
}

Even not needed at all, the setup method is executed and resources created, which is just a waste.

Don't Share Dependencies amoung Tests

The same is true for dependencies. Instead of doing this (with Spring):

@Autowired
private AccountRegistry registry;

@Value("${test.admin.username}")
private String adminUsername;

@Test
void registered_user_account_is_in_the_registry() {
  Account account = new UserAccount("test", "test@example.com", "pwd1", registry);
  account.register();

  assertThat(registry.byUsername("test").isPresent()).isTrue();
}

@Test
void admin_account_has_admin_rights() {
  Account account = new UserAccount(adminUsername, "test@example.com", "pwd1");

  assertThat(account.hasAdminRights()).isTrue();
}

...it's way better to this (with Spring and JUnit 5):

@Test
void registered_user_account_is_in_the_registry(@Autowired AccountRegistry registry) {
  Account account = new UserAccount("test", "test@example.com", "pwd1", registry);
  account.register();

  assertThat(registry.byUsername("test").isPresent()).isTrue();
}

@Test
void admin_account_has_admin_rights(@Value("${test.admin.username}") String username) {
  Account account = new UserAccount(username, "test@example.com", "pwd1");

  assertThat(account.hasAdminRights()).isTrue();
}

Conclusion

Write your tests so that everything the test needs is included inside the test. Letting the test code go out of its boundaries makes the test dependent on its context, which brings unnecessary complexity into the test suite.

Benefits of reducing code duplicity don't really count here, because having hard-to-maintain tests pays a much higher price than a few extra lines of code.

Happy testing!