Policy describing the difference between "check failed" and "could not evaluate check"

[cpovirk]

assertThat(myList).containsExactly("foo", "bar");

If myList is null, we throw NullPointerException. This is different than calling fail() -- and not just because NullPointerException is different than AssertionError. The more important difference is when the user uses or assume() rather than assert(). In that case, the difference is between a failing test and a skipped test. (A less severe case but still problematic case is expect().)

I think that we're doing the right thing in throwing NullPointerException: "Does null contain "foo"?" is best answered not with "No, it doesn't" but rather with: "The question you're asking is malformed. Please fix it." But we should officially Decide this and then document it.

[cpovirk]

Hmm, but possibly we should check for null and throw NullPointerException ourselves. That way, we can provide a better failure message. I mean, it probably won't be lots better, but if the user has provided a custom failure message or name, it would be nice of us to include it.

[cgruber]

How do people feel about "doing the right thing" with respect to bad test writing (e.g., throwing index out of bounds if you check for a negative length on a string), but we trap that and throw a TruthInvalidTestException which wraps it, for cases where we know damned well you wrote a bad test, not that you have a failing test.

Any traction on that?

[cgruber]

Either way, we should include a best practices for writing Truth checks, per the above, and include this information there, and abide by it internally, at least.

[kevinb9n]

I don't completely follow why TruthInvalidTestException would be useful.

On Fri, Dec 11, 2015 at 1:40 PM, Christian Edward Gruber < [email protected]> wrote:

Either way, we should include a best practices for writing Truth checks, per the above, and include this information there, and abide by it internally, at least.

— Reply to this email directly or view it on GitHub https://github.com/google/truth/issues/207#issuecomment-164053915.

Kevin Bourrillion | Java Librarian | Google, Inc. | [email protected]

[cpovirk]

Random: A thing that we could do is to surface any withFailureMessage or named values in the message of any NullPointerException, etc. But I think it's probably not worthwhile: It's going to require more API, and it will likely come up rarely, perhaps typically during the initial writing of the tests, when the error message is arguably less important.

[cgruber]

On considering further, I think the policy of throwing AssertionError (or ComparisonFailure) where appropriate, but allowing test-errors to just bubble up seems appropriate. And that's what we're doing, so no work there, thankfully.

I'm not entirely sure what's worth documenting about it, insofar as that's kind of how I have always interpreted JUnit treating AssertionError and other Throwables differently.

So.. this feels like... documenting the already documented JUnit behavior, as opposed to anything more meaningful. :/ Can you think of a way to frame this in documentation that would be better?

[cpovirk]

My main reason for wanting to document this is discussions like https://github.com/google/truth/issues/253#issuecomment-241201450, which point out the advantages of throwing AssertionError for "bug in your test." It's easy to forget about assume() in these cases and do something that will break it.

I'm not sure of the right framing at the moment. Maybe it should be part of a general point about "Remember that not everyone is using assert":

• so don't throw AssertionError (use fail)
• so don't fail multiple times for a single assertion (I think we have an internal bug about how this affects expect)
• so throw exceptions for bogus checks instead of using fail

[dimo414]

We're inconsistent here, currently, too - at least in ThrowableSubject we fail() on nulls in order to support chained assertions.

We'd discussed making Subject.actual() NPE if the input is null, and adding a Subject.nullableActual() method to support the few tests (like .isNotNull()) that actually work with null inputs.

[cpovirk]

Another place that we're inconsistent about this is within OptionalSubject/GuavaOptionalSubject: isPresent/isEmpty/isAbsent fails for a null input, ~while hasValue throws for a null input~. [edit: sort of but not in the way I meant. See my following post.] All those seem equally bogus.

Those also suggest to me that my phrasing of "bug in your test" is not a great one: A test legitimately might want to check for, say, an empty optional. And if the code under test accidentally returns null, then the test is catching a real problem in the code under test. (This can also hold for my original example of a Collection.) The point is more "Something went wrong before we could even perform the test that you wanted, so we're not going to claim that the condition 'was met' or that it 'was not met.'"

[cpovirk]

Oops, no, I misread: The case that hasValue throws for is hasValue(null). (While even that could indicate a bug in the prod code, it does sound more likely to mean "a bug in the test" :))

In contrast, assertThat(nullOptional).hasValue(foo) fails in the same way that assertThat(emptyOptional).hasValue(foo) or assertThat(optionalWithValueBar).hasValue(foo) fails—for better or for worse. (At this particular moment, I don't want to get back into the tradeoffs between NPE and failure for the null case.)