6

assume you have a unit test that contains these lines

assertNotNull(someVal);
assertNotEmpty(someVal);

This obviously checks that someVal is not null and is populated with something.

The question is, is the first line necessary and does it add anything to the test? Should we not just have the second line, and if it's null it will throw a null pointer exception which still indicates a failing unit test (but not a failing assertion).

What's the best practice in such simple cases?

Ashkan Aryan
  • 3,504
  • 4
  • 30
  • 44

4 Answers4

5

You shouldn't see a NPE as a test failure. You should actually assert it is not null and provide an error message,

assertNotNull(someVal, "Some descriptive message saying why value should not be null");

pengibot
  • 1,492
  • 3
  • 15
  • 35
4

It depends. Let's say you're testing the function myFunc. If myFunc returns null in non-exceptional cases, I think it's reasonable to check that the result is both non-null and the correct value because it can make your test clearer.

If null is an exceptional case though, then checking it is akin to catching a RuntimeException in your test. Just clutters your test and obscures your intent.

sharakan
  • 6,821
  • 1
  • 34
  • 61
  • I disagree, for the method to return null instead of throwing the exception, the return must be explicit. Therefore the method is designed to return null and so the null check should be performed. As Matthew states, an NPE in a test is not a good thing since the person to see the failure doesn't know if it is an error in the code or test. – John B Oct 25 '12 at 13:06
  • 1
    @JohnB Explicit, but perhaps not intended. Or it's the result of a lower level class returning null (return componentObject.getFunctionResult()). Also, consider this. That reasoning means that whenever you write a test on a function that returns an object, you should assertNotNull(). Do you do that? – sharakan Oct 25 '12 at 13:10
  • Actually yes because I do TDD and so the first thing that is done is to return some non-null value. I get that if you are not doing TDD that would not be as obvious but probably still good practice. That said, when checking each thing within the returned object I don't always check for null first. Hmmm. OK practice or lazyness? Kinda on the fence. – John B Oct 25 '12 at 13:25
  • @JohnB Interesting point about the objects you're checking on the returned object. I do think it's telling, but I would ;). Back to TDD leading you to check non-null first thing though, do you also put a try/catch around the function under test to catch RuntimeExceptions, and fail() in the catch block? If not, why is that different? – sharakan Oct 25 '12 at 13:46
  • Because of the causal line of code in the exception stack trace. If the exception was thrown in the method under test you know the error is in the method under test. If the exception is thrown from a line of code in the test it is not as obvious what has the bug (test or method under test). – John B Oct 25 '12 at 14:32
3

There is a difference between a failure and an error in JUnit. A failure is something that is expected (that is explicitly tested for with assertXXX() or fail()) and an error is something that is not, usually an Exception. See What's the difference between failure and error in JUnit?.

If you call

assertNotNull(someVal);

then you're saying that this value should not be null, and you're specifically testing for that. If you let the NullPointerException happen, then the person interpreting the error will not know whether the code under test is correct, or you just haven't thought of all of the cases.

It's a matter of intent. It's a good idea to add a explanation message as well, as others have pointed out.

Community
  • 1
  • 1
Matthew Farwell
  • 60,889
  • 18
  • 128
  • 171
2

I'm not a big fan of assertNot()'s and more generally testing unexpected things that could happen to the SUT, let alone testing them in addition to the normal Assert in every single test.

IMO you should only test for nominal states of your objects. If it is normal for someVal to be null in some circumstances, then create a dedicated test method checking that it is indeed null in that scenario. Choose an intention-revealing name for your test, like someValShouldBeNullWhen...(). Do the same for empty, or any other value.

You see that unexpected things happen to your SUTs by looking at your tests exception messages, not by trying to forecast each of these unexpected things in advance and cramming your tests with assertNots for them.

guillaume31
  • 13,738
  • 1
  • 32
  • 51