Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Resolves #1365 #1569

Closed
wants to merge 1 commit into from
Closed

Resolves #1365 #1569

wants to merge 1 commit into from

Conversation

npathai
Copy link
Contributor

@npathai npathai commented Oct 27, 2018

Resolves #1365

  • runLeaf method of ParentRunner clears the interrupt status flag after execution of each test
  • Added test case to verify interrupt status

kcooney
kcooney reviewed Nov 7, 2018
View reviewed changes
src/main/java/org/junit/runners/ParentRunner.java Outdated
@@ -346,7 +345,7 @@ protected final void runLeaf(Statement statement, Description description,
EachTestNotifier eachNotifier = new EachTestNotifier(notifier, description);
eachNotifier.fireTestStarted();
try {
statement.evaluate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if instead of updating runLeaf() we should update methodBlock() to return a Statement that calls Thread.interrupted() at the end of the statement chain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should almost certainly modify classBlock() to return a Statement that calls Thread.interrupted() at the end of the statement chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kcooney All right. I am looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kcooney Updated methodBlock() to return a statement that clears interrupt status and reverted runLeaf. I am not sure how updating classBlock() will help?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npathai I assume that we would want to handle the case where the interrupt bit was set in a class rule or @AfterClass method. Those are handled by classBlock()

@npathai
Copy link
Contributor Author

npathai commented Dec 4, 2018

@kcooney I have incorporated the review comments and cleared interrupt status flag from methodBlock() and classBlock().

kcooney
kcooney approved these changes Dec 4, 2018
View reviewed changes
Copy link
Member

@kcooney kcooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

src/main/java/org/junit/runners/ParentRunner.java Outdated
/**
* @return a {@link Statement}: clears interrupt status of current thread after execution of statement
*/
protected Statement withInterruptIsolation(final Statement statement) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally would prefer this is be final

@npathai
Copy link
Contributor Author

npathai commented Dec 5, 2018

@kcooney Yes I agree. Made the method final. Should I squash the commits?

@kcooney
Copy link
Member

kcooney commented Dec 6, 2018

@npathai we usually do a squash and merge unless the author asks us to keep the commits separate. If you think these changes should be broken up a certain way on the main branch, please squash them.

@npathai
Copy link
Contributor Author

npathai commented Dec 10, 2018

@kcooney I have squashed the commits. But Travis build for openjdk-6 failed, I am not sure why.

@kcooney
Copy link
Member

kcooney commented Dec 10, 2018

Here are the logs:

$ ~/bin/install-jdk.sh --target "/home/travis/openjdk6" --workspace "/home/travis/.cache/install-jdk" --feature "6" --license "GPL"
install-jdk.sh 2018-10-17
Expected feature release number in range of 9 to 12, but got: 6
The command "~/bin/install-jdk.sh --target "/home/travis/openjdk6" --workspace "/home/travis/.cache/install-jdk" --feature "6" --license "GPL"" failed and exited with 3 during .

kcooney
kcooney requested changes Dec 10, 2018
View reviewed changes
Copy link
Member

@kcooney kcooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, noticed some minor problems.

We should file a separate issue about the broken openjdk6 build.

src/main/java/org/junit/runners/ParentRunner.java Outdated Show resolved Hide resolved
src/test/java/org/junit/tests/running/classes/ThreadsTest.java Outdated Show resolved Hide resolved
@npathai
fix issue junit-team#1365, cleared thread interrupt status flag from …
eec91b6 
…classBlock() and methodBlock(). The flag is cleared after each test case

completes and after AfterClass/ClassRule are executed
@npathai
Copy link
Contributor Author

npathai commented Dec 11, 2018 •
edited
Loading

@kcooney All done. Raised issue for failing openjdk-6 build as well. Once merged I will update the release notes.

@npathai npathai mentioned this pull request Dec 11, 2018
Closed
@marcphilipp
Copy link
Member

@kcooney This should make it into 4.13, right?

@kcooney
Copy link
Member

kcooney commented Dec 13, 2018

@marcphilipp your call.

@kcooney kcooney mentioned this pull request Dec 16, 2018
Closed
@marcphilipp marcphilipp added this to the 4.13-beta-2 milestone Dec 16, 2018
@marcphilipp
Copy link
Member

@npathai Please rebase your branch onto master where I've fixed the openjdk6 build in the meantime.

@ijuma
Copy link

ijuma commented Jan 5, 2019

It would be great to get this in so the long awaited 4.13 can be released. :)

@marcphilipp marcphilipp mentioned this pull request Jan 12, 2019
Merged
@marcphilipp
Copy link
Member

Since this PR seems to have stalled, I've resubmitted the changes in #1586.

@marcphilipp marcphilipp removed this from the 4.13-beta-2 milestone Jan 12, 2019
@npathai
Copy link
Contributor Author

npathai commented Jan 25, 2019

@marcphilipp Apologies, I was away for some time so couldn't reply.

@marcphilipp
Copy link
Member

@npathai No worries! I picked up your work and finished it in #1586.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kcooney kcooney kcooney approved these changes

@marcphilipp marcphilipp marcphilipp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@npathai @kcooney @marcphilipp @ijuma