1
\$\begingroup\$

I am testing my repository class:

interface AccountRepository{
    void save(Account account);
    Optional<Account> findById(Long id);
}

And when writing tests, I ended up with two semantically different tests, but they essentially contain exactly the same code:

@DisplayName("user exists -- account does not exist -- save persists")
@Test
void save_userExistsAccountDoesNotExist_persisted() {
    //arrange
    User existingUser = userRepo.save(new User("John", "Smith"));
    Account account = new Account(existingUser, null);
    //act
    repo.save(account);
    //assert
    assertThat(repo.findById(account.getId())).contains(account);
}

@DisplayName("user exists -- account exists -- findById finds")
@Test
void findById_userExistsAccountExists_found() {
    //arrange
    User existingUser = userRepo.save(new User("John", "Smith"));
    Account existingAccount = repo.save(new Account(existingUser, null));
    //act & assert
    assertThat(repo.findById(existingAccount.getId())).contains(existingAccount);
}

I don't really want to remove one of them because that serves different purposes in my head but I am worried that I may have understood something wrong.

\$\endgroup\$
2
\$\begingroup\$

In my opinion, if you think that each test has a different meaning, there is nothing wrong with them.

What you can do to extract the duplication is to have a dedicated method to arrange your test. But in this case there will be no real gain.

\$\endgroup\$
2
  • \$\begingroup\$ @Sam, please consider accepting an answer if you have one. \$\endgroup\$ – gervais.b Jan 10 '19 at 15:08
  • 4
    \$\begingroup\$ It's very reasonable to wait more than three hours to see if a better reply come in before accepting the first answer. \$\endgroup\$ – Eric Stein Jan 10 '19 at 15:24
3
\$\begingroup\$

One of the hardest piece of knowledge to learn as a developer is: code duplication does not mean responsibility duplication.

Having said this, you are testing different methods so it's ok to have more than one test. In this case, you are testing complementary (or were they supplementary? I never remember it) methods, so to test one, you need to have the other one working properly.

However, there are some code duplication that will bite you in the future (but since this question is from the past, you can tell me if I'm wrong or not).

  1. You are duplicating the way a User entity is created, so if you change its constructor, you will have to modify your tests, even though it might not matter in the actual logic you are testing. It happens something similar with the Account entity. To avoid this you can have a private method or to a public builder.
  2. You are duplicating the way a User is created and persisted. The same problem as in point #1 happens. You probably don't care how or when is created, you just need it to be created and persisted properly. This applies also for the Account entity with a slight different: in the first test you need to make the code of save into the repo explicit since it's what you are testing.

Besides all of this, when you are writing tests you need to tell the reader what's important and what's not and describe that accordingly. It's a balance between explicitness and abstraction. For instance in the example, I've written a method called createAccount to encapsulate the logic of creation and persistence of the Account entity. However, someone would say that if a developer reading the test needs to know what the arrange does, it has to move to the method implementation (a test should be self-explanatory and auto).

@DisplayName("user exists -- account does not exist -- save persists")
@Test
void save_userExistsAndAccountDoesNotExist_accountIsSaved() {
    // Arrange
    User existingUser = createUser();
    Account createdAccount = makeAccount(existingUser);

    // Act
    repo.save(createdAccount);

    // Assert
    assertThat(repo.findById(createdAccount.getId())).contains(createdAccount);
}

@DisplayName("user exists -- account exists -- findById finds")
@Test
void findById_userExistsAndAccountExists_returnTheAccount() {
    // Arrange
    User existingUser = createUser();
    Account existingAccount = createAccount(existingUser);

    // Act
    Optional<Account> foundAccount = repo.findById(existingAccount.getId());

    // Assert
    assertThat(foundAccount).contains(existingAccount);
}

private User createUser(){
    return userRepo.save(new User("John", "Smith"));    
}

private Account createAccount(User existingUser){
    return repo.save(makeAccount(existingUser));
}

private Account makeAccount(User user){
    new Account(user, null);
}
\$\endgroup\$
2
  • \$\begingroup\$ I don't get point #2. Isn't it more readable if you call the repositories in the tests? Let's just have createUser and createAccount and then explicitly call repositories if we need to \$\endgroup\$ – Sam May 22 '19 at 8:53
  • 1
    \$\begingroup\$ That was exactly what I was trying to explain the following paragraph: you have to find the balance between duplication, coupling, expressiveness, conciseness... It also depends on the business, the project, the team, the language... \$\endgroup\$ – A Bravo Dev May 22 '19 at 13:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.