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.

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?

share|improve this question
add comment

3 Answers 3

up vote 14 down vote accepted

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.

share|improve this answer
    
Thank you for your suggestion. I agree, that the name of the method could be better, but the name which you suggest is really very long. Therefore, in my opinion, it would be better to put the information into Javadoc comment. At least something like "Test creation of a user with email ..." is way better readable than "TestCreationOfAUserWithEmail ... ". –  Yoda Apr 19 at 18:09
    
some IDE's (IntelliJ comes to mind) doesn't like it if the first word is not test, regardless if the @Test annotation is present. IMO that is a bad idea on their part, but sometimes we have to live with what we have –  Robert Snyder Apr 19 at 18:09
4  
Why would it matter how long the method name is? It's something you will never call yourself. All that name has to do is show up in your test result window and be as descriptive as possible, you gain nothing by shortening. –  Jeroen Vannevel Apr 19 at 18:11
    
@JeroenVannevel I am getting your point. In that case it is better to use more descriptive name. –  Yoda Apr 19 at 18:17
1  
@Yoda: C# habit, in Java it should be lowercase. My mistake –  Jeroen Vannevel Apr 19 at 18:59
show 3 more comments

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.

share|improve this answer
    
Good points. All tests inherit from class TestDaoSetup which has ContextConfiguration(... configuration locations here ...), TransactionConfiguration and Transactional annotation. I like your second point. I will use it for sure. Unfortunatelly, I can not mark more answers as correct, they should really add it to this site. –  Yoda Apr 19 at 19:13
    
That doesn't have to, we are here to help each other out ;) –  chillworld Apr 19 at 19:18
add comment

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.

share|improve this answer
    
Thank you for your code review and suggestions which you provided. –  Yoda Apr 19 at 18:10
    
+1. This test really does look like it's testing too much at the same time. –  Jules Apr 20 at 12:20
add comment

Your Answer

 
discard

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

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