From 2db63942882d91020b46d7333285e5c94f1d1e52 Mon Sep 17 00:00:00 2001 From: Stefan Birkner Date: Sat, 2 Jan 2021 16:54:40 +0100 Subject: [PATCH] Tidy up FailOnTimeoutTest Improve readability and reusability. When I started to read the test it took me some time to understand it. The reason was the configurable statement TestStatement and the evaluateWith... methods. The refactored test uses fixed Statements when possible and uses smaller methods for wrapping FailOnTimeout with a ThrowingRunnable and for wrapping an arbitrary statement with a FailOnTimeout. It also calls FailOnTimeout#evaluate directly when possible. --- .../runners/statements/FailOnTimeoutTest.java | 267 +++++++++--------- 1 file changed, 135 insertions(+), 132 deletions(-) diff --git a/src/test/java/org/junit/internal/runners/statements/FailOnTimeoutTest.java b/src/test/java/org/junit/internal/runners/statements/FailOnTimeoutTest.java index f1e7f79a886b..20ca9cd55630 100644 --- a/src/test/java/org/junit/internal/runners/statements/FailOnTimeoutTest.java +++ b/src/test/java/org/junit/internal/runners/statements/FailOnTimeoutTest.java @@ -8,23 +8,24 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import static org.junit.Assume.assumeFalse; import static org.junit.Assume.assumeTrue; import java.util.Arrays; -import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; import org.junit.function.ThrowingRunnable; -import org.junit.internal.runners.statements.Fail; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; import org.junit.runners.model.Statement; import org.junit.runners.model.TestTimedOutException; @@ -34,176 +35,129 @@ */ @RunWith(Parameterized.class) public class FailOnTimeoutTest { - private static final long TIMEOUT = 100; - private static final long DURATION_THAT_EXCEEDS_TIMEOUT = 60 * 60 * 1000; //1 hour - private final TestStatement statement = new TestStatement(); - - private final boolean lookingForStuckThread; - private final FailOnTimeout failOnTimeout; - - @Parameterized.Parameters(name = "lookingForStuckThread = {0}") + @Parameters(name = "lookingForStuckThread = {0}") public static Iterable getParameters() { return Arrays.asList(Boolean.TRUE, Boolean.FALSE); } - public FailOnTimeoutTest(Boolean lookingForStuckThread) { - this.lookingForStuckThread = lookingForStuckThread; - this.failOnTimeout = builder().withTimeout(TIMEOUT, MILLISECONDS).build(statement); - } + @Parameter + public boolean lookingForStuckThread; + + @Test + public void noExceptionIsThrownWhenWrappedStatementFinishesBeforeTimeoutWithoutThrowingException() + throws Throwable { + FailOnTimeout failOnTimeout = failAfter50Ms(new FastStatement()); - private FailOnTimeout.Builder builder() { - return FailOnTimeout.builder().withLookingForStuckThread(lookingForStuckThread); + failOnTimeout.evaluate(); + + // test is successful when no exception is thrown } @Test public void throwsTestTimedOutException() { assertThrows( TestTimedOutException.class, - evaluateWithWaitDuration(DURATION_THAT_EXCEEDS_TIMEOUT)); + run(failAfter50Ms(new InfiniteLoop()))); } @Test public void throwExceptionWithNiceMessageOnTimeout() { - TestTimedOutException e = assertThrows( - TestTimedOutException.class, - evaluateWithWaitDuration(DURATION_THAT_EXCEEDS_TIMEOUT)); - assertEquals("test timed out after 100 milliseconds", e.getMessage()); + Exception e = assertThrows( + Exception.class, + run(failAfter50Ms(new InfiniteLoop()))); + assertEquals("test timed out after 50 milliseconds", e.getMessage()); } @Test public void sendUpExceptionThrownByStatement() { - RuntimeException exception = new RuntimeException(); - RuntimeException e = assertThrows( - RuntimeException.class, - evaluateWithException(exception)); + Exception exception = new RuntimeException(); + Exception e = assertThrows( + Exception.class, + run(failAfter50Ms(new Fail(exception)))); assertSame(exception, e); } @Test public void throwExceptionIfTheSecondCallToEvaluateNeedsTooMuchTime() throws Throwable { - evaluateWithWaitDuration(0).run(); + DelegateStatement statement = new DelegateStatement(); + FailOnTimeout failOnTimeout = failAfter50Ms(statement); + + statement.delegate = new FastStatement(); + failOnTimeout.evaluate(); + + statement.delegate = new InfiniteLoop(); assertThrows( TestTimedOutException.class, - evaluateWithWaitDuration(DURATION_THAT_EXCEEDS_TIMEOUT)); + run(failOnTimeout)); } @Test public void throwTimeoutExceptionOnSecondCallAlthoughFirstCallThrowsException() { - try { - evaluateWithException(new RuntimeException()).run(); - } catch (Throwable expected) { - } + DelegateStatement statement = new DelegateStatement(); + FailOnTimeout failOnTimeout = failAfter50Ms(statement); - TestTimedOutException e = assertThrows( + statement.delegate = new Fail(new AssertionError("first execution failed")); + assertThrows( + AssertionError.class, + run(failOnTimeout) + ); + + statement.delegate = new InfiniteLoop(); + assertThrows( TestTimedOutException.class, - evaluateWithWaitDuration(DURATION_THAT_EXCEEDS_TIMEOUT)); - assertEquals("test timed out after 100 milliseconds", e.getMessage()); + run(failOnTimeout)); } @Test public void throwsExceptionWithTimeoutValueAndTimeUnitSet() { TestTimedOutException e = assertThrows( TestTimedOutException.class, - evaluateWithWaitDuration(DURATION_THAT_EXCEEDS_TIMEOUT)); - assertEquals(TIMEOUT, e.getTimeout()); - assertEquals(TimeUnit.MILLISECONDS, e.getTimeUnit()); - } - - private ThrowingRunnable evaluateWithDelegate(final Statement delegate) { - return new ThrowingRunnable() { - public void run() throws Throwable { - statement.nextStatement = delegate; - statement.waitDuration = 0; - failOnTimeout.evaluate(); - } - }; - } - - private ThrowingRunnable evaluateWithException(Exception exception) { - return evaluateWithDelegate(new Fail(exception)); - } - - private ThrowingRunnable evaluateWithWaitDuration(final long waitDuration) { - return new ThrowingRunnable() { - public void run() throws Throwable { - statement.nextStatement = null; - statement.waitDuration = waitDuration; - failOnTimeout.evaluate(); - } - }; - } - - private static final class TestStatement extends Statement { - long waitDuration; - - Statement nextStatement; - - @Override - public void evaluate() throws Throwable { - sleep(waitDuration); - if (nextStatement != null) { - nextStatement.evaluate(); - } - } + run(failAfter50Ms(new InfiniteLoop()))); + assertEquals(50, e.getTimeout()); + assertEquals(MILLISECONDS, e.getTimeUnit()); } @Test public void stopEndlessStatement() throws Throwable { - InfiniteLoopStatement infiniteLoop = new InfiniteLoopStatement(); - FailOnTimeout infiniteLoopTimeout = builder().withTimeout(TIMEOUT, MILLISECONDS).build(infiniteLoop); - try { - infiniteLoopTimeout.evaluate(); - } catch (Exception timeoutException) { - sleep(20); // time to interrupt the thread - int firstCount = InfiniteLoopStatement.COUNT; - sleep(20); // time to increment the count - assertTrue("Thread has not been stopped.", - firstCount == InfiniteLoopStatement.COUNT); - } - } - - private static final class InfiniteLoopStatement extends Statement { - private static int COUNT = 0; - - @Override - public void evaluate() throws Throwable { - while (true) { - sleep(10); // sleep in order to enable interrupting thread - ++COUNT; - } - } + InfiniteLoop infiniteLoop = new InfiniteLoop(); + assertThrows( + TestTimedOutException.class, + run(failAfter50Ms(infiniteLoop))); + + sleep(20); // time to interrupt the thread + infiniteLoop.stillExecuting.set(false); + sleep(20); // time to increment the count + assertFalse( + "Thread has not been stopped.", + infiniteLoop.stillExecuting.get()); } @Test - public void stackTraceContainsRealCauseOfTimeout() throws Throwable { - StuckStatement stuck = new StuckStatement(); - FailOnTimeout stuckTimeout = builder().withTimeout(TIMEOUT, MILLISECONDS).build(stuck); - try { - stuckTimeout.evaluate(); - // We must not get here, we expect a timeout exception - fail("Expected timeout exception"); - } catch (Exception timeoutException) { - StackTraceElement[] stackTrace = timeoutException.getStackTrace(); - boolean stackTraceContainsTheRealCauseOfTheTimeout = false; - boolean stackTraceContainsOtherThanTheRealCauseOfTheTimeout = false; - for (StackTraceElement element : stackTrace) { - String methodName = element.getMethodName(); - if ("theRealCauseOfTheTimeout".equals(methodName)) { - stackTraceContainsTheRealCauseOfTheTimeout = true; - } - if ("notTheRealCauseOfTheTimeout".equals(methodName)) { - stackTraceContainsOtherThanTheRealCauseOfTheTimeout = true; - } + public void stackTraceContainsRealCauseOfTimeout() { + TestTimedOutException timedOutException = assertThrows( + TestTimedOutException.class, + run(failAfter50Ms(new StuckStatement()))); + + StackTraceElement[] stackTrace = timedOutException.getStackTrace(); + boolean stackTraceContainsTheRealCauseOfTheTimeout = false; + boolean stackTraceContainsOtherThanTheRealCauseOfTheTimeout = false; + for (StackTraceElement element : stackTrace) { + String methodName = element.getMethodName(); + if ("theRealCauseOfTheTimeout".equals(methodName)) { + stackTraceContainsTheRealCauseOfTheTimeout = true; + } + if ("notTheRealCauseOfTheTimeout".equals(methodName)) { + stackTraceContainsOtherThanTheRealCauseOfTheTimeout = true; } - assertTrue( - "Stack trace does not contain the real cause of the timeout", - stackTraceContainsTheRealCauseOfTheTimeout); - assertFalse( - "Stack trace contains other than the real cause of the timeout, which can be very misleading", - stackTraceContainsOtherThanTheRealCauseOfTheTimeout); } + assertTrue( + "Stack trace does not contain the real cause of the timeout", + stackTraceContainsTheRealCauseOfTheTimeout); + assertFalse( + "Stack trace contains other than the real cause of the timeout, which can be very misleading", + stackTraceContainsOtherThanTheRealCauseOfTheTimeout); } private static final class StuckStatement extends Statement { @@ -238,36 +192,85 @@ public void lookingForStuckThread_threadGroupNotLeaked() throws Throwable { final AtomicReference innerThreadGroup = new AtomicReference(); final AtomicReference innerThread = new AtomicReference(); final ThreadGroup outerThreadGroup = currentThread().getThreadGroup(); - ThrowingRunnable runnable = evaluateWithDelegate(new Statement() { + FailOnTimeout failOnTimeout = failAfter50Ms(new Statement() { @Override public void evaluate() { innerThread.set(currentThread()); ThreadGroup group = currentThread().getThreadGroup(); - assertNotSame("inner thread should use a different thread group", outerThreadGroup, group); + assertNotSame("inner thread should use a different thread group", + outerThreadGroup, group); innerThreadGroup.set(group); - assertTrue("the 'FailOnTimeoutGroup' thread group should be a daemon thread group", group.isDaemon()); + assertTrue("the 'FailOnTimeoutGroup' thread group should be a daemon thread group", + group.isDaemon()); } }); - runnable.run(); + failOnTimeout.evaluate(); - assertTrue("the Statement was never run", innerThread.get() != null); + assertNotNull("the Statement was never run", innerThread.get()); innerThread.get().join(); - assertTrue("the 'FailOnTimeoutGroup' thread group should be destroyed after running the test", innerThreadGroup.get().isDestroyed()); + assertTrue("the 'FailOnTimeoutGroup' thread group should be destroyed after running the test", + innerThreadGroup.get().isDestroyed()); } @Test public void notLookingForStuckThread_usesSameThreadGroup() throws Throwable { assumeFalse(lookingForStuckThread); + final AtomicBoolean statementWasExecuted = new AtomicBoolean(); final ThreadGroup outerThreadGroup = currentThread().getThreadGroup(); - ThrowingRunnable runnable = evaluateWithDelegate(new Statement() { + FailOnTimeout failOnTimeout = failAfter50Ms(new Statement() { @Override public void evaluate() { + statementWasExecuted.set(true); ThreadGroup group = currentThread().getThreadGroup(); assertSame("inner thread should use the same thread group", outerThreadGroup, group); } }); - runnable.run(); + failOnTimeout.evaluate(); + + assertTrue("the Statement was never run", statementWasExecuted.get()); + } + + private FailOnTimeout failAfter50Ms(Statement statement) { + return FailOnTimeout.builder() + .withTimeout(50, MILLISECONDS) + .withLookingForStuckThread(lookingForStuckThread) + .build(statement); + } + + private ThrowingRunnable run(final FailOnTimeout failOnTimeout) { + return new ThrowingRunnable() { + public void run() throws Throwable { + failOnTimeout.evaluate(); + } + }; + } + + private static class DelegateStatement extends Statement { + Statement delegate; + + @Override + public void evaluate() throws Throwable { + delegate.evaluate(); + } + } + + private static class FastStatement extends Statement { + @Override + public void evaluate() throws Throwable { + } + } + + private static final class InfiniteLoop extends Statement { + final AtomicBoolean stillExecuting = new AtomicBoolean(); + + @Override + public void evaluate() throws Throwable { + while (true) { + sleep(10); // sleep in order to enable interrupting thread + stillExecuting.set(true); + } + } } }