1
\$\begingroup\$

I'm working on a new service, for the past year I haven't needed to write tests from scratch. I've either been bug fixing, or adding features that require modification of tests, but not writing everything from scratch.
I need to make sure I'm doing things right. The unit tests below test a service that queries a database for data.

The *Proc files use Springs @Repository annotation, and each one calls a different stored procedure in the database. For these tests I'm mocking everything. Have I done it correctly? Is there anything I have overlooked? Anything I've possibly missed? The tests all pass when I run them in my IDE or with maven. In this particular instance those method calls simply return the object(s) retrieved from the database, so there isn't any actual business logic in there apart from calling the get() method from the proc objects that I'm mocking.
Have I done things the correct way?

    @RunWith(MockitoJUnitRunner.class)
    @SpringBootTest
    public class FusServiceImplTest {

        @Mock
        private GetFileInfoByIdProc getFileInfobyIdProc;
        @Mock
        private GetFileInfoProc getFileInfoProc;
        @Mock
        private GetFileInfoByFileNameProc getFileInfoByFileNameProc;
        @Mock
        FileInfoVO voMock;
        @Mock
        FileInfoVO voMock2;

        @InjectMocks
        private FusServiceImpl service;

        @Test
        public void whenGetFileById_thenReturnFile(){
            final Long ID = 99999L;

            when(getFileInfobyIdProc.get(ID)).thenReturn(voMock);



    FileInfoVO voResult = service.getFile(ID);

        assertThat(voResult.getFileName())
                .isEqualTo(voMock.getFileName());


    }

    @Test
    public void whenGetFileByName_thenReturnFile(){
        final String NAME = "test.file";

        when(getFileInfoByFileNameProc.get(NAME)).thenReturn(voMock);

        FileInfoVO voResult = service.getFile(NAME);

        assertThat(voResult.getFileName())
                .isEqualTo(voMock.getFileName());


    }

    @Test
    public void whenGetAllFiles_thenReturnAllFiles(){

        List<FileInfoVO> list = new ArrayList<>();
        list.add(voMock);
        list.add(voMock2);

        when(getFileInfoProc.get()).thenReturn(list);

        List<FileInfoVO> voListResult = service.getAllFiles();

        assertThat(voListResult.size())
                .isEqualTo(list.size());


    }

}
\$\endgroup\$
  • \$\begingroup\$ You can Mock your database at the DAO level, but it probably more productive to actually setup a test database on a loopback address or with a Type 1/2 JDBC driver. tutorialspoint.com/jdbc/jdbc-driver-types.htm Code Review is not the correct place to post this kind of question, it's purpose is to review existing code. \$\endgroup\$ – Martin Spamer Dec 2 '18 at 13:12
  • \$\begingroup\$ Thanks for your response, I'll check that out. I'm not sure of your distinction with existing code though, is it because I'm actively working on it? I was about to commit it and was looking for a code review from here before I pushed it. \$\endgroup\$ – bot_bot Dec 3 '18 at 7:43
2
\$\begingroup\$

Something that stands out is that calling voMock.getFileName() on mocked instances will always return null as no mock response has been defined. So your validation logic of

assertThat(voResult.getFileName())
            .isEqualTo(voMock.getFileName());

essentially winds up being

assertThat(voResult.getFileName())
            .isEqualTo(null);

This could end up giving false positives where voResult is incorrect, but the test passes because the filename is null.

You would need to specify a filename for the response object by calling when(voMock.getFileName()).thenReturn("TestFile"). This way your test can actually determine that the returned object is the expected one by checking the unique expected value of voMock.getFileName().

Alternatively, if the requirement is that the instance returned from the service method is the same instance that the *Proc method returns, just avoid calling getFileName when you're verifying the test result. You could instead use something like assertSame(voMock, voResult) to ensure that the instance returned by the service method is the same one that is returned by the *Proc method.

I would always also recommend writing fail/error cases for your class as well. I couldn't give an example without seeing the FusServiceImpl class, but it can sometimes help with spotting issues like the one I listed above. For instance, if you have a test case like

@Test
public void whenGetFileById_thenReturnFile(){
    final Long ID = 99999L;

    when(getFileInfobyIdProc.get(ID)).thenReturn(voMock);



    FileInfoVO voResult = service.getFile(ID - 1);

    assertThat(voResult.getFileName())
            .isNotEqualTo(voMock.getFileName());


}

and that test case passes, you know you've probably made a mistake in the test logic. You can also validate that the method fails in the correct manner - does it return an object with null filename? A null return value? Or raise an exception? It's the non-ideal cases that will crash applications in production, so make sure to test for those cases as well.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I went with assertThat() in the end, as that makes more sense. \$\endgroup\$ – bot_bot Dec 3 '18 at 7:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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