Create better Unit test

Question: Is there anything you could do in order to improve the test?

I am using Spring Framework 4, Hibernate 4, JUnit 4. All DAO unit tests inherit from TestDaoSetup class:

@ContextConfiguration(locations={
        "classpath:/config/test/spring.xml",
        "classpath:/config/test/hibernate.xml"
})
@TransactionConfiguration
@Transactional
public class TestDaoSetup {
}

One of unit tests:

```
@RunWith(SpringJUnit4ClassRunner.class)
public class CityDaoImplTest extends TestDaoSetup {
@Rule
public ExpectedException exception = ExpectedException.none();

@Autowired
private CityDao cityDao;

@Autowired
private CountryDao countryDao;

@Test
public void create_Created(){
CountryEntity countryEntity = countryDao.find(1L);

CityEntity cityEntity = new CityEntity();
cityEntity.setName("Košice");
cityEntity.setCountryEntity(countryEntity);

cityDao.create(cityEntity);

Assert.assertNotNull("Expected not null value.",cityDao.find(cityEntity.getId()));
}

@Test
public void create_NoCountryAssociation_ExceptionThrown(){
exception.expect(ConstraintViolationException.class);

CityEntity cityEntity = new CityEntity();
cityEntity.setName("Košice");

cityDao.create(cityEntity);
cityDao.flush();
}

@Test
public void create_ExistingNameSameCountry_ExceptionThrown() {
exception.expect(ConstraintViolationException.class);

CountryEntity countryEntity = countryDao.find(1L);

CityEntity cityEntity = new CityEntity();
cityEntity.setName("Bratislava");
cityEntity.setCountryEntity(countryEntity);

cityDao.create(cityEntity);
cityDao.flush();
}

@Test
public void create_ExistingNameDifferentCountry_Created(){
CountryEntity countryEntity = countryDao.find(2L);

CityEntity cityEntity = new CityEntity();
cityEntity.setName("Bratislava");
cit

External data

Most of your test cases are expected that some records already exist. The details of these records are not visible in the class, which makes them an external dependency. It also makes the test cases harder to understand.

A simple fix is to add the necessary test data in the test cases, or in the @Before method.

Make assumptions explicit

Some of the test cases are hard to understand because they make assumptions that are not exactly obvious, for example:

@Test
public void create_ExistingNameDifferentCountry_Created(){
    CountryEntity countryEntity = countryDao.find(2L);

    CityEntity cityEntity = new CityEntity();
    cityEntity.setName("Bratislava");
    cityEntity.setCountryEntity(countryEntity);

    cityDao.create(cityEntity);

    Assert.assertNotNull("Expected not null value.", cityDao.find(cityEntity.getId()));
}

Here, I suppose you are testing that you can create a city with the same name in a different country. This test case seems to make some assumptions:

• There is no city named Bratislava in the Country with id=2

• There is a city name Bratislava in some other Country

It's better to make these assumptions part of the code, for example:

private CountryEntity createNewCountry() {
    // TODO: create a new unique country (different from all existing)
}

@Test
public void create_ExistingNameDifferentCountry_Created(){
    CountryEntity someCountry = createNewCountry();
    createCity("Bratislava", someCountry);

    CountryEntity anotherCountry = createNewCountry();

    CityEntity cityEntity = new CityEntity();
    cityEntity.setName("Bratislava");
    cityEntity.setCountryEntity(anotherCountry);

    cityDao.create(cityEntity);

    Assert.assertNotNull("Expected not null value.", cityDao.find(cityEntity.getId()));
}

There are no more meaningless numbers (such as 2L) in this test case, and the assumptions are loud and clear.

Another similar example:

@Test
public void update_ExistingNameSameCountry_ExceptionThrown(){
    exception.expect(ConstraintViolationException.class);

    CityEntity cityEntity = cityDao.find(2L);
    cityEntity.setName("Bratislava");

    cityDao.update(cityEntity);
    cityDao.flush();
}

This test case seems to assume that the city with id=2 is named Bratislava. Consider this instead:

@Test
public void update_ExistingNameSameCountry_ExceptionThrown(){
    exception.expect(ConstraintViolationException.class);

    CityEntity cityEntity = cityDao.findSomeCity();
    cityEntity.setName(cityEntity.getName());

    cityDao.update(cityEntity);
    cityDao.flush();
}

No more hidden assumptions and invisible external data. The city can be any city, if you try to create a new city with the same name as the sample city, the method should throw.

Many of your other methods suffer from the same problem. Review them and ask yourself if there are any assumptions that are not written inside the method itself, and make them explicit.

Covering all corners

Some of your test cases could be more strict, for example:

• In create_Created, in addition to checking that you can load the new entry that was created, you could check that:

• the number of entries in the database got increased by 1

• Assert.assertEquals(cityEntity, cityDao.find(cityEntity.getId()))

• Similarly in create_ExistingNameDifferentCountry_Created

Expected exceptions

Have you considered this instead of the @Rule for expected exceptions:

@Test(expected=ConstraintViolationException.class)
public void create_NoCountryAssociation_ExceptionThrown(){
    // ...
}

Personally I find this more readable. @Rule is more powerful, because it can match exception messages too, but you're not using that feature anyway. This may be a matter of taste.

Naming

I think your test case names could be more intuitive, for example:

• create_city_with_country_works instead of create_Created

• create_city_without_country_throws instead of NoCountryAssociation_ExceptionThrown

• ...

As for camelCase vs underscores vs mixed vs something else, I think it doesn't matter. You can use what you like best. The general naming rules probably don't apply to the test cases the same way as to regular code. Test case names should be specific, identify what is being tested, and as such they tend to be very long.

For example, underscores are very similar to spaces, so create_city_without_country_throws is just a little bit harder to read than a sentence. In createCityWithoutCountryThrows there is no visual separation between the words, which makes it much harder to read than a sentence. At least this is my opinion.

The bottom line is, use whatever naming is agreed in your team, or else whatever naming is more readable to you.

StackExchange Code Review Q#48624, answer score: 11