Does this unit test cover all edge cases? [closed]

Question

I want to test a simple controller and I want to know if this unit test covers all cases:

Controller:

@RequestMapping(method = RequestMethod.GET)
public List<Tags> getAllTags(@RequestParam(value = "query", required = false) String query,
        @RequestParam(value = "limit", required = false) Integer limit,
        @RequestParam(value = "start", required = false) Integer start,
        @RequestParam(value = "approved", required = false) Boolean approved,
        @ActiveLanguage Language language,
        @ActiveUser User user) {
    if (limit == null) {
        limit = 10;
    }
    if (start == null) {
        start = 0;
    }
    if (approved == null) {
        approved = true;
    }
    if (query != null) {
        return tagService.findTagsByQuery(user, query, limit, start, language, approved);
    } else {
        return tagService.findTags(user, limit, start, language);
    }
}

Test:

@Test
public void testGetAllTags_query_notnull() throws Exception {
    List<Tags> tagsList = createTagList();

    //Individual parameters
    String query = "test";
    Boolean approved = true;

    Mockito.when(this.tagServiceMock.findTagsByQuery(user, query, limit, start, language, approved)).thenReturn(tagsList);

    List<Tags> t = tagController.getAllTags(query, limit, start, approved, language, user);

    assertSame(tagsList, t);
    assertNotNull(t);
    assertNotNull(tagsList);

    verify(tagServiceMock, times(1)).findTagsByQuery(user, query, limit, start, language, approved);
    verifyNoMoreInteractions(tagServiceMock);

}

@Test
public void testGetAllTags_query_null() throws Exception {
    List<Tags> tagsList = createTagList();

    //Individiual Parameters
    String query = null;
    Boolean approved = true;

    Mockito.when(this.tagServiceMock.findTags(user, limit, start, language)).thenReturn(tagsList);

    List<Tags> t = tagController.getAllTags(query, limit, start, approved, language, user);
    assertEquals(tagsList, t);
    assertNotNull(t);
    assertNotNull(tagsList);

    verify(tagServiceMock, times(1)).findTags(user, limit, start, language);
    verifyNoMoreInteractions(tagServiceMock);

}

Is there anything left that is not covered by this unit test?

Answer 1

As far as I can see the @RequestParam annotation can have a defaultValue parameter - once that's used the method becomes a bit simpler:

@RequestMapping(method = RequestMethod.GET)
public List<Tags> getAllTags(@RequestParam(value = "query", required = false) String query,
        @RequestParam(value = "limit", required = false, defaultValue = "10") int limit,
        @RequestParam(value = "start", required = false, defaultValue = "0") int start,
        @RequestParam(value = "approved", required = false, defaultValue = "true") boolean approved,
        @ActiveLanguage Language language,
        @ActiveUser User user) {
    if (query != null) {
        return tagService.findTagsByQuery(user, query, limit, start, language, approved);
    } else {
        return tagService.findTags(user, limit, start, language);
    }
}

I'd also consider some edge cases for query, like the empty string or a string with just whitespace for that matter.

Now for the unit tests, the naming could probably be improved, but I also don't pay that much attention to it to be honest.

If the tagsList (or any other part like mocking methods etc.) is a common thing in the tests consider moving it into the setUp method (or equivalent) so it doesn't clutter up the test methods.

The assertSame is fine, you're basically just testing whether the object is passed through directly, however the "not null" tests afterwards are useless - tagsList shouldn't (can't) be null, so why is it being verified here?

times(1) is default for Mockito, so leave it out. Also the verifyNoMoreInteractions is a bit discouraged in general since it might make tests more brittle than they'd have to be, also since you're not gaining much from it here I'd leave it out too.

IMO since this is at the outermost layer I'd rather have an integration tests that actually checks the parameter parsing and validation too - this particular test doesn't have that much value considering you're just passing through values.