-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
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 <[email protected]>
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. |
Fixes #1652