Merge pull request junit-team#981 from kcooney/fail-on-timeout-builder

Create a Builder for FailOnTimeout.

src/main/java/org/junit/internal/runners/statements/FailOnTimeout.java

@@ -21,19 +21,94 @@ public class FailOnTimeout extends Statement {
     private final boolean lookForStuckThread;
     private volatile ThreadGroup threadGroup = null;
 
-    public FailOnTimeout(Statement originalStatement, long millis) {
-        this(originalStatement, millis, TimeUnit.MILLISECONDS);
+    /**
+     * Returns a new builder for building an instance.
+     */
+    public static Builder builder() {
+        return new Builder();
+    }
+
+    /**
+     * Creates an instance wrapping the given statement with the given timeout in milliseconds.
+     *
+     * @param statement the statement to wrap
+     * @param timeoutMillis the timeout in milliseconds
+     * @deprecated use {@link #builder()} instead.
+     */
+    @Deprecated
+    public FailOnTimeout(Statement statement, long timeoutMillis) {
+        this(builder().withTimeout(timeoutMillis, TimeUnit.MILLISECONDS), statement);
     }
 
-    public FailOnTimeout(Statement originalStatement, long timeout, TimeUnit unit) {
-        this(originalStatement, timeout, unit, false);
+    private FailOnTimeout(Builder builder, Statement statement) {
+        originalStatement = statement;
+        timeout = builder.timeout;
+        timeUnit = builder.unit;
+        lookForStuckThread = builder.lookForStuckThread;
     }
 
-    public FailOnTimeout(Statement originalStatement, long timeout, TimeUnit unit, boolean lookForStuckThread) {
-        this.originalStatement = originalStatement;
-        this.timeout = timeout;
-        timeUnit = unit;
-        this.lookForStuckThread = lookForStuckThread;
+    /**
+     * Builder for {@link FailOnTimeout}.
+     */
+    public static class Builder {
+        private boolean lookForStuckThread = false;
+        private long timeout = 0;
+        private TimeUnit unit = TimeUnit.SECONDS;
+
+        private Builder() {
+        }
+
+        /**
+         * Specifies the time to wait before timing out the test.
+         *
+         * <p>If this is not called, or is called with a {@code timeout} of
+         * {@code 0}, the returned {@code Statement} will wait forever for the
+         * test to complete, however the test will still launch from a separate
+         * thread. This can be useful for disabling timeouts in environments
+         * where they are dynamically set based on some property.
+         *
+         * @param timeout the maximum time to wait
+         * @param unit the time unit of the {@code timeout} argument
+         * @return {@code this} for method chaining.
+         */
+        public Builder withTimeout(long timeout, TimeUnit unit) {
+            if (timeout < 0) {
+                throw new IllegalArgumentException("timeout must be non-negative");
+            }
+            if (unit == null) {
+                throw new NullPointerException("TimeUnit cannot be null");
+            }
+            this.timeout = timeout;
+            this.unit = unit;
+            return this;
+        }
+
+        /**
+         * Specifies whether to look for a stuck thread.  If a timeout occurs and this
+         * feature is enabled, the test will look for a thread that appears to be stuck
+         * and dump its backtrace.  This feature is experimental.  Behavior may change
+         * after the 4.12 release in response to feedback.
+         *
+         * @param enable {@code true} to enable the feature
+         * @return {@code this} for method chaining.
+         */
+        public Builder withLookingForStuckThread(boolean enable) {
+            this.lookForStuckThread = enable;
+            return this;
+        }
+
+        /**
+         * Builds a {@link FailOnTimeout} instance using the values in this builder,
+         * wrapping the given statement.
+         *
+         * @param statement
+         */
+        public FailOnTimeout build(Statement statement) {
+            if (statement == null) {
+                throw new NullPointerException("statement cannot be null");
+            }
+            return new FailOnTimeout(this, statement);
+        }
     }
 
     @Override
@@ -86,8 +161,8 @@ private Exception createTimeoutException(Thread thread) {
                 new Exception ("Appears to be stuck in thread " +
                                stuckThread.getName());
             stuckThreadException.setStackTrace(getStackTrace(stuckThread));
-            return new MultipleFailureException    
-                (Arrays.<Throwable>asList(currThreadException, stuckThreadException));
+            return new MultipleFailureException(
+                Arrays.<Throwable>asList(currThreadException, stuckThreadException));
         } else {
             return currThreadException;
         }
@@ -117,12 +192,12 @@ private StackTraceElement[] getStackTrace(Thread thread) {
      * problem or if the thread cannot be determined.  The return value is never equal 
      * to {@code mainThread}.
      */
-    private Thread getStuckThread (Thread mainThread) {
-        if (threadGroup == null){
+    private Thread getStuckThread(Thread mainThread) {
+        if (threadGroup == null) {
             return null;
         }
         Thread[] threadsInGroup = getThreadArray(threadGroup);
-        if (threadsInGroup == null){
+        if (threadsInGroup == null) {
             return null;
         }
 
@@ -162,13 +237,16 @@ private Thread[] getThreadArray(ThreadGroup group) {
         while (true) {
             threads = new Thread[enumSize];
             enumCount = group.enumerate(threads);
-            if (enumCount < enumSize) break;
+            if (enumCount < enumSize) {
+                break;
+            }
             // if there are too many threads to fit into the array, enumerate's result
             // is >= the array's length; therefore we can't trust that it returned all
             // the threads.  Try again.
             enumSize += 100;
-            if (++loopCount >= 5) 
+            if (++loopCount >= 5) {
                 return null;
+            }
             // threads are proliferating too fast for us.  Bail before we get into 
             // trouble.
         }
@@ -185,8 +263,9 @@ private Thread[] getThreadArray(ThreadGroup group) {
     private Thread[] copyThreads(Thread[] threads, int count) {
         int length = Math.min(count, threads.length);
         Thread[] result = new Thread[length];
-        for (int i = 0; i < length; i++)
+        for (int i = 0; i < length; i++) {
             result[i] = threads[i];
+        }
         return result;
     }
 

src/main/java/org/junit/rules/Timeout.java

@@ -118,6 +118,17 @@ public Timeout lookingForStuckThread(boolean enable) {
     }
 
     public Statement apply(Statement base, Description description) {
-        return new FailOnTimeout(base, timeout, timeUnit, lookForStuckThread);
+        try {
+            return FailOnTimeout.builder()
+                .withTimeout(timeout, timeUnit)
+                .withLookingForStuckThread(lookForStuckThread)
+                .build(base);
+        } catch (final Exception e) {
+            return new Statement() {
+                @Override public void evaluate() throws Throwable {
+                    throw new RuntimeException("Invalid parameters for Timeout", e);
+                }
+            };
+        }
     }
 }

src/main/java/org/junit/runners/BlockJUnit4ClassRunner.java

@@ -5,6 +5,7 @@
 
 import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
 
 import org.junit.After;
 import org.junit.Before;
@@ -310,7 +311,12 @@ protected Statement possiblyExpectingExceptions(FrameworkMethod method,
     protected Statement withPotentialTimeout(FrameworkMethod method,
             Object test, Statement next) {
         long timeout = getTimeout(method.getAnnotation(Test.class));
-        return timeout > 0 ? new FailOnTimeout(next, timeout) : next;
+        if (timeout <= 0) {
+            return next;
+        }
+        return FailOnTimeout.builder()
+               .withTimeout(timeout, TimeUnit.MILLISECONDS)
+               .build(next);
     }
 
     /**

src/test/java/org/junit/tests/experimental/rules/TimeoutRuleTest.java

@@ -23,6 +23,7 @@
 import org.junit.rules.Timeout;
 import org.junit.runner.JUnitCore;
 import org.junit.runner.Result;
+import org.junit.runner.notification.Failure;
 
 public class TimeoutRuleTest {
     private static final ReentrantLock run1Lock = new ReentrantLock();
@@ -101,6 +102,16 @@ public static class HasGlobalTimeUnitTimeout extends AbstractTimeoutTest {
         @Rule
         public final TestRule globalTimeout = new Timeout(200, TimeUnit.MILLISECONDS);
     }
+
+    public static class HasNullTimeUnit {
+
+        @Rule
+        public final TestRule globalTimeout = new Timeout(200, null);
+
+        @Test
+        public void wouldPass() {
+        }
+    }
 
     @Before
     public void before() {
@@ -140,4 +151,15 @@ public void longTimeout() {
         assertThat(HasGlobalLongTimeout.logger.toString(), containsString("run5"));
         assertThat(HasGlobalLongTimeout.logger.toString(), containsString("run6"));
     }
+
+    @Test
+    public void nullTimeUnit() {
+        Result result = JUnitCore.runClasses(HasNullTimeUnit.class);
+        assertEquals(1, result.getFailureCount());
+        Failure failure = result.getFailures().get(0);
+        assertThat(failure.getException().getMessage(),
+                containsString("Invalid parameters for Timeout"));
+        Throwable cause = failure.getException().getCause();
+        assertThat(cause.getMessage(), containsString("TimeUnit cannot be null"));
+    }
 }