-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detach the exception hierarchy from AssertionError #4
Comments
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 In any case, let's see what the community has to say. |
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? |
+1 This is something I actually wanted to bring up. 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:
PracticalConsider a simple retrying logic which re-executes a block of code upon failure, i.e. when it throws an exception.
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.
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. |
Introducing a more generic way to provide additional information for debugging upon assertion failures. The DebugInformation is key-value based and can contain actual any information. In addition to this extended the ValueWrapper (now renamed to ValueDescriptor as it *describes* the value) so that it does some effort to serialize objects applying some strategies. This way we could also provide images as for example for UI tests. The current state neither includes documentation, more tests and misses the commonly used exception for all exceptions provided inside here. This approach also addresses issues: * ota4j-team#20 support for exception creation strategies * ota4j-team#4 detach AssertionError from exceptions here * ota4j-team#9 partially, as you the debug information provided here should eventually merge into a standard test report.
As the decision about ota4j-team#4 is not yet clear and some points (support of old frameworks/tests) this is an alternative introducing the AssertionError again to the hierarchy... with the drawback of duplicated code.
@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 I'd tell that giving up the |
Since JUnit 5 was released and uses these classes, we can't change this now. We should close this bug. |
Indeed, thanks! |
You miss a point here. |
@alb-i986 But I admit, this does not look like a typical case ;-) and that's why a mocking framework should be used. |
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.
The text was updated successfully, but these errors were encountered: