Timeout parameter on @Test Annotation does not override Timeout Rule

[voomdoon]

I refer to https://github.com/junit-team/junit/wiki/Timeout-for-tests

If I use

@Rule
public Timeout globalTimeout = Timeout.seconds(10);

@Test(timeout=20000)
public void testWithTimeout() {
  ...
}

the test schould timeout afer 20s but it times out after 10s

[kcooney]

~~I don't see any way that we can fix this. It's possible that the timeout rule could look at the @Test attribute to override the timeout it applies, but the code that handles the timeout attribute doesn't know anything about what rules are applied.~~

Edit: We could set a ThreadLocal in FailOnTimeout.evalulate() to indicate that we are running inside of a FailOnTimeout statement, and therefore don't need to apply a new timeout. Then we could have Timeout.apply() look at the annotations on the Description to see if there is a timeout specified, and if so, use that instead. The only problem I see is finding a way to write a test for this that isn't flaky.

[baev]

@kcooney seems like the dirty hack. What about remove withPotentialTimeout from BlockJUnit4ClassRunner#methodBlock, and manage all timeouts logic in rules? We can access to annotations timeouts Description#getAnnotations(). So all we need - add some logic to withRules which will add Timeout if no rule present but some timeouts in annotations.

Also after this change test methods with timeout parameter will run in the same thread which runs the fixture's @Before and @After methods.

[kcooney]

@baev withPotentialTimeout is a non-final protected methods, so third-party runners can call it or override it. If they override it, they are expecting it to be called, so we would break code if we removed the call to withPotentialTimeout from BlockJUnit4ClassRunner#methodBlock.

[baev]

@kcooney withPotentialTimeout is deprecated at 01.07.09 by Saff with comment "Will be private soon: use Interceptors instead" (see e4c7fac8 ).

[kcooney]

@baev good point! We could make withPotentialTimeout private in JUnit 5, though it would break people:

• http://docs.spring.io/autorepo/docs/spring/3.1.x/javadoc-api/org/springframework/test/context/junit4/SpringJUnit4ClassRunner.html
• http://www.massapi.com/class/org/apache/openejb/junit/context/ContextWrapperStatement.html

Calling the @Before and @After methods use the same thread (and timeout) as the test could also break people, in a much more subtle way, so we'll have to decide if that's something we're willing to do.

[baev]

BTW we can return empty Statement instead FailOnTimeout.

Calling the @Before and @After methods use the same thread (and timeout) as the test could also break people, in a much more subtle way, so we'll have to decide if that's something we're willing to do.

Agree. Personally i would like to deprecate timeout parameter in @Test annotation and add new annotation @Timeout with parameters long timeout and TimeUnit unit... But it can cause lots of misunderstanding like "i annotate to my fixture method but it doesn't work"

[baev]

or we can use withPotentialTimeout only if there are no Timeout rules present and get timeout from annotation in Timeout otherwise

[kcooney]

The ThreadLocal solution seems less invasive. Besides the "yuck" factor, where there any concerns?

[baev]

There are problems with timeout architecture - two different ways to specify timeouts, with different behavior. Using ThreadLocal we add one more implicit (kind of magic?) relation. Maybe this solution will solve the current request but in feature it can cause some problems. I propose to make timeouts more clear and flexible.

[kcooney]

@baev having timeouts more clear and flexible would be nice. Can we think of a simple path to get there? Having the code that handles the timeout attribute of @Test have to look to see if there is a timeout ~~annotation~~ Rule isn't very simple.

One thing I like about the ThreadLocal approach is most of the logic for managing timeouts is in FailOnTimeout