Fix for issue #687: handle AssertionErrors and AssumptionViolatedExceptions by default again

[UrsMetz]

Change the default behaviour of the ExpectedException rule back to handling AssertionErrors and AssumptionViolatedExceptions by default to fix issue #687, i.e. one does not have to call ExpectedException#handleAssertionErrors() or ExpectedException#handleAssumptionViolatedExceptions() to make the rule handle those exceptions.

[dsaff]

Sounds good to me. @stefanbirkner, one last chance to argue we should maintain the 4.11 behavior, rather than reverting to 4.10's behavior?

[stefanbirkner]

I will check the new code with regard to #121 and #98.

[UrsMetz]

@dsaff @stefanbirkner I added another commit that just changes of the name of one of the test classes because the renaming I did while implementing this pull request made no sense (but I only realised this today :-/)

[stefanbirkner]

AssertionError and AssumptionViolatedException are two exceptions, which are used by JUnit to influence the test run. An AssertionError signals the JUnit framework that an assertion does not apply to the current test. An AssumptionViolatedException signals the JUnit framework that it should skip a test.

The ExpectedException rule handles exceptions but IMHO it should not handle AssertionError and AssumptionViolatedException because this intercepts the messaging between the test and the JUnit framework.

AssumptionViolatedException

The following test should be skipped (like in JUnit 4.11):

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

    @Test
    public void test() {
        thrown.expect(NullPointerException.class);
        assumeTrue(false);
        throw new NullPointerException();
    }
}

But the following test should be successful, because someone explicitly tests for AssumptionViolatedException:

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

    @Test
    public void test() {
        thrown.expect(AssumptionViolatedException.class);
        assumeTrue(false);
    }
}

AssertionError

The following test should fail because of 'just another reason' (like in JUnit 4.11):

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

    @Test
    public void test() {
        thrown.expect(NullPointerException.class);
        fail("just another reason");
        throw new NullPointerException();
    }
}

But the following test should run successful, because someone explicitly tests for AssertionError:

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

    @Test
    public void test() {
        thrown.expect(AssertionError.class);
        fail("just another reason");
    }
}

Fix
I extended the ExpectedException (stefanbirkner/junit@c4e761e), but I'd like to review my own code before I create a pull request.

[stefanbirkner]

The fix is already done. I still revise the documentation of ExpectedException. Hopefully it will be finished on Monday.

[stefanbirkner]

It's done. See #720.

[dsaff]

@stefanbirkner, sorry, it's been a busy couple of weeks.

I think what we want is a situation in which ExpectedException catches all exceptions, and evaluates them against its matcher. If the exception matches its expectation, it returns normally (the test passes). If the exception does not match its expectation, then it throws something:

1. If it's a "JUnit-special" exception type (AssertionError or AssumptionViolatedException), throw the exception directly.
2. Otherwise, wrap the exception with an explanation of what was expected and what was thrown.

(Optionally, we could create a method that would allow customization of which exceptions fall into category #1 and #2)

This would seem to simultaneously resolve the desires in bug #687 and also #121.

Have I convinced everyone? :-)

[stefanbirkner]

@dsaff Maybe, I'm clairvoyant ;-) #720 is doing this. The handle... methods can be used to move AssertionError or AssumptionViolatedException from category 1 to 2.

[dsaff]

@stefanbirkner, ah, yes, you're right. I overskimmed #720. I'll comment more there.

[dsaff]

I think #720 completely supersedes this one now.

[dsaff]

Thanks for re-starting the discussion--it was really valuable.