[GitHub] flink pull request #5689: [FLINK-4569][tests] Respect exceptions thrown in t...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/5689


---

[GitHub] flink pull request #5689: [FLINK-4569][tests] Respect exceptions thrown in t...

[zentol]

Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/5689#discussion_r174140412
  
--- Diff: 
flink-tests/src/test/java/org/apache/flink/test/example/client/JobRetrievalITCase.java
 ---
@@ -119,6 +121,11 @@ public void run() {
lock.release();
 
resumingThread.join();
+
+   Throwable exception = error.get();
+   if (exception != null) {
+   throw new AssertionError(exception);
+   }
--- End diff --

assertNull throws a NPE. This throws the actual exception with stacktrace 
etc and is the most equivalent to just throwing the exception.


---

[GitHub] flink pull request #5689: [FLINK-4569][tests] Respect exceptions thrown in t...

[kl0u]

Github user kl0u commented on a diff in the pull request:

https://github.com/apache/flink/pull/5689#discussion_r174135885
  
--- Diff: 
flink-tests/src/test/java/org/apache/flink/test/example/client/JobRetrievalITCase.java
 ---
@@ -119,6 +121,11 @@ public void run() {
lock.release();
 
resumingThread.join();
+
+   Throwable exception = error.get();
+   if (exception != null) {
+   throw new AssertionError(exception);
+   }
--- End diff --

Is this any different from using `Assert.assertNull(error.get())`? Not that 
it makes any difference but just to know why you picked throwing the exception 
here. Personally I think that the assertNull seems more explicit when you read 
the code and it is less verbose.


---

[GitHub] flink pull request #5689: [FLINK-4569][tests] Respect exceptions thrown in t...

[zentol]

GitHub user zentol opened a pull request:

https://github.com/apache/flink/pull/5689

[FLINK-4569][tests] Respect exceptions thrown in thread in 
JobRetrievalITCase

## What is the purpose of the change

This PR ensures that exceptions that occur in the submitting thread 
actually fail the test. Also contains minor fixes, like setting a thread name 
(for debugging purposes) and releasing the lock in the `SemaphoreInvokable` to 
allow multiple runs in the IDE.

## Verifying the change

One can modify the test to throw an exception after connecting 
successfully, which will fail the test.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/zentol/flink 4569

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/5689.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #5689


commit 13c383d439550b1ebdc869981a7ffa19a6c8a151
Author: zentol 
Date:   2018-03-13T12:00:47Z

[FLINK-4569][tests] Respect exceptions thrown in thread in 
JobRetrievalITCase




---