Java Unit Testing - Make return value of a method called by enclosing method to Empty

Question

I have this method and implementation is done. However, when I look at Jacoco report, it says that the else {} and catch() part is not being covered by Unit Test Code Coverage.

public class SomeService {
    
    public List<Something> getAll() throws SomeCustomException {
        List<Something> listOfSomething = new ArrayList<>();
        try (Data data = DataUtil.get(Constants.SOME_TABLE_NAME)){
            ResultSet resultSet = data.retrieveAll();
            if(resultSet != null && !resultSet.isEmpty()){
                Something something;
                for (ResultSetRow row : resultSet){
                    something = new Something();
                    something.setX(row.get("someColumnX"));
                    //...
                    listOfSomething.add(something);
                }
            }else{
                throw new SomeCustomException("ResultSet is null");
            }
        }catch (ExceptionOne | ExceptionTwo e){
            throw new SomeCustomException(e.getMessage(), e);
        }
        return listOfSomething;
    }
    
}

So I tried to create a Unit Test method to attempt to cover the else(){} part. However, I think what I'm doing is not correct or has no effect to the someServiceMock

Basically, I want the resultSet to be empty so that it goes to the else {} part and cover the test on else {} branch. Problem is, I'm not sure how to set it because it's inside the getAll() method

@Test
void getAllReturnsEmpty() {
    SomeService someServiceMock = mock(SomeService.class);
    ResultSet resultSetMock = mock(ResultSet.class);
    when(!resultSetMock.isEmpty).thenReturn(false);
    
    assertThat(someServiceMock.getAll().isEmpty()).isTrue();
}

Is there a way to achieve what I want? I want to hava 99% Java code coverage

Thank you

Answer 1

Like people stated in the comments, you're mocking the wrong things. You can mock static things (like "DataUtil#get"), but since this doesn't go to the core of the question, let's say we have

public class SomeService {
   Supplier<Data> dataSupplier; // set in constructor, can be () -> DataUtils.get(SOME_TABLE)

   ...
        try (Data data = dataSupplier.get()){
...

To get your coverage, you can then

// exception cases
// exception one
when(dataSupplier.get()).thenThrow(new ExceptionOne());
// exception two
when(dataSupplier.get()).thenThrow(new ExceptionTwo());
// empty retrieval cases
Data data = mock(Data.class);
when(dataSupplier.get()).thenReturn(data);
// covers != null case
when(data.retrieveAll()).thenReturn(null);
// covers != null case
when(data.retrieveAll()).thenReturn(List.of());

// verify SomeCustomException is thrown

All four of these need to be in separate test cases (with the same verification).

Answer 2

This question is complex, but every point where it gets complex it breaks apart into a few options, all of which return back to: This is a bad idea.

The nature of data.retrieveAll()

There are only two options here:

1. data.retrieveAll() is specced to be capable of returning null.
2. data.retrieveAll() is specced such that it cannot possibly return that.

If it's case #1, then just.. write your test that way. There is a way to make a data here (mock out Data.get(Constants.SOME_TABLE_NAME) to do this). However, case #1 sounds like a grave mistake:

Sub-consideration: The nature of querying all rows from a table

This breaks down into 3 options that might be considered as '.. and then it should return null'. And null is wrong for all of them:

1. The table exists and has no records. You shouldn't return null - you should return an empty result set (the first call to .next() already returns false).
2. The table does not exist and that's weird. You shouldn't return null - you should throw SQLException. It should throw something, and I think it's wrong to throw anything but SQLException, but it depends on the situation. But, throw something.
3. The table does not exist, and that's not weird at all - that's normal design of the db. In which case, there is some semantic notion that 'table does not exist' represents, and usually it is to be treated as identically to 'the table does exist, but has no rows in it'. Therefore, do not return null - return an empty result set just like case #1, because to the caller there should be no difference between 'table exists and is empty' and 'table does not exist'.

I don't know what your app does or how it is supposed to work, but it's going to boil down to one of those 3, and none of them end in '... and null could possibly be returned'.

Back to the nature of retrieveAll()

We have now proven it can only be case 2: It cannot possibly return null. It could throw something, or it could return an empty result set maybe.

Logical conclusion

You don't test for the impossible except the code that itself is specced that way: You can write a unit test for Data.retrieveAll if you must, to confirm it does not return null (which is itself impossible to get covered as, well, it can't), but you certainly can write tests that an exception is indeed thrown if you attempt to query a non-existent table, and that an empty resultset is indeed returned if you query an empty table.

Once you've got those tests, you don't retread that ground in another test. Think about it: If the test for 'code that does X using A' (X here is your getAll() method, A here is Data.retrieveAll) also implies you must be testing aspects of A, then where does it end? Program code tends to result, and this is no exaggeration, in the 'top level call' (say, somebody that loads youtube.com, the front page of youtube) resulting in literally thousands of sub-systems being invoked: The user's playlists and preferences need to be loaded, their google authentication identity needs to be loaded and confirmed, and so on.

Would you want to write a unit test for that that is 50,000 lines long, and is mostly a copy/paste of thousands of other unit tests, which are themselves 20,000 lines long because they, too, test the entire stack below them?

Of course not, that's emphatically does not scale. If Data.retrieveAll is specced to not return null you don't write code for it (your else line must go away entirely, as it triggers on an impossible scenario), and you don't test for it except in the unit tests for the retrieveAll method itself, if at all.

And once you cease writing code to cover the impossible, you no longer get a hole in coverage.

The same principle applies to the catch block. This, too, breaks down into a few steps but none of them result in a scenario where you need to accept a hole in code coverage:

1. The method can throw that exception. Then mock it out or just make an instance that will reliably throw it. For example, if you have a db query processor and you want to test SQLExceptions that fall out, just run the query "THIS IS NOT A VALID SQL QUERY;", or "SELECT 1/0;" maybe.
2. The method cannot possibly throw it. Then don't write a catch for it, and thus don't test that catch code, as it can't run. If relevant (and usually it is not), write code that ensures this exception isn't thrown, on the unit test for that method, not in your unit tests for the getAll methods). It shouldn'be listed in its throws clause.
3. Annoyingly you are implementing a method because a supertype (interface or superclass) has it, and it throws a checked exception that your implementation would never throw. However, you can simply omit that exception from the throws list of your implementation. However, if the caller (your getAll method) treats the object as its supertype, javac will still demand that you catch it (or throw it on), which may result in a catch block that cannot occur. This entire scenario is suspect - that API needs review. Possibly that exception needs to be unchecked (exceptions that end up as being literally impossible under any scenario more than once in a blue moon should be unchecked, or the API needs to be updated to separate those cases out). If you can't work around the API, we hit workaround territory: You either mock it out, or you use something like lombok's @SneakyThrows, or you fork the library, or you accept that you have a catch block that jacoco will mark off as uncovered. Stick a note on to tell jacoco you don't care and move on - we've left 'how do I write nice clean code' territory once we decided we're using an ugly library that needs workarounds to function properly.
4. The method cannot possibly throw it today but you expect it soon will in some near-future update, and you want the code you are writing that uses that method to already be written to handle that likely-relevant-in-the-near-future case, in which case you really do have to mock it out and test it. Jacoco's warning is 'correct', in that your test cases are incomplete as long as you don't test that catch block path.

If you must, use assert

It might feel bad to just leave a case such as 'what if Data.retrieveAll returns null' as completely uncovered. Not just 'there is no test for that scenario' but even 'the code doesn't even attempt to deal with that scenario'. However, you do it every day. Do you ever write this:

String v = z.toLowerCase();
assertTrue(v instanceof String);

You don't. Or, if you think you do, you've forgotten to test thousands of such impossibilities. That's why we don't test for these things: The list of 'exotic things that are impossible' is limitless, hence, it is obviously impossible to test for it all.

However, perhaps you consider that the impossibility of "data.retrieveAll() returns null" is still impossible, but 'less impossible' than e.g. your JVM somehow ending up with the object returned by toLowerCase() being a non-String. If you must address it, I suggest you add an assert. Not a junit assert, an actual assert:

assert resultSet != null: "Data.retrieveAll() broke spec and returned null";

Back to your code snippet

Your code snippet has a pretty significant bug in it, which your unit test isn't going to catch and which jacoco isn't going to fix for you either. It's fortunate: It's a nice example of why trying to cover every case just means you end up blowing your own legs off with this stuff. You've overcomplicated things and in so doing introduced a bug in code that you're likely never going to actually trigger. (Is a bug still a bug if it is impossible to trigger? You're going to have to ask a philosopher, and while you're at it, ask em if a tree that falls in a forest when no body is around to hear it still makes a sound).

The problem you have here is that if an empty (not null, but empty) is returned, your code throws a custom exception with the wrong message. The problem with 'wrong / misleading text' is that you can't unit test for those. Comments and method names have the same issue. How do you write a unit test that catches that:

int add(int a, int b) {
  return a + b;
}

Is misnamed because it should be called plus instead, add implies that this code actually changes a (which is not possible in java, but, still - wrong name)? You don't. You have to accept that messages and naming is beyond the scope of unit tests and thus beyond the scope of what jacoco can do for you.

Here's the right way to write your snippet:

List<Something> listOfSomething = new ArrayList<>();
    try (Data data = DataUtil.get(Constants.SOME_TABLE_NAME)) {
      for (ResultSetRow : data.retrieveAll()) {
        Something something = new Something();
        something.setX(row.get("someColumnX"));
        listOfSomething.add(something);
      }
    } catch (ExceptionOne | ExceptionTwo e) {
      throw new SomeCustomException(e);
    }
    return listOfSomething;
}

Note:

• It is far shorter.
• Its local variables are better scoped (there is no performance gain in declaring a local once and re-using it in that loop. Don't believe me? Run javap on both versions and be enlightened how it makes zero difference in the produced bytecode, thus trivially proving it cannot possibly make a difference)
• It just casually assumes Data.retrieveAll() couldn't possibly return null. This is good - you don't nullcheck "foo".toLowerCase() either - you trust that methods you call do what their spec says they do. Even if you don't, the effect of this is precisely what you want: A NullPointerException would occur if retrieveAll fails its spec and does return null. That's a good thing: The NPE's name is correct (the problem is indeed a null pointer being present in a place where the coder presumed it couldn't happen), its stack trace is correct (it points at precisely where this misunderstanding has occurred, and the faulty code is high in the trace), and its message is correct (on modern VMs, the message is "retrieveAll()" or some such).
• It inlines a simple single-use variable. (resultSet is gone as a variable).
• It cleaned up the creation of a wrapper-exception. You don't need to re-tread the message.
• If you feel the null cause does need some attention, there's assert. However, here you don't need to do that - the null situation is handled properly, no need for an explicit assert (it results in an NPE. Which is what you want).
• In contrast, it does not throw if the resultset is empty. However, why should it? "There are no rows" is a valid state surely. If it is not a valid state, then, sure, check it and throw something, and mock Data.retrieveAll so that you can create that state. You're now testing something that is a valid respons from Data.retrieveAll (i.e. retrieveAll is specced such that it says it is indeed possible for it to return a resultset that has zero rows in it), but is not a valid response/scenario from the viewpoint of getAll().
• However, that sounds bizarre to me. Nothing in getAll indicates to me that it converts 'no results' into an exception. That means your method is likely badly named at least. I bet the right answer is to just accept that 'no results' is just as valid as 'some results'. But if not, either getAll needs a rename or you need to ensure the context (the name of the class this is in, or the explanation in e.g. package-info.java that explains how to mentally model what this API does) needs to make it really obvious that 'no results' is unintended as a scenario and results in exceptions if it comes up.

Answer 3

Just commenting on the assertion, it is preferable to use

assertThat(someServiceMock.getAll()).isEmpty();

Not only it reads better but the error message will be explicit too (it would complain you getAll list is not empty) instead the non explicit expected true but got false