Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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");
        cityEntity.setCountryEntity(countryEntity);

        cityDao.create(cityEntity);

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

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

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

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

    @Test
    public void update_ExistingNameDifferentCountry_Updated(){
        CityEntity cityEntity = cityDao.find(3L);
        cityEntity.setName("Bratislava");

        cityDao.update(cityEntity);

        Assert.assertEquals("Expected different value.", "Bratislava", cityDao.find(3L).getName());
    }

    @Test
    public void delete_NotUsed_Deleted() {
        CityEntity cityEntity = cityDao.find(4L);

        cityDao.delete(cityEntity);

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

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

        CityEntity cityEntity = cityDao.find(1L);

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

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

EDIT:

To make test conditions more clear:

  • There are separated configurations, one is specifically for testing purposes.
  • There is database script which prepopulates the test database with test data before tests actually start.
  • Data are the same for each test, because there is a rollback after each test.
  • Test data are not prepopulated in each test class, but they are prepopulated in separate SQL file, which is executed before tests actually start.
share|improve this question
    
This is an integration test though, not a unit test. A unit test should depend on the implementation being correct only. Whereas these cases test whether the spring wire-up is correct, the test database creation script includes a unique key for city name on country table, test database enforces such keys etc. –  abuzittin gillifirca May 2 at 12:57

2 Answers 2

up vote 9 down vote accepted

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.

share|improve this answer
1  
Thanks for a nice code review, however I don't agree with the first point "Side effects". After each test there is rollback and therefore database is still in the same state for every test. I created a file on github with contents of log for create_Created test - gist.github.com/jakubpolak/d4fd091941a4ccc1cf24 –  user41403 May 1 at 8:01
1  
You're right, I didn't know that @Transactional on the class will do that. I dropped that point, thanks for the heads up! –  janos May 1 at 8:39
    
@janos Using expected = … allows test setup to make the test pass. And yes, in our project we had this happen on several occasions. You just have to use the rule correctly (see my post). I also don't think snake_case is more readable and would argue that this is a hard fact as you make it sound – it's just different for everyone. We actually had internal discussions on this and there was just no consensus. –  Ingo Bürk May 1 at 9:51
    
@IngoBürk are you saying that expected= can be buggy? I've never seen that, works reliably for me every time. About naming, my last sentence concludes to find a consensus in your team, or use what you like, period. Btw, JLS doesn't address the case of unit tests. Actually I don't see mention about camelCase either, though I definitely agree that's the convention in production code. –  janos May 1 at 10:26
    
@janos No, expected= itself is not buggy, but if you have a test expecting some exception and someone changes the test setup, the setup might now throw this exception. Since your test expects it, it passes – even though it just became useless because it's not testing the production code anymore. –  Ingo Bürk May 1 at 10:28
public void create_Created(){

This violates Java coding standards. In Java, the convention is camelCase, not snake_case. On top of that, the name create_Created is utterly meaningless. What is that supposed to mean? Give your tests names describing what they test!

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

It's good that you are using the ExpectedException rule, but you are using it wrong. Don't expect the exception until right before the call that you expect to trigger the exception – test setup etc. needs to be done before!

Some of your tests don't seem to set up the data you are using for assertions. For your test cases to be independent, you should create the entry in the database from the test and then assert that the DAO can read this value (or do whatever you're testing). Don't rely on what's in the database – it's a messy road to go down because your tests will start depending on each other, potentially causing what we call blinking tests (sometimes they pass, sometimes they fail).

Common test teardowns also belong in a @After method rather than being done in every test individually.

share|improve this answer
    
Unit tests are a different beast when it comes to method naming, using underscores is definitely a good thing because the way the methodname is built (or should be built) is a lot different than a method in a traditional project. –  Jeroen Vannevel May 1 at 1:32
    
@JeroenVannevel I disagree, I find camelCase named test methods just as readable as snake_case named tests – and in that case, the consistency argument wins. However, it is a matter of convention. In our project, we have a certain type of integration tests, called scenario tests, for which we use snake_case. The only purpose for this, though, is that the framework used for these tests generates reports from the test names that are sent to business, and snake_case allows us to generate normal sentences from the names. –  Ingo Bürk May 1 at 7:36
    
For unit test naming I use these conventions, in my opinion they are clear and logical - osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html –  user41403 May 1 at 8:04
    
Since I did some research, I decided to post this on programmers.*. –  Ingo Bürk May 1 at 8:08
2  
@IngoBürk Regarding this "it's a messy road to go down because your tests will start depending on each other" - they won't because each test has exactly the same data. There is a rollback after each test. I explained this in comment to janos's answer as well. –  user41403 May 1 at 9:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.