Unit testing Video Service class

Question

I wrote my first unit testing code on a service class using Mockito. The code looks like this:

// Test Class
public class TestVideoService {
 private VideoService videoService;
 @Mock private VideoRepository videoRepository;
 @Rule public MockitoRule mRule = MockitoJUnit.rule();
 @Before
 public void setUp(){
 this.videoService = new VideoService(videoRepository);
 }
 @Test
 public void testGetAllVideos() throws Exception{ 
 when(videoRepository.findAll()).thenReturn(new ArrayList<Video>());
 assertEquals(new ArrayList<Video>(), videoService.getAllVideos(), "tested");
 }
}

I also have several questions. I feel like I'm just comparing the same mock values (ArrayList of Video) – what's the point of doing this, or am I just doing it the wrong way?

Some other classes:

@Service
public class VideoService {
 @Autowired
 private VideoRepository videoRepository;
 public VideoService(VideoRepository videoRepository){
 this.videoRepository = videoRepository;
 }
 public List<Video> getAllVideos(){
 return videoRepository.findAll();
 }
}
@Repository
public interface VideoRepository extends JpaRepository<Video, Long> {
}

• Martin Schröder: You are still using JUnit4: Switch to JUnit5.

Answer 1

Welcome to code review and thanks for sharing your code.

When writing UnitTests keep in mind that they are documentation. Test code has less restrictions for identifier names. This means that method names are usually longer and you should not "shorten" your unit test methods.

1. The test name should describe the expected behavior in detail

   @Test
   public void getAllVideos_returnsCompleteListFromRepository() throws Exception{ 
    // ...
   

2. A unit test always has three parts: arrange, act and assert. You should always have this three parts visible in your test methods.

   In your example you do the act part implicitly in the assert line. This prevents you from introducing variable that could give useful information about the result:

   @Test
   public void getAllVideos_returnsCompleteListFromRepository()
    // arrange 
    List<Video> allVideosInRepository = new ArrayList<>();
    when(videoRepository.findAll()).thenReturn(new ArrayList<Video>());
    // act
    List<> allVideosFromRepository = videoService.getAllVideos();
    // assert
    assertEquals(allVideosInRepository, allVideosFromRepository, "tested");
   }
   

   For the same reason the String parameter of the assert* method should always be a useful hint in case the test fails because it is part of the error message:

    assertEquals(allVideosInRepository,
    allVideosFromRepository,
    "is same object");
   

---

I feel like i just compare the same mock values ArrayList of Video and what's the point of doing this or i just do it wrong way.

This is not a problem of your unit tests but of your current design.

For the time being your VideoService class is nothing more than a facade to a Collection, there is no business behavior (yet) to be tested.

• l0b0: The AAA pattern is interesting, I learned them as Given, When and Then.
• Timothy Truckle: @l0b0 Yes, it they are synonym, but IMHO it is easier to memorize that your tests should have AAA quality rating (like stocks)... ;o)

Answer 2

The idea of the test class is ok: you want to test the service, mock the repository and you check on the outcome.

In the test, it would be nice to use the AAA (arrange/act/assert) pattern. In my tests, i call these setup, execute and verify.

There is no logic in the service layer, and your method returns the database objects. Also you only have one test, with an empty arraylist. What if you find 1,2,..1000 items. What would you return, the whole list or the first 10,... Additional questions you can have: In what order are they returned? Are you handling errors,...

Imagine some logic on your service layer and these tests will get more to it

Answer 3

Some notes:

1. If you're using mocks you are pretty much by definition doing integration rather than unit testing, because mocks enable you to test how a piece of code integrates with something else having a specific behaviour.
2. The primary goal of tests is to gain confidence that the code does what you expect. A sub-goal of this is clarity - the test should be as simple as possible while demonstrating something new and non-trivial about the code under test. Because of these I would inline setUp until you add more test cases which definitely need the exact same preconditions.
3. A useful pattern for test names is that they should have a name starting with the word "should" which says exactly and only what the test is meant to demonstrate. This is much more helpful than the "test" prefix, which encourages test names which only say which method is being tested. So if you're trying to test that getAllVideos simply forwards the response from the video repository's findAll you could name it something like getAllVideosShouldReturnVideoRepositoryFindAllReturnValue. At this point it's pretty obvious that the service isn't actually doing anything interesting yet — a better implementation would have no video repository at all — you might as well skip this test and think of the simplest interesting thing that the code could do, such as counting videos, filtering videos by some property, or something else entirely.
4. The third argument to assertEquals is only useful so long as it actually conveys something about the test which the assertion output doesn't already convey. This becomes super clear with TDD, because you always run the failing test before implementing the production code, so you get to check whether the error message is useful enough to debug the problem when the test inevitably fails in the future.

• Timothy Truckle: to 1. No, it is the opposite. The definition of a unit test is to test the logic of your unit in isolation, and you need Mocks to cut of possibly changing behavior of other units your unit under test depends on.
• Timothy Truckle: to 4: The name of the test method should describe the business behavior expected while the String argument of the assert* method should clarify the technical expectation not met.
• vikingsteve: Not really.... I think point 1 is mistaken
• asaf92: I really disagree with the first point. A class has dependencies that are (hopefully) loosely coupled through an interface. In unit-testing, you are testing a single unit of code (a class). Mocking the dependencies is the only way to test one unit, while instantiating and initializing all the dependencies is an integration test.

Answer 4

Testing strategy seems like it might be a bit out of scope for Code Review, but having historically also found this process a bit confusing, my suggestion would be to not explicitly test your VideoService class. Test its consumers.

It's not necessary (or desirable) for there to be a one-to-one mapping of test classes to implementation classes. Your entry point to your tests should be your API - the point from which you can perform a useful operation (this can be your definition of a "unit") - in the case of a REST API this is probably your controller class. You might still end up with the situation where you end up with a mock repository which does the same as you have now, but you should be able to test all the other collaborators using their actual implementations rather than mocks. This way you will end up with less brittle tests, since you won't be mocking out all your collaborators and you won't have to update all those mocks if some behaviour changes.