Change the Timeout rule to never timeout the test if zero is passed in. Fixes #875

[madrob]

No description provided.

[busbey]

Looks good, fixes my test case.

It does change a bit of behavior from 4.7-4.11. In those, a negative timeout would cause an error. With this change, negative will be interpreted as "no timeout." Since that's the same thing the Test annotation's timeout does, it seems fine. Worth noting though.

Is it worth adding a note in the documentation for Test and Timeout that a non-positive timeout means to wait forever?

[kcooney]

Thanks for the fix.

Note with this change, if timeout is zero, the test will run in the same thread. In 4.7 it still ran in a different thread.

I think we should instead treat zero as if they passed in MAX_INT

[kcooney]

Also, could you put in a more useful description? See http://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message

Thanks!

[madrob]

Would that be MAX_INT seconds or millis?

[busbey]

We could instead update FailOnTimeout's getResult:

    private Throwable getResult(FutureTask<Throwable> task, Thread thread) {
        try {
          if (fTimeout > 0) {
            return task.get(fTimeout, fTimeUnit);
          } else {
            return task.get();
          }
        } catch (InterruptedException e) {
            return e; // caller will re-throw; no need to call Thread.interrupt()
        } catch (ExecutionException e) {
            // test failed; have caller re-throw the exception thrown by the test
            return e.getCause();
        } catch (TimeoutException e) {
            return createTimeoutException(thread);
        }
    }

That would get you execution in a different thread and behavior consistent with the Test annotation.

[madrob]

Updated. I had not considered Sean's suggestion, but can do that instead if it is deemed preferable.

[kcooney]

@madrob I think Sean's suggestion was much better than mine; his works regardless of the units passed in. My only concern with fixing this FailOnTimeout is that it might cause a change in behavior in the timeout attribute of @Test compared with 4.7. I haven't looked at the 4.7 behavior; could someone on this thread do that?

[madrob]

@kcooney In 4.7 @Test would happily wait forever if the timeout attribute was not set. The default value returned by the timeout() method on the annotation is 0, so every test without a set timeout verifies this behavior. That part is pretty well covered against regressions already :)

Was there a different concern you had?

[kcooney]

@madrob no other concerns. Let's make the fix that @busbey suggested.

[madrob]

Implemented in 14ecb4b and already pushed.

[busbey]

lgtm

[marcphilipp]

+1

[kcooney]

nit: this should be indented one more space (we use a four-space indent in JUnit)

[kcooney]

LGTM with one very minor style comment. Thanks!

[madrob]

Indent fixed in c52397d. Somehow I managed to accidentally sneak a tab in there earlier.

[kcooney]

Thanks!

[stefanbirkner]

@madrob Could you please make a note at https://github.com/junit-team/junit/wiki/4.12-release-notes

[madrob]

Done, how's that look?

[Stephan202]

The comments above discuss subtle differences with earlier versions of JUnit. Not sure which of those are applicable to the code that was eventually merged (didn't check), but I think that the current release notes entry is not particularly useful:

In JUnit 4.7 - 4.11 setting up a Timeout rule with value 0 would result in no timeout, consistent with @Test(timeout = 0). The switch in 4.12 to using FutureTask for the implementation would instead cause a timeout immediately, and this change brings the behavior back in line with previous versions.

A regular user upgrading from JUnit 4.11 to 4.12 will conclude that "nothing changed for me" and wonder why such implementation details are mentioned in the release notes. (@madrob effectively just caught a regression and fixed it in time.) Can we instead document the ways in which timeouts are now handled differently, if at all, and otherwise omit this from the release notes?

[kcooney]

@Stephan202 good point. I removed the text about this pull from the release notes.