Support Truth Subject#check

[ben-manes]

The test assertion library support will infer from an assertThat statement for use in a test method. A more advanced user may want to write custom assertions, known as a Subject, in order to create their own reusable ones. This internally uses the check method to use the nicely formatted error messaging subsystem. Currently NullAway does not understand this for reasoning about the possibility of null dereferences.

In this example the check("value").that(value).isNotNull() precedes the check...containsValue(value) and emits a warning that containsValue requires a non-null parameter.

  private void checkValue(BoundedLocalCache<Object, Object> bounded,
      Node<Object, Object> node, @Nullable Object key, @Nullable Object value) {
    if (!bounded.collectValues()) {
      check("value").that(value).isNotNull();
      if ((key != null) && !bounded.hasExpired(node, bounded.expirationTicker().read())) {
        check("containsValue(value) for key %s", key)
            .about(map()).that(bounded).containsValue(value);
      }
    }
    checkIfAsyncValue(value);
  }

/Users/ben/projects/caffeine/caffeine/src/test/java/com/github/benmanes/caffeine/cache/LocalCacheSubject.java:381: error: [NullAway] passing @Nullable parameter 'value' where @NonNull is required
            .about(map()).that(bounded).containsValue(value);
                                                      ^
    (see http://t.uber.com/nullaway )
  Did you mean '@SuppressWarnings("NullAway") private void checkValue(BoundedLocalCache<Object, Object> bounded,'?

The benefit is that test methods can continue to stay focused on the behavioral aspects instead of having repeated logic that inspects implementation details. For example inspecting the listener configured on the cache so ensure the correct events were emited.

assertThat(context).removalNotifications().withCause(REPLACED)
    .contains(context.original()).exclusively();

---

Another scenario to support is assertWithMessage(msg).that(subject).isNotNull(). This is an assertThat where the message can be supplied for a better description.

I've also had cases where the null assertion was not taken into account and it warns about the following statement. I believe this is because I have my own FutureSubject and MethodNameUtil looks for concrete classes rather than at the polymorphic type.

assertThat(future).isNotNull();
future.complete(Int.MAX_VALUE);

[msridhar]

Thanks for the report. On this part:

The benefit is that test methods can continue to stay focused on the behavioral aspects instead of having repeated logic that inspects implementation details. For example inspecting the listener configured on the cache so ensure the correct events were emited.

assertThat(context).removalNotifications().withCause(REPLACED)
    .contains(context.original()).exclusively();

I didn't quite get the connection to nullability for this example?

Conceptually, adding the requested support is not difficult. It's just a bit of a pain to implement. We can look into it, though I think we have higher-priority issues to deal with at the moment.

[ben-manes]

Thanks, it’s not much of an issue. I think only once or twice, but maybe in a larger code base it will be desirable. I’d backlog until others ask for it, or close as informational.

The last was merely an example for why custom assertions is nice and unrelated. Most developers don’t realize they can do this or that it’s the intended usage of the assertion library. I wanted to show that in case it came across as an obscure use-case.

[cpovirk]

From the perspective of Truth, recognizing isNotNull() calls on subtypes of Subject does sound nice, as does recognizing the assertWithMessage pattern :) (There are probably good examples of both somewhere in Error Prone, like maybe in ChainedAssertionLosesContext for the latter?)

For what it's worth, I generally discourage running nullness checking on tests, since the cost of NPE there is generally lower, though I do see some advantages to it, and I'd be interested in thoughts from either of you, given that you may have given it more thought than I have.

One other note on check: check is interesting because a failed check doesn't necessarily abort execution. It does when it happens in the context of an assertThat call, but it doesn't when used with Expect, which collects potentially multiple failures for later reporting, rather than immediately throw AssertionError. Given that, it arguably makes sense for a nullness checker to believe, after check(...).that(foo).isNotNull(), that foo could still be null. (It arguably also makes sense to just not worry about that :))

[ben-manes]

For what it's worth, I generally discourage running nullness checking on tests, since the cost of NPE there is generally lower, though I do see some advantages to it, and I'd be interested in thoughts from either of you, given that you may have given it more thought than I have.

I agree that I typically wouldn't enable this for tests given the low maintainability benefit to noise ratio. In this case, since some assertion support exists, extending it slightly could be nice for those rarer cases where it is enabled (but as a lowest priority ticket). Caffeine seems popular at 250M Maven Central downloads in 2024 and, though I lack a comparison metric and believe it is mostly uncached CI runs, that is high enough to deserve extra care.

Therefore, I went through the exercise of enabling all of the static analyzers as a low effort approach that might unearth bugs and thankfully only found mistakes in test logic that reduced coverage. That reasoning also led me to try to maximize line coverage (99% is certainly ridiculous) which did uncover edge cases. I've found bugs each time that I try to tighten up lax testing practices, such as when covering all cases when a callback may fail (review led to a fix). Fuzz testing the spec parser found minor bugs and thankfully linearization model checking only found bugs in the tool itself. I also ported other collection library test suites (jsr, apache, eclipse, guava) in case other developers thought of a scenario that I didn't consider.

Most importantly for my own sanity, at the start I knew that the mental complexity was far too high for me to reliably implement the library or make changes. I took the unorthodox approach of brute forcing the test suite to cover all configurations and that avoided countless bugs. Similarly, the cache simulator as a test analysis tool was a necessity to explore algorithmic ideas and have a simpler reference to verify against to avoid hit rate regressions.

This is all a long winded way to say that the library pushed well past my abilities so I happily use every possible scheme to make up for my shortcomings. And, given that I rarely get to write much code anymore, its a nice way to decompress with low mental effort that keeps improving the code quality. For the average code base all of this is masochistic and developers should find their own happy balance so your general discouragement is correct.

[msridhar]

I can see things both ways (and this is based only on a limited amount of personal experience). If your whole code base is run under a nullness checker, and you can assume client code will be also, I can see value in running the nullness checker on tests as an extra check on your nullness types. E.g., you might write a function that accepts null for a parameter but you forget to annotate the parameter as @Nullable; the nullness checker probably won't directly report an error for this. But if you write a test that passes null to that function, and you're running tests under nullness checking, you'll get a NullAway error for that, which could prompt you to update the type, which in turn better documents the API for clients.

On the other hand, if you're writing a library where clients may not be nullness checked (like Caffeine), you might be adding defensive null checks in your functions, even for @NonNull parameters. If you want to test for these defensive null checks, then you might end up having to suppress a bunch of NullAway warnings on your tests.

As the Java ecosystem moves toward more and more nullness checking, I expect it will make more and more sense to run nullness checking on tests. And this makes sense in a way: for a language like Kotlin that bakes in nullness type checking, by default the checking runs on your test code as well.

PS:

Caffeine seems popular at 250M Maven Central downloads in 2024

@ben-manes just curious where do you see these stats?

[ben-manes]

oss.sonatype.org provides statistics privately to the artifact owner, although half the time this dashboard is broken and fails to load. It retains a 12 months history, offers no comparisons wrt popularity, and the only user analysis is unique IPs (perhaps is meaningless in cloud CI). I downloaded the csv export, taking Dec 2023 instead of projecting, which sums to 250,327,009. It is a feel good number like github stars, likely meaningless but also kind of neat. I have no idea about more popular libraries, like Guava, though JUnit recently posted ~50M/month for v5 (122M across major versions).