Backwards wildcard bound on ExpectedException.expectCause()

[tavianator]

The signature of that method is expectCause(Matcher<? extends Throwable> expectedCause). This accepts Matcher<RuntimeException>, but possibly passes it other types such as IOException, resulting in heap pollution.

On the other hand, it forbids Matcher<Object>, even though that would be completely type-safe.

The type should be Matcher<? super Throwable> instead.

[kcooney]

I don't see why the current API would result in class cast exceptions. Note that Matcher.matches() takes an Object regardless of what T is bound to. If you look at TypeSafeMatcher, it ensures that the item is the expected type before doing a cast. Other matcher implementations either check the type before casting or don't require a cast.

Can you give an example of code that compiles that uses expectCause with a standard Hamcrest matchers/APIs)which results in a problematic exception?

It's true that developers would get a compile time error with the current API when trying to pass a Matcher<Object>) (for instance, if the developer used nullValue() or passed an Object to isSame() or equalTo()) but it sounds rare and avoidable.

I can see someone passing a subclass of Throwable to equalTo() (which is quite reasonable) and your proposed change would make that a compile time error.

[tavianator]

Ah you're right that ClassCastExceptions shouldn't happen, I forgot that the matcher interface accepts Object instead of T. It's still wrong IMO taking PECS into account (producer extends, consumer super), but not going to cause heap pollution after all.

And yeah, my suggestion would break expectCause(equalTo(e)) so I guess that's not tenable. Is there a use for the wildcard bound at all? There's already expect(Matcher<?>), so maybe expectCause(Matcher<?>) would make sense.

I only noticed this by trying expectCause(instanceOf(IllegalStateException.class)), which was easily fixed by using any(...) instead. So maybe no change is really necessary.

[kcooney]

The signature of instanceOf() looks broken, which leads to this problem. You can explicitly qualify the generic signature, but it's simpler to avoid instanceOf() and use any()

[kcooney]

Turns out I was wrong... the signature isn't broken. The two methods are used for different purposes:

hamcrest/JavaHamcrest#39

So I'm actually not sure there is a good reason to use Matcher<? extends Throwable> here. If we were adding the matcher APIs to ExpectedException today, either Matcher<Throwable> or Matcher<? super Throwable> would have been good choices.

Given that, and given the signature of expect(), I think changing expectCause() to take Matcher<?> is reasonable

[UrsMetz]

Changing expectCause() to accept Matcher<?> has the downside that something like

thrown.expectCause(contains("foo"))

isn't caught by the compiler anymore. IMHO in a type checked language like Java the method signatures shouldn't only check that the arguments technically work for what is done inside the method but also that the semantics are okay (as far as that's possible).

[kcooney]

@UrsMetz that is true, bit that also holds true for expect(). Also, if you passed a Matcher<String> the test would never pass.

Again, if we wrote the API today I would not suggest Matcher<?> but we have released this API so we have some constraints.

We may be willing to "correct" the generics in JUnit5 (because users will be told to expect some changes, and we will likely still make high-impact big fixes to 4.x after 5.0 is released).

That all being said, a lambda-based API for making assertions on exceptions is the long-term plan.

[UrsMetz]

@kcooney I once raised an issue (#610) regarding the signature of expec(). The issue with "better" generics on these methods is that (at least with some older Java versions) one (sometimes) has to give the type argument explicitly. Otherwise the compiler won't be happy. :-/

[marcphilipp]

I agree that Matcher<? super Throwable> would actually be correct. Changing it would break backwards compatibility and make it hard to use on Java versions below 8. Thus, I would go for Matcher<?> because it allows to use Matcher<Object>.

[kcooney]

Note that we are considering removing our Hamcrest dependencies, so you might want to file this bug against the java-hamcrest project (https://github.com/hamcrest/hamcrest-junit).

@npryce I would suggest that java-hamcrest 1.x.x.x should use Matcher<?> for the argument for expectCause() and java-hamcrest 2.x.x.x should use Matcher<? super Throwable>