Global Timeout using Junit Rule

[kishoretak]

Hi, Currently, this library is overriding timeout annotation to have a global timeout. Junit has deprecated this option and introduced Timeout Rule, which also considers the time spent in the before and after method along with the unit test's time. (timeout annotation doesn't consider before and after method time). I've made this change in the library via overriding getTestRules method of BlockJUnit4ClassRunner and adding timeout rule to existing rule list. Please share your thought on same.

Thanks

[sbabcoc]

I considered using the Timeout rule to implement the global timeout feature, but this mechanism doesn't provide the necessary level of control. The primary issue is that the Timeout rule sets the maximum duration of the entire atomic test, including @Before and @After methods. That behavior means that tests can time out because of delays in setup or teardown, which aren't actually part of the test.

If you look here, you'll see that I added information to this JUnit 4 Wiki page after the information about the Timeout rule. As far as I know, the timeout element of the @Test annotation of JUnit 4 has never been deprecated.

[kishoretak]

Hi @sbabcoc Thanks for your prompt response!!

• The primary issue is that the Timeout rule sets the maximum duration of the entire atomic test, including @Before and @After methods.

This is exactly what we expect as we want to enforce the overall timeout, I agree that it might not be correct behavior for your use-case. I wanted to know your thoughts on my approach for our use-case.

• As far as I know, the timeout element of the @Test annotation of JUnit 4 has never been deprecated.

Yes, the timeout element of the @Test annotation of JUnit 4 is not deprecated. I was referring to this - https://github.com/junit-team/junit4/blob/6d0fad48ce3a05b32d903d2016c24d276b6e1eb8/src/main/java/org/junit/runners/BlockJUnit4ClassRunner.java#L356

[sbabcoc]

Weird that they deprecated a protected method that almost no one will even notice, but failed to deprecate the timeout element of the @Test annotation that relates to it. It's easy to apply a global timeout through a proxy of the @Test annotation, but auto-applied test rules are another matter entirely. There's no way to exempt specific classes and methods from the effects of the Timeout rule. I'd also need to dig into the guts of JUnit 4 to figure out how a framework extension like JUnit Foundation would go about adding a test rules automatically.

[kishoretak]

I'm overriding the getTestRules method of BlockJUnit4ClassRunner and appending a new timeout rule to existing rules. https://github.com/junit-team/junit4/blob/master/src/main/java/org/junit/runners/BlockJUnit4ClassRunner.java#L434

package com.nordstrom.automation.junit;

import net.bytebuddy.implementation.bind.annotation.Argument;
import net.bytebuddy.implementation.bind.annotation.RuntimeType;
import net.bytebuddy.implementation.bind.annotation.SuperCall;
import net.bytebuddy.implementation.bind.annotation.This;
import org.junit.rules.TestRule;
import org.junit.rules.Timeout;

import java.util.List;
import java.util.Objects;
import java.util.concurrent.Callable;

public class GetTestRules {
    /**
     * Interceptor for the {@link org.junit.runners.BlockJUnit4ClassRunner#getTestRules(Object)}  getTestRules} method.
     *
     * @param runner target {@link org.junit.runners.BlockJUnit4ClassRunner BlockJUnit4ClassRunner} object
     * @param proxy  callable proxy for the intercepted method
     * @param target the test case instance
     * @return {@code anything} - JUnit test class instance
     * @throws Exception {@code anything} (exception thrown by the intercepted method)
     */
    @RuntimeType
    public static List<TestRule> intercept(@This final Object runner, @SuperCall final Callable<?> proxy, @Argument(0) final Object target) throws Exception {
        List<TestRule> testRules = (List<TestRule>) LifecycleHooks.callProxy(proxy);
        Long timeout = null;
        try {
            timeout = Long.getLong("junitTestTimeoutInMilliSeconds");
            System.out.println("junitTestTimeoutInMilliSeconds = " + timeout);
        } catch (Exception ignored) {
        }
        if (!Objects.isNull(timeout)) {
            testRules.add(Timeout.millis(timeout));
        }
        return testRules;
    }
}

[sbabcoc]

Good work! This provides a great starting point. Here are some characteristics the implementation should probably have:

1. Does the test object already have a Timeout rule attached to it?
   • YES => If the existing Timeout rule specifies a longer interval than the default test timeout, leave it alone.
   • NO => Replace the existing Timeout rule with a new one built with the default timeout.
2. Does the @Test annotation of the method for which the target test object was created have the timeout element?
   • YES => Proxy the @Test annotation with one that removes the timeout element.

To maximize configurability, you could provide the ability to specify a Timeout subclass that should be used instead of the default Timeout class.

NOTE: The handling of scenarios where both the Timeout rule and the timeout element of the @Test annotation are specified should be carefully considered. Simply removing the timeout element doesn't consider situations where the annotation timeout is longer than the rule timeout. I haven't been able to ponder this long enough to formulate a strategy for handling this situation.

[kishoretak]

Thanks @sbabcoc Regarding point 2. Does the @Test annotation of the method for which the target test object was created have the timeout element? YES => Proxy the @Test annotation with one that removes the timeout element.

You mean if a test already has timeout arg set in @Test annotation then remove it?

Few more point to consider -

1. We'll be using both timeout annotation and timeout rule for global timeout?
2. YES - Do we want to use the same global timeout parameter (junit.timeout.test) to be used in both timeout annotation and timeout rule or two separate config params?
3. NO - we'll remove timeout annotation and use only timeout rule going forward for the global timeout.

This will derive the overall strategy for the final (impacting) timeout which a test going to consider.

[sbabcoc]

Both timeout features can be active concurrently. The timeout element of the @Test annotation limits the duration of the test method in isolation; the Timeout rule limits the duration of the atomic test, which includes all setup and cleanup methods. It's very easy to specify conflicting values if management is provided for both timeout features, but I guess this falls under the heading of caveat emptor.

The current management feature enables global control of test method timeout via the TEST_TIMEOUT configuration option. I propose adding a new TIMEOUT_RULE option to provide management of your Timeout rule feature.

When applying the TEST_TIMEOUT option:

• Does the @Test annotation specify shorter timeout interval?
  • YES => proxy the @Test annotation, assigning the global timeout interval.
  • NO => leave it alone.

When applying the TIMEOUT_RULE option:

• Does the @Test annotation specify the timeout element with longer interval?
  • YES => Adopt the value of the timeout element as the Timeout rule interval.
• Does the test object already have a Timeout rule attached to it?
  • YES, with same or longer interval => Leave the existing Timeout rule alone.
  • YES, with shorter interval => Replace the existing Timeout rule with a new one built with the default timeout.
  • NO => Add Timeout rule built with default timeout.

[sbabcoc]

I've added management of a global Timeout rule. I'll be adding the ability to disable timeouts entirely.

[kishoretak]

Thanks @sbabcoc, actually I wanted to merge this change from my end as part of my contribution to open-source :)

[sbabcoc]

Sorry, @kishoretak! Your feature suggestion was excellent, and the code you contributed was a great starting point. I ended up creating my own implementation of this feature, but I should have handled this set of revisions so that you got the contributor credit you deserve. I just released a new version that consolidates the two flavors of JUnit timeout control much better. The README documentation of timeout management could probably use some attention, and a pull request for this content would give you contributor credit.

[kishoretak]

Thanks, @sbabcoc! I've identified one use-case which is kind of a feature request, currently, the library doesn't support timeout if the test is using PowerMockRunner (which is pretty common). I've added this functionality to my repo and want to check whether it's a correct use-case for this library. Let me know your thoughts on the same.

[sbabcoc]

@kishoretak Can you share a sample project that reproduces the issue?

It seems likely that the PowerMock runner is pulling JUnit classes into the class loader before JUnit-Foundation can install hooks in them. PowerMock itself declares dependencies on several byte code generation libraries, and it's probably mucking about with some of the same core JUnit classes as JUnit-Foundation does.

[kishoretak]

@sbabcoc It should be reproducible for any test class defining @RunWith(PowerMockRunner.class). As the Powermock runner doesn't extend the ParentRunner, instead it extends Runner abstract class and we'are overriding the behavior of ParentRunner and it's subclasses.

Also, there is an issue with using timeout parameter in @Test Annotation with PowerMockRunner - https://github.com/powermock/powermock/issues/817

[sbabcoc]

Any reference to PowerMockRunner is sufficient to disable the intercepts being installed by JUnit Foundation, because the PowerMock library is loading JUnit core classes before we can add our hooks. I don't what we could do to fix this.

[sbabcoc]

It's possible that I could create a wrapper for custom runners that would postpone loading the JUnit classes until after we installed our hooks. I'll throw something together to see if this is a successful technique.

[kishoretak]

Yeah, my approach is similar to above.

 .type(nameContains("PowerMockJUnit47MethodRunner"))
                .transform((builder, type, classloader, module) -> builder
                       .method(named("createStatement")).intercept(MethodDelegation.to(createStatement))
                        .implement(Hooked.class))

But as the junit-foundation is only meant to be helper for junit and not for powermock, So I'm not sure whether it should hanlde powermock cases or not.

[sbabcoc]

Yes, JUnit Foundation is a general-purpose solution for JUnit 4.12. PowerMock is using deprecated JUnit interfaces, which JUnit Foundation doesn't currently support. This is the real reason the installed hooks are never invoked. The wrapper approach I was exploring is insufficient to add support for PowerMock.

The interfaces being used by PowerMock have been deprecated since JUnit 4.5. The ClassRoadie class, which PowerMock uses in its runner implementations, includes this comment:

Included for backwards compatibility with JUnit 4.4. Will be removed in the next major release. Please use BlockJUnit4ClassRunner in place of JUnit4ClassRunner.

JUnit Foundation extends the current JUnit framework, and adding support for the deprecated interfaces would require a lot of work. Rules weren't added until JUnit 4.7, which is why PowerMock support for them is so uneven (they just hacked in as best they could).

You may want to explore switching over to JMockit instead. This library is under active development, with support for JUnit 4, JUnit 5, and TestNG.

[kishoretak]

@sbabcoc, I totally agree with you on PowerMock using deprecated JUnit interfaces (ClassRoadie and MethodRoadie). I'll explore JMockit for our use-case, thanks for your suggestion.