pnowojski commented on a change in pull request #12525:
URL: https://github.com/apache/flink/pull/12525#discussion_r442806170
##########
File path:
flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/tasks/StreamTaskTest.java
##########
@@ -207,9 +209,8 @@ public void testCleanUpExceptionSuppressing() throws
Exception {
testHarness.waitForTaskCompletion();
}
catch (Exception ex) {
- if (!ExceptionUtils.findThrowable(ex,
ExpectedTestException.class).isPresent()) {
- throw ex;
- }
+ assertTrue(ex.getCause() instanceof
ExpectedTestException);
+
assertTrue(Iterables.getOnlyElement(asList(ex.getCause().getSuppressed()))
instanceof FailingTwiceOperator.DisposeException);
Review comment:
Yes, right. But in case of some bug, like NPE, or some other unexpected
failure, it would be better to know immediately from the exception (visible in
Azure) to know what has happened. Instead of just getting `expected true but
was false` message.
Slightly better but IMO an overkill version would be to:
1. check if top level exception is `ExpectedTestException`, if not, throw
the original exception
2. check if there is exactly one single expected suppressed exception. if
not:
3. If there is more then one, or just one but different then expected, throw
original
4. if there is no suppressed exception, throw `Assert.fail("Missing
suppressed DisposeException");`
My proposal for double
```
if (!condition1) { throw ex; }
if (!condition2) { throw ex; }
```
Is just simpler and good enough, as it presents all of the required
information to debug the test without re-running it in the debugger/with extra
debug messages.
----------------------------------------------------------------
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]
