Mark ThreadGroups created by FailOnTimeout as daemon groups #1687
Conversation
src/main/java/org/junit/internal/runners/statements/FailOnTimeout.java
Outdated
Show resolved
Hide resolved
…out.java Clarify comment in FailOnTimeout. Co-authored-by: Stefan Birkner <github@stefan-birkner.de>
|
FYI: I updated the test to verify that the |
|
So apparently marking the |
|
Which versions did you try? |
|
It was a race condition. The test needed to join on the inner thread before checking the Lots of changes to squash, but leaving as-is for now so you can all see the changes. |
|
@kcooney Can you please have a look at #1690. I changed the test that checks that the thread group is destroyed. IMO it is now checking the behavior that we are looking for. What do you think? It you are ok with the new test then your PR can be reduced to the change in FailOnTimeout and does not need to change the test. |
|
@stefanbirkner the test in this pull now verifies that the thread group is destroyed. It has a few unnecessary assertions (I was trying to determine why it passed on my machine but not in the CI build). I would be happy to remove the now-unnecessary extra assertions (Edit: I pushed a commit to remove the unnecessary assertions). Even so, the test in this pull seems a bit more straight-forward than the one in yours. WDYT? |
|
I tend to like your approach more. If I change my opinion I can still create another PR. |
|
I changed the explanation in #1690 and removed changing the test. |
| } catch (IllegalThreadStateException e) { | ||
| // If a thread from the group is still alive, the ThreadGroup cannot be destroyed. | ||
| // Swallow the exception to keep the same behavior prior to this change. | ||
| threadGroup.setDaemon(true); |
There was a problem hiding this comment.
There should be a comment here pointing to this pull request and explaining why they should be daemon.
There was a problem hiding this comment.
#1691 adds an explanation
Fixes #1652