Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should hasMessageThat() have a null check for "Causal chain is not deep enough" like hasCauseThat() does? #665

Open
cpovirk opened this issue Feb 13, 2020 · 1 comment
Labels
P3 not scheduled type=enhancement Make an existing feature better

Comments

@cpovirk
Copy link
Member

cpovirk commented Feb 13, 2020

We added this way back in the original hasCauseThat() implementation in CL 143231718:

if (actual == null) {
check("getCause()")
.withMessage("Causal chain is not deep enough - add a .isNotNull() check?")
.fail();
return ignoreCheck()

I don't remember ever discussing whether to do it for hasMessageThat(), too, but the case seems pretty good there, too?

@cpovirk cpovirk added P3 not scheduled type=enhancement Make an existing feature better labels Feb 24, 2020
@cpovirk
Copy link
Member Author

cpovirk commented Apr 8, 2020

And as a general rule, it's unfortunate for us to NPE for an unexpectedly null part of the value under test. (Contrast to throwing NPE on an assertion like containsExactlyElementsIn(null). There, NPE makes sense.)

We recently got a contribution to avoid this in some StringSubject methods. Maybe someday we'll enable full null checking for Truth, at which point we'll be able to easily see everywhere that we need to handle null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 not scheduled type=enhancement Make an existing feature better
Projects
None yet
Development

No branches or pull requests

1 participant