Detach the exception hierarchy from AssertionError

[cgruber]

At this point, since this is a new library, and the major test infrastructure projects are all involved in discussing it, it might be timely to finally detach from AssertionError, whose intention was quite different than "Failing test". Since the frameworks are going to need to adapt to use this library anyway, there's no necessary payoff for such frameworks and tools to require AssertionError at the top of the chain.

I admit, there is some value in retaining it to support those libraries who decide NOT to adopt this, but that's not necessarily a goal - supporting non-adopters... is it? Should it be? If Test-ng, JUnit, and Surefire-pojo all know how to interpret opentest4j exceptions, then that's pretty much universal coverage. With that level of support, I think Truth would be fine with having an exception hierarchy that doesn't top out with AssertionError as the default (with a non-ota4j assertion as an optional extension for test harnesses that refuse to use it).

So... bottom line... I propose that we consider NOT having AssertionError be the top level exception.

[sbrannen]

Very interesting proposal.

We in fact considered such an approach but abandoned it very quickly simply due to the fact that most (if not all) testing frameworks on the JVM treat AssertionError as a test failure. With that in mind, we hope to add value without breaking backwards compatibility with existing frameworks... and at the same time we want any new sublasses of AssertionError to be transparently treated as test failures as well.

In any case, let's see what the community has to say.

[cgruber]

I have never wanted exceptions-as-interfaces more than right now. lol. It would be nice to have these APIs not be saddled with a poor life choice from way back, but allow assertion frameworks like Truth or AssertJ to make their exceptions work on either system. But as you say, let's see what others have to say.

In my mind, the worst is that if someone uses ot4j-based exceptions from an assertion library on an old runner, it may misinterpret the exception as an infrastructure failure than a test-failure. That can be mitigated by minor releases to support ot4j exceptions. But in all cases, the test still fails. It's a risk of misdiagnoses, seen immediately from the exception name. So... falsely attributed failures, not false positives. Am I correct in that analysis?

I know we could live with that at Google, as rolling out support for these in our test running infrastructure is a reasonably small effort. Thoughts?

[alb-i986]

+1

This is something I actually wanted to bring up. I think that AssertionError should be detached from the Error hierarchy, and rather extend RuntimeException.

It's quite a big change that may not have such a great practical impact, but..

"Philosophical"

As already pointed out, it would just make more sense. From Error's javadoc:

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions [..] these errors are abnormal conditions that should never occur. That is, Error and its subclasses are regarded as unchecked exceptions for the purposes of compile-time checking of exceptions.

Practical

Consider a simple retrying logic which re-executes a block of code upon failure, i.e. when it throws an exception.

public  <V> V retry(Callable<V> callable){
    try {
        return callable.call();
    } catch (Error e) {
        // OOM errors should not be swallowed
        throw e;
    } catch (Throwable t) {
        return retry(callable);
    }
}

I think it's wise to exit the retry loop in case an Error such as OOMError is thrown, therefore we need to add a catch block where we re-throw the underlying Error.

But if we use this retry logic in a test context (e.g. Selenium tests, which is my use case), this won't work, given that AssertionError is an Error. Therefore we are forced to add another catch block, handling this corner case:

public  <V> V retry(Callable<V> callable){
    try {
        return callable.call();
    } catch (AssertionError ae) {
        return retry(callable);
    } catch (Error e) {
        // OOM errors should not be swallowed
        throw e;
    } catch (Throwable t) {
        return retry(callable);
    }
}

So we end up with having duplication. This is not too bad, but it would be cleaner if the exception identifying a test assertion failure was not an Error.

[pdolezal]

@alb-i986 OK, I admit I'm got in this dicussion just by an accident, so I probably miss much your knowledge and context. However, your examples seem to me like good arguments to retain the inheritance from AssertionError. When inheriting from RuntimeException, it is IMO much higher chance that the code under the test catches the exception instead of being interrupted (and the runner perhaps does not get any chance to process the exception then).

I'd tell that giving up the AssertionError inheritance means that the testing framework should not use exceptions at all for testing purposes. But it would mean breaking the current practice and introduce even worse compatibility problems, wouldn't it?

[kcooney]

Since JUnit 5 was released and uses these classes, we can't change this now. We should close this bug.

[marcphilipp]

Indeed, thanks!

[alb-i986]

@pdolezal

However, your examples seem to me like good arguments to retain the inheritance from AssertionError. When inheriting from RuntimeException, it is IMO much higher chance that the code under the test catches the exception instead of being interrupted (and the runner perhaps does not get any chance to process the exception then).

You miss a point here. Assertions are run by tests, thus it's the test that throws the exception. Therefore the code under test will never be able to catch one. It's the test framework the only responsible for handling such exceptions.

[pdolezal]

@alb-i986 Except for the situations where a mock should be used. A poor's man solution then could be a hand-written mock that uses assertions to verify that the code under test, e.g., invokes a method on a given object with expected correct arguments.

But I admit, this does not look like a typical case ;-) and that's why a mocking framework should be used.