ExpectedException#expect() should take Matcher<? extends Throwable> as its argument

[UrsMetz]

This is a "re-post" of an issue originally opened at Junit: junit-team/junit#610. As JUnit is going to depreciate and eventually remove ExpectedException I'm re-raising it hear in order to start a discussion whether this change should be made:

Consider the following test:

public class WrongUsageOfExpectedExceptionApi {
    @Rule
    public ExpectedException thrown = ExpectedException.none();

    @Test
    public void wrongUsageOfExpectedExceptionApi() throws Exception {
        thrown.expect(RuntimeException.class);

        // thrown.expectMessage() should be used here
        // because an exception cannot start with a string
        thrown.expect(startsWith("some message"));
        throw new RuntimeException("some message");
    }
}
...

When you have something like this in a test it can be quite hard to figure out why the test fails. At the moment ExpectedException#expect takes a Matcher<?> as its argument. By changing it to take only arguments of the type Matcher<? extends Throwable> the compiler could spot the mistake in the test given above. I tried the code changes needed for the suggested change in the API with the current master (92a3f73) and all tests pass. I understand that these changes break the API and non-generic Matchers won't work with the new signature of the method but is there any other reason not to have ExpectedException#expect only take arguments of the type Matcher<? extends Throwable>? Apart from the exclusion of non-generic Matchers the only downside I see is that now you have to specify the generic type: That is you have to write for example

ExpectedException#expect(CoreMatchers.<Throwable>not(CoreMatchers.<Throwable>instanceOf(type)));

instead of

ExpectedException#expect(not(instanceOf(type)));

[UrsMetz]

junit-team/junit#1073 should also be considered when discussing this issue.

[npryce]

This is definitely an improvement.

The problem with instanceof and not(instanceof(...)) is that instanceof has the wrong signature, which will be fixed in Hamcrest eventually

[UrsMetz]

@npryce Should I open a pull request with the suggested changes? @tavianator Do you want to tackle the issue with ExpectedException.expectCause() you mentioned in junit-team/junit#1073?

Considering the discussion in #11 and junit-team/junit#1150: I'm not sure whether the changes would just be a small step in the "migration process" to having hamcrest-junit extend the "new" assertion-library-agnostic version of ExpectedException and whether those discussions should already be taking into consideration or postponed to a late point in time.

[tavianator]

@UrsMetz I'm happy to do that. But for similar reasons to that bug, I believe that expect() should take a Matcher<? super Throwable>, not a Matcher<? extends Throwable>.

[UrsMetz]

I just had a look at the code of ExpectedException in hamcrest-java (should have done that before :-/; I thought it was identical to that from JUnit 4.12 but it is not): expectCause already takes a Matcher<? super Throwable>. @npryce: As you changed expectCause to take a Matcher<? super Throwable> I guess the propossed pull request should also make expect take a Matcher<? super Throwable>?

[kcooney]

FYI: The JUnit team just updated ExpectedException.expectThrows() in JUnit to take in a Matcher<?> (it used to take Matcher<? extends Throwable> ).

[kcooney]

BTW, the title of this bug is wrong. You should mot modify expectThrows() to take in Matcher<? extends Throwable>, because that would prevent callers from using CoreMatchers.notNullValue(). It could be changed to Matcher<? super Throwable>