Verifier and similar rules should validate if MultipleFailureException is thrown

[Ambeco]

ErrorCollector and MultipleFailureException are incredibly useful tools, but feel only half integrated into Junit. ExternalResource and TestWatcher both execute their functionality even if the test throws an exception, and if that functionality itself throws, then these accumulate the multiple exceptions into a MultipleFailureException. However, Verifier (and thus, almost all rules that extend it), does NOT run if the test throws a MultipleFailureException, which is a missed opportunity. This is especially ironic since ErrorCollector itself extends Verifier, so if a test has multiple ErrorCollector rules for any reason (inheritance or toolchain RuleChains), then only the innermost ErrorCollector collects errors. ErrorCollector and Verifier ought to work nicely together.

Verifier ought to wrap its base.evaluate(); call in a try/catch, and if it catches a MultipleFailureException, then it ought to execute .verify(). It should accumulate the MultipleFailureException exceptions and any new exception(s?) thrown by .verify(), and throw those as a new MultipleFailureException.

We could consider catching all exceptions and running .verify() and rethrowing the original exception afterward, but for tests failing with non-MultipleFailureException, running post-test-verification almost certainly doesn't make sense. It would virtually always fail, providing noise instead of value, so I do not propose executing it for any exception other than MultipleFailureException. It is worth noting that this reasoning only really applies to exceptions thrown from the test method itself, so ideally we do want to run post-test-verification if inner Rules' tear-downs throw exceptions, but usefully, existing JUnit rules only throw MultipleFailureException in teardown, so no additional work is needed.

I haven't yet examined the junit-5 code to see (A) this has already been fixed, (B) 5 has new class/method names, or (C) if 5 rules now throw exceptions besides MultipleFailureException in teardown. I understand and accept that no new junit-4 releases are planned, and that if this feature is accepted, then the changes will probably only be implemented in junit-5.

[kcooney]

Thanks for the detailed write-up.

A change like this would likely break existing users of Verifier, so even if we were considering making another release of JUnit 4 we would need to provide a way for users to maintain the current behavior.

I'm personally not convinced that a Verifier should run if the Statement that follows it raises a MultipleFailureException. There could be numerous reasons why such an exception may be thrown (including a test that has a single rule chain that has a Verifier rule is around an ErrorCollector rule).

I'm also confused about your proposed change. Let's say you have a custom subclass of Verifier to check some post conditions. You then use that class in a test that also uses ErrorCollector and you put the Verifier around the ErrorCollector. You seem to be proposing that a test method that indicates multiple failures (via ErrorCollector) would result in your verifier's verify() method being called, but a test that that raises an AssertionError would not result in the verify() method being called.

It is worth noting that this reasoning only really applies to exceptions thrown from the test method itself

A TestRule doesn't know whether the next Statement in the chain runs the test method.

but usefully, existing JUnit rules only throw MultipleFailureException in teardown, so no additional work is needed.

People can write their own rules. They are free to throw MultipleFailureException in apply() before calling the next Statement in the statement chain. In fact, classes that extend ExternalResource can raise a MultipleFailureException in before().

I haven't yet examined the junit-5 code

JUnit 5 has a different extension mechanism (see the Extension Model section of the JUnit 5 User Guide). JUnit5 issues can be filed at https://github.com/junit-team/junit5/issues