Writing better JUnit tests

Question

I am using Spring Framework, Hibernate and JUnit. I am testing persisting of a UserEntity.

The UserEntity has the following associations:

• ManyToMany - City
• ManyToMany - ActivityCategory

• ManyToOne - UserType

  @Test
  public void testCreateNotExistingEmail() {
      UserTypeEntity userTypeEntity = userTypeDao.find(1L);
  
      BCryptPasswordEncoder encoder = new BCryptPasswordEncoder(16);
      String password = encoder.encode("123456");
  
      List<CityEntity> cityEntities = new ArrayList<>();
      cityEntities.add(cityDao.find(1L));
      cityEntities.add(cityDao.find(2L));
  
      List<ActivityCategoryEntity> activityCategoryEntities = new ArrayList<>();
      activityCategoryEntities.add(activityCategoryDao.find(4L));
      activityCategoryEntities.add(activityCategoryDao.find(5L));
  
      UserEntity userEntity = new UserEntity();
      userEntity.setEmail("user@domain.com");
      userEntity.setIsActive(true);
      userEntity.setPassword(password);
      userEntity.setUserTypeEntity(userTypeEntity);
      userEntity.setCities(cityEntities);
      userEntity.setActivityCategories(activityCategoryEntities);
  
      userDao.create(userEntity);
  
      UserEntity userEntityCreated = userDao.find(userEntity.getId());
  
      Assert.assertNotNull(userEntityCreated);
      Assert.assertEquals(2, userEntityCreated.getCities().size());
      Assert.assertEquals(2, userEntityCreated.getActivityCategories().size());
  }
  

Is there anything I could do better in the test?

Answer 1

A minor remark: your test is named testCreateNotExistingEmail. The @Test annotation already indicates that this is a test so it doesn't have to be repeated in the methodname itself.

Adding to that: I like to structure my tests in the format [UnitOfWorkName]_[ScenarioUnderTest]_[ExpectedBehaviour].

It is hard to tell what exactly your test does when I look at the code because the methodname indicates something about a nonexisting email, which leads me to think about an exception handled somewhere.

But when I look at the code I see no assertions like that at all. In fact, I only see an email being used once in a seemingly irrelevant context.

Some type of clarification is needed: a better method name. Something along the lines of createUser_WithNonExistingEmail_ShouldReturn_StandardCitiesAndCategories

Now when the test fails amongst a big group of tests, you can pinpoint more closely what scenario is going wrong exactly before you dig into it.

Answer 2

The name of your test method is testCreateNotExistingEmail but I don't see how it's related to what you're testing.

What is the purpose of the test? Is it that you can successfully create a user entity if there is no other user entity with the same email? If so, then I would do something like this instead:

@Test
public void testCreateNotExistingEmail() {
    UserEntity userEntity = new UserEntity();
    userEntity.setEmail("nonexistent@domain.com");
    userDao.create(userEntity);
}

@Test(expected=UniqueKeyIntegrityException.class)
public void testCannotCreateWithExistingEmail() {
    String email = "user@domain.com";

    // or something like this:
    Assert.assertNull(userDao.findByEmail(email));

    UserEntity userEntity = new UserEntity();
    userEntity.setEmail(email);
    userDao.create(userEntity);  // success

    userEntity.setEmail(email);
    userDao.create(userEntity);  // throws
}

---

Do you need to set all those fields for the test? If not, then omit them. For example if a user without password is valid, then don't set a password in this test case, as it's unnecessary. Unit tests should be as short as possible.

---

Keep in mind that every unit test method should test only one specific thing (though that can of course include multiple assertions). I would separate the tests targeted on UserEntity itself and the tests that verify the correct relationships (with city and category).

---

Your tests on the relationships (with city and category) could be more strict. It would be better to compare the ids of each city and each category in userEntity and userEntityCreated. I would use separate test methods:

• One test for city relationships
• One test for category relationships
• For extra safety, one more test with city + category. This can be simplified, for example compare only the sizes as you already did.

---

Beware of potential side effects. For example, if you have multiple tests that use userDao, make sure to reinitialize in @Before so that all tests start with the same setup and don't have side effects on one another.

Answer 3

The answers already given are good ones but I see that there are some points what aren't said.

First :

Your test does include some DB, maybe a generated when you test or a dev database on a server.
Is you class @Transactional or not?
Personally I find it better that you make your tests @Transactional when you will do a save operation.

Why :

You will be sure that your db is rolled back when your method ends.
This means, setting the DB back in original state as in the begin of the method.
Executions of tests can change in order so this can lead to failing of previous/further tests.

Second:

Assert.assertNotNull(userEntityCreated);
Assert.assertEquals(2, userEntityCreated.getCities().size());
Assert.assertEquals(2, userEntityCreated.getActivityCategories().size());

Why do you not provide a text message with the assert for when it fails like this :

Assert.assertNotNull("userEntity is not createrd",userEntityCreated);
Assert.assertEquals("Size of city's is not correct",2, userEntityCreated.getCities().size());
Assert.assertEquals("Size of ActivityCategory is not correct",2, userEntityCreated.getActivityCategories().size());

Why: When your test fails you will see this error message, so you know directly what assert is failing in your method.

Hope this can help you further.