0
\$\begingroup\$

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?

\$\endgroup\$
5
  • 2
    \$\begingroup\$ generally speaking for each if branch you should have a test case. just based off of that rudimentary rule, no \$\endgroup\$
    – Dan
    Commented Sep 11, 2016 at 10:52
  • \$\begingroup\$ I would agree - but being honest the "limit" and "start" thing is kind of not well written anyway and i don't want to test that. Is it ok besides the "limit" and "start" case? \$\endgroup\$ Commented Sep 11, 2016 at 11:01
  • \$\begingroup\$ I don't think there is currently enough information to answer your question. What do the find tags functions do? What parameters are they expecting? Can they throw errors? Is your code responsible for handling them if they are thrown? ... \$\endgroup\$
    – forsvarir
    Commented Sep 11, 2016 at 12:04
  • 1
    \$\begingroup\$ "anyway and i don't want to test that". If you don't want to test it, why do you ask us whether you've covered everything? \$\endgroup\$
    – Mast
    Commented Sep 11, 2016 at 12:31
  • \$\begingroup\$ I'm asking more if the general approach is fine - so yes -> i should test limit and start. But as I said - is it ok besides that? \$\endgroup\$ Commented Sep 11, 2016 at 12:46

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$

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