TypeSafeDiagnosingMatcher/CombinableMatcher conflicts with nullValue Matcher

[mmichaelis]

The following assertion never evaluate to true:

assertThat(null, either(equalTo(something)).or(nullValue()))

The root cause lies in the CombinableMatcher being a TypeSafeDiagnosingMatcher which implements matches() as:

public final boolean matches(Object item) {
    return item != null
        && expectedType.isInstance(item)
        && matchesSafely((T) item, new Description.NullDescription());
}

Thus for null the matchers will never be queried. I suppose the solution should be something like:

public final boolean matches(Object item) {
    return (item == null || expectedType.isInstance(item))
        && matchesSafely((T) item, new Description.NullDescription());
}

[UrsMetz]

Today I also was puzzled why assertThat(null, either(is(something)).or(nullValue())) did yield an AssertionError.
But I think this can't be easily fixed easily because the JavaDoc of TypeSafeDiagnosingMatcher explicitly states that null is checked by TypeSafeDiagnosingMatcher and won't be passed to matchesSafely, cf. the JavaDoc. So a change of this would result in a breaking change for existing code. But I agree that this is confusing and maybe an eitherNull().or() construct or even something more pluggable would be great to have.

[hughwphamill]

Proposed a solution here: #56

[sf105]

If that's the behaviour you want, then you probably want a different matcher. The point of a TypeSafe matcher is that confirms that the actual value has the right type, which null doesn't.

I'm also wondering about the use case for "either something or null" in an assertion. Surely you know which one is appropriate when you write the test?

[UrsMetz]

Maybe the fact that one wants to use either(equalTo(something)).or(nullValue()) might be a hint that the design or the API has some problems. But sometimes I also felt the need to use the matcher in this way and was surprised that either(equalTo(something)).or(nullValue()) didn't match even though my code returned null.

One idea might be to add a mismatch description to TypeSafeDiagnosingMatcher when null is passed to matches() so that it is more obvious for users of Matchers inheriting why the matcher doesn't match null.

[sf105]

It's up to the assertion calling the matcher to report the actual value. In this case, I would expect it to show the null.

[StarBax89]

This is caused by this little pice of code in TypeSafeDiagnosingMatcher

@Override
    @SuppressWarnings("unchecked")
    public final boolean matches(Object item) {
        return item != null
            && expectedType.isInstance(item)
            && matchesSafely((T) item, new Description.NullDescription());
    }

The item != null and expectedType.isInstance(item) is false for item = null

This can be fixed by adding this dirty hack to CombinableMatcher

@Override
  public final boolean matches(Object item) {
    if(this.matcher instanceof AnyOf)
    {
      AnyOf anyOf = (AnyOf) this.matcher;
      List<Matcher<T>> matchers = (List<Matcher<T>>) anyOf.getMatchers();
      Boolean matches = false;
      for(Matcher eachMatcher : matchers)
      {
        if(eachMatcher.matches(item))
        {
          matches = true;
        }
      }
      return matches;
    }
    else {
      return super.matches(item);
    }
  }

Therefore TypeSafeDiagnosingMatcher's matches method must be non final and ShortcutCombination needs a getter for matchers.

I know this is really dirty, maybe someone can make a fix with this information. I'll also try to write a better fix for that.

EDIT:
I also wrote a test
private static final CombinableMatcher<String> EMPTY_STRING_OR_NULL = CombinableMatcher.either(equalTo("")).or(nullValue());

@Test public void
    acceptNullOrEmptyString() {
        assertMatches("or didn't pass", EMPTY_STRING_OR_NULL, null);
        assertMatches("either didn't pass", EMPTY_STRING_OR_NULL, "");
    }