Should the default stub of JUnit asserts like assertNotNull really be NotNull?

[agentgt]

I am aware of checker/src/main/java/org/checkerframework/checker/nullness/permit-nullness-assertion-exception.astub

But I have to seriously wonder why the default is not the above particularly since JUnit does not actually throw NPE on assertNotNull but rather a junit specific assertion exception?

It seems like this would hurt uptake of the library and doesn't really offer any advantage. I also could not find the junit4 astub so is this a blanket of all methods named assertNotNull?

I have feeling this has probably been discussed before but I just could not find it.

[mernst]

Thanks for your message.

Could you take a look at https://checkerframework.org/manual/#annotate-normal-behavior (this is the first search hit for "junit" in the manual) and see whether that is helpful in explaining the rationale? Your comments are welcome.

JUnit does not actually throw NPE on assertNotNull but rather a junit specific assertion exception

You are right about that. From a user's point of view, the program has crashed due to misuse of null. The user doesn't care the specific cause; the programmer's goal is to prevent crashes due to misuse of null. Neither NullPointerException nor AssertionFailedError is acceptable at run time. I will adjust the text at the beginning of the Nullness Checker chapter to clarify the goal, which is slightly broader than preventing just NullPointerException.

I also could not find the junit4 astub so is this a blanket of all methods named assertNotNull?

JUnit 5 annotations appear in file checker/src/main/java/org/checkerframework/checker/nullness/junit-assertions.astub. You are right that a file of JUnit 4 annotations should be added. We would gladly accept a pull request for that.

[agentgt]

I'm just not sure the authors of JUnit share the same thoughts as JUnit is designed to crash in a specific context and it is their library to annotate ideally.

Do you think they would have annotated it like that? I would be curious to ping them what their thoughts are on that.

My opinion is it doesn't really crash because it is null. It throws an expected exception that the framework uses and catches because your assertion is wrong which happens to be null.

I understand the rationale checker has and I think that part of doc explains it but each library is going to probably have their own slant on that.

My concern is what people will do which I did cause I'm lazy and don't use assertNotNull that often is put @SuppressWarnings on the test classes.

Regardless I can accept whatever decision as I can plug the stub file I'm just afraid that it will inhibit uptake of checker and or worse inhibition to write tests.

[agentgt]

Perhaps an example of how this came up might shift the mindset. I have a method called @Nullable Object findOrNull(input);. With out assertNotNull being nullable I would have to do:

@Nullable Object o = findOrNull(input);
if (o == null) {
  fail("o should not be null for this test");
}

See I think Checker is caught up on the idea because it says "assert" that it is defensive programming when JUnit is not defensive programming like Object.requireNonNull or the Preconditions in Guava. JUnit is all about writing tests as easy as possible and absolutely is not going to crash. It is going to catch the exception and run the other tests. Furthermore it isn't going to have an error exception but a failure exception which is totally expected.

If I did Object.requiresNotNull on the other hand JUnit would report that as Error not Failure because an NPE.

In fact if I have checker setup you should never need assertNotNull if it can't take nullable. Why would I ever make a failure exception for something that is an error? That is using assertNotNull on a guaranteed NonNull because if it does fail I'm getting a failure and not an error. That is bad.

Just because something throws an exception when passed null does not mean it is a crash state ESPECIALLY because JUnit has even defined that assertNotNull as FAILURE and not ERROR. If you are just basing it on it always throws an exception when null that then we are violating the "don't look at the code but the contract" which is throughout the manual.

Anyway I'm sorry to get more contentious about it but I have hard time believing that the JUnit assertNotNull is not designed for nullables to be passed.

[kelloggm]

I find @agentgt's argument that assertNonNull in JUnit specifically is not for defensive programming to be convincing.

Personally, I would use assertNonNull in a test if and only if I:

1. thought that it would be an error if the value was nullable, but
2. I could not prove it was nonnull (and thus felt the need to write a test)

In general, if I can't prove something to myself, then it seems like a mistake to ask the checker to prove it for me: usually, the checker is less expressive than the sort of proof that one can do in one's head (this is usually a good thing!).

[mernst]

I'm not ignoring this, just busy. Thanks for the thoughtful comments! I will reply shortly.