markhobson / hamcrest-compose

Hamcrest matchers for composition.
Apache License 2.0
30 stars 6 forks source link

Make ConjunctionMatcher Work For Null Values #16

Closed Megamiun closed 6 years ago

Megamiun commented 6 years ago

As noted by me in #13:

[...] compose doesn't match some values because of TypeSafeDiagnosingMatcher#matches, that reject any null value, impossibiliting constructs like compose("nullStringValue", is(nullValue()))).

And commented by @markhobson:

[...] The nullValue use-case for compose sounds like a bug, could you raise another issue for that please? Perhaps ConjunctionMatcher should extend DiagnosingMatcher instead of TypeSafeDiagnosingMatcher to fix that.

Unfortunately, it seems extending DiagnosingMatcher is the only option, as the TypeSafeDiagnosingMatcher#matches is final, so we can't override this method to bypass and accept null values.

markhobson commented 6 years ago

I have a commit for this but I'm now questioning whether this actually a valid use-case? i.e. Why would you want to compose a single null-matcher? It can't be sensibly composed with any other matcher since null doesn't have many other properties apart from being null :)

The downside to fixing this use-case is that the mismatch description for assertions like assertThat(null, personEqualTo(new Person("x", "y", "z"))) goes from:

Expected: a person with title "x"
          and first name "y"
          and last name "z"
     but: was null

to:

Expected: a person with title "x"
          and first name "y"
          and last name "z"
     but: was null
          and was null
          and was null

which is worse in my opinion.

Megamiun commented 6 years ago

Thinking a little, yes, it can have a heavy price to add this. I only have reached this problem because I tried to do a internal hasFeature implementation with a varargs, and passed the varargs directly to compose.

But I already made a solution, using single instance hasFeatureValue on those checks, so there is no problem with this barrier.

So I think I'll close this issue, @markhobson.

markhobson commented 6 years ago

Agreed, thanks for clarifying.