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.

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

Change in behavior for Timeout Rule in 4.12 #875

Closed
busbey opened this issue Apr 14, 2014 · 3 comments
Closed

Change in behavior for Timeout Rule in 4.12 #875

busbey opened this issue Apr 14, 2014 · 3 comments
Milestone

Comments

@busbey
Copy link

busbey commented Apr 14, 2014

In versions 4.7 through 4.11 setting up a Timeout Rule with a value of 0 would result in no timeout. e.g.

public class MyTest {

  @Rule
  Timeout probablyTimeout = new Timeout(NumberUtils.toInt(System.getProperty("test.timeout")));

  @Test
  public void checkSomethingThatTakesAwhile() {
    // ...
  }
}

In this case, commons-lang's toInt will default to 0 for unparseable or empty strings, which Timeout will then interpret as "never timeout". This behavior was a result of FailOnTimeout relying on Thread.join(timeout), which has a contract of 0 meaning "wait forever". This makes it behave the same as the Test annotation's timeout parameter.

In 4.12 FailOnTimeout switched to using FutureTask.get(timeout). That method treats non-positive timeouts to mean "timeout right now".

So when updating to JUnit 4.12, the above example will suddenly start failing if it relies on the 0 timeout behavior to allow an external system to handle test timeout in some cases, e.g. while using Jenkins for CI.

This puts it at odds with both the previous behavior and the method timeout annotation.

madrob pushed a commit to madrob/junit that referenced this issue Apr 14, 2014
@kcooney
Copy link
Member

kcooney commented Apr 14, 2014

ould you send us a pull request that fixes this? I would like to keep using FutureTask, so we could just add a speacial case that treats zero as MAX_INT

madrob pushed a commit to madrob/junit that referenced this issue Apr 14, 2014
Fixes junit-team#875

In versions 4.7 through 4.11 setting up a Timeout Rule with a value of
0 would result in no timeout. In 4.12 FailOnTimeout switched to using
FutureTask.get(timeout). That method treats non-positive timeouts to
mean "timeout right now".

This fix checks that the timeout value is positive before using it,
and otherwise will wait indefinitely.
madrob pushed a commit to madrob/junit that referenced this issue Apr 14, 2014
Fixes junit-team#875

In versions 4.7 through 4.11 setting up a Timeout @rule with a value of
0 would result in no timeout. In 4.12 FailOnTimeout switched to using
FutureTask.get(timeout). That method treats non-positive timeouts to
mean "timeout right now".

This fix checks that the timeout value is positive before using it,
and otherwise will wait indefinitely.
kcooney added a commit that referenced this issue Apr 16, 2014
Change the Timeout rule to never timeout the test if zero is passed in. Fixes #875
fshepherd pushed a commit to fshepherd/junit that referenced this issue Apr 22, 2014
Fixes junit-team#875

In versions 4.7 through 4.11 setting up a Timeout @rule with a value of
0 would result in no timeout. In 4.12 FailOnTimeout switched to using
FutureTask.get(timeout). That method treats non-positive timeouts to
mean "timeout right now".

This fix checks that the timeout value is positive before using it,
and otherwise will wait indefinitely.
fshepherd pushed a commit to fshepherd/junit that referenced this issue Apr 22, 2014
Fixes junit-team#875

In versions 4.7 through 4.11 setting up a Timeout @rule with a value of
0 would result in no timeout. In 4.12 FailOnTimeout switched to using
FutureTask.get(timeout). That method treats non-positive timeouts to
mean "timeout right now".

This fix checks that the timeout value is positive before using it,
and otherwise will wait indefinitely.
@mcqueenorama
Copy link

Is there a workaround for this? I see no releases that include this patch.

@kcooney
Copy link
Member

kcooney commented Aug 20, 2016

This was fixed in c52397d. Apparently the fix didn't get into the 4.13 release notes. I'll reopen the bug as a reminder.

@kcooney kcooney reopened this Aug 20, 2016
@kcooney kcooney added the 4.13 label Aug 20, 2016
@kcooney kcooney closed this as completed Jan 22, 2017
@kcooney kcooney modified the milestone: 4.13 Aug 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants