rdblue commented on a change in pull request #2684:
URL: https://github.com/apache/iceberg/pull/2684#discussion_r649370729
##########
File path: api/src/test/java/org/apache/iceberg/AssertHelpers.java
##########
@@ -39,12 +42,11 @@ public static void assertThrows(String message,
Class<? extends Exception> expected,
String containedInMessage,
Callable callable) {
- try {
- callable.call();
- Assert.fail("No exception was thrown (" + message + "), expected: " +
- expected.getName());
- } catch (Exception actual) {
- handleException(message, expected, containedInMessage, actual);
+ AbstractThrowableAssert<?, ? extends Throwable> check =
assertThatThrownBy(callable::call)
+ .withFailMessage(message)
+ .isInstanceOf(expected);
+ if (null != containedInMessage) {
+ check.hasMessageContaining(containedInMessage);
Review comment:
Can you post an example of the output when these checks fail? One nice
thing about using `handleException` is that the original exception is added to
the `AssertionError` as a suppressed exception so you can still see it. Is that
done by the new library?
Also, why not rewrite all of the methods here, including
`assertThrowsCause`? We can probably remove `handleException` if we are moving
to the new library.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
