Skip to content

forked from #450 and improved #544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 15, 2012
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,21 @@

import org.junit.runners.model.Statement;

import java.util.concurrent.TimeUnit;

public class FailOnTimeout extends Statement {
private final Statement fOriginalStatement;

private final TimeUnit fTimeUnit;
private final long fTimeout;

public FailOnTimeout(Statement originalStatement, long timeout) {
public FailOnTimeout(Statement originalStatement, long millis) {
this(originalStatement, millis, TimeUnit.MILLISECONDS);
}

public FailOnTimeout(Statement originalStatement, long timeout, TimeUnit unit) {
fOriginalStatement = originalStatement;
fTimeout = timeout;
fTimeUnit = unit;
}

@Override
Expand All @@ -23,7 +30,7 @@ public void evaluate() throws Throwable {
private StatementThread evaluateStatement() throws InterruptedException {
StatementThread thread = new StatementThread(fOriginalStatement);
thread.start();
thread.join(fTimeout);
fTimeUnit.timedJoin(thread, fTimeout);
if (!thread.fFinished) {
thread.recordStackTrace();
}
Expand All @@ -42,7 +49,7 @@ private void throwExceptionForUnfinishedThread(StatementThread thread)

private void throwTimeoutException(StatementThread thread) throws Exception {
Exception exception = new Exception(String.format(
"test timed out after %d milliseconds", fTimeout));
"test timed out after %d %s", fTimeout, fTimeUnit.name().toLowerCase()));
exception.setStackTrace(thread.getRecordedStackTrace());
throw exception;
}
Expand Down
82 changes: 66 additions & 16 deletions src/main/java/org/junit/rules/Timeout.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,45 +4,95 @@
import org.junit.runner.Description;
import org.junit.runners.model.Statement;

import java.util.concurrent.TimeUnit;

/**
* The Timeout Rule applies the same timeout to all test methods in a class:
*
* <pre>
* public static class HasGlobalTimeout {
* public static String log;
* public static class HasGlobalLongTimeout {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the name change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsaff
because it is named class according the pull i forked from

*
* &#064;Rule
* public Timeout globalTimeout= new Timeout(20);
*
* &#064;Test
* public void testInfiniteLoop1() {
* log+= &quot;ran1&quot;;
* for (;;) {
* }
* }
* public void run1() throws InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the new javadoc is more confusing than the old javadoc. We can't enumerate all the ways a test could time out, so why choose these few?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have also the infinite loop, just see my last update which added run4.

There are only two things which may happen in user's code:

  • not interrupted execution -we are testing with infinite loops
  • a code which can be interrupted will be interrupter after timeout. It is a leegal situation in methods which throw InterrupterException initiated by JVM, and there are only twose I mentioned, noting more.

Because this is a different scenario than loops, and nobody mentioned before.

It's important that the thread is interrupted (Thread.interrupt()), therefore should be also tested in this way and we are testing it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can of course limit the ways when interrupted code. Yes, maybe it's too much.

* Thread.sleep(100);
* }
*
* &#064;Test
* public void run2() throws InterruptedException {
* reentrantLock.lockInterruptibly();
* try {
* ...
* } finally {
* reentrantLock.unlock();
* }
* }
*
* &#064;Test
* public synchronized void run3() throws InterruptedException {
* ...
* wait();
* }
*
* &#064;Test
* public void testInfiniteLoop2() {
* log+= &quot;ran2&quot;;
* for (;;) {
* }
* }
* public void run4() {
* //uninterrupted code
* for (;;)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the canonical Java infinite loop is

while (true) {}

I think if we name the method infiniteLoop(), and use that style, we can drop the comment-within-a-javadoc below.

* ; //extremely, an infinite loop
* }
* }
* </pre>
*
* A test is running in an extra thread, and its execution is interrupted via {@link Thread#interrupt}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's tighten the text.

"Each test is run in a new thread. If the specified timeout elapses before the test completes, its execution is interrupted via Thread#interrupt. This happens in [etc]"

* when the timeout elapsed. This happens in interruptable I/O and locks, and methods in {@link Object}
* and {@link Thread} throwing {@link InterruptedException}.
*
* @since 4.7
*/
public class Timeout implements TestRule {
private final int fMillis;
private final long fTimeout;
private final TimeUnit fTimeUnit;

/**
* @param millis the millisecond timeout
* Create a {@code Timeout} instance with the timeout specified
* in milliseconds.
* This constructor is deprecated and will be removed in the next release.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take off "and will be removed in the next release".

* Instead use {@link #Timeout(long)} or
* {@link #Timeout(long, java.util.concurrent.TimeUnit)}.
*
* @param millis the maximum time in milliseconds to allow the
* test to run before it should timeout
*/
@Deprecated
public Timeout(int millis) {
fMillis = millis;
this((long) millis);
}

/**
* @param millis the millisecond timeout
* @since 4.11
*/
public Timeout(long millis) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we decided the last time we introduced TimeUnit to a class that the benefit to having three constructors wasn't worth the cognitive load of having to choose which constructor. Let's please not add this constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsaff
Should be this constructor removed now?
thx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcphilipp
@dsaff
@kcooney
@dharkness

I would be very happy to remove the constructor Timeout(int), but i am worry about what would happen if i do.
So i simulated this scenario, where your tests dependent on 4.10 using Timeout(int) suddenly change the junit version without recompiling them based on junit 4.11.

This will happen:

Exception in thread "main" java.lang.NoSuchMethodError: org.junit.rules.Timeout.(I)V

If it's the rule to update the documentation, then we should do it. If not, then i will remove this constructor.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the suggestion was to remove the constructor taking a long value. Keep the original int constructor for compatibility and add only the long + TimeUnit constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dharkness

See #406, which says that casting long to int is an evil due to truncation in numbers.

new Timeout( (int) DateUtils.MILLIS_PER_MINUTE * 5 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option that I like is to have

@Deprecated Timeout(int) {}
Timeout(long, TimeUnit)
static Timeout millis(long millis);
static Timeout seconds(long seconds);
static Timeout minutes(long minutes);

That way there's easy, non-confusable shortcuts for the most common options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsaff
I can do it now, but then you have two paradigms in one: a factory and typical object creation.

this(millis, TimeUnit.MILLISECONDS);
}

/**
* Create a {@code Timeout} instance with the timeout specified
* at the unit of granularity of the provided {@code TimeUnit}.
*
* @param timeout the maximum time to allow the test to run
* before it should timeout
* @param unit the time unit for the {@code timeout}
* @since 4.11
*/
public Timeout(long timeout, TimeUnit unit) {
fTimeout = timeout;
fTimeUnit = unit;
}

public Statement apply(Statement base, Description description) {
return new FailOnTimeout(base, fMillis);
return new FailOnTimeout(base, fTimeout, fTimeUnit);
}
}
119 changes: 97 additions & 22 deletions src/test/java/org/junit/tests/experimental/rules/TimeoutRuleTest.java
Original file line number Diff line number Diff line change
@@ -1,46 +1,121 @@
package org.junit.tests.experimental.rules;

import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;

import org.junit.Ignore;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestRule;
import org.junit.rules.Timeout;
import org.junit.runner.JUnitCore;
import org.junit.runner.Result;

import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantLock;

import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;

public class TimeoutRuleTest {
public static class HasGlobalTimeout {
public static String log;
private static final ReentrantLock run1Lock = new ReentrantLock();

private static volatile boolean run4done = false;

public static class HasGlobalLongTimeout {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could reduce the duplication between this class and the next one:

public abstract static class AbstractTimeout {
  // fields _except_ for @Rule

  @Test public void run1() { ... }
  @Test public void run2() { ... }
  @Test public void run3() { ... }
  @Test public void run4() { ... }
}

public static class HasGlobalTimeout extends AbstractTimeout {
  @Rule public TestRule globalTimeout = new Timeout(20);
}

public static class HasGlobalTimeUnitTimeout {
  @Rule public final TestRule globalTimeout = new Timeout(50, TimeUnit.MILLISECONDS);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsaff
makes sense

public static final StringBuffer logger = new StringBuffer();

@Rule
public TestRule globalTimeout = new Timeout(20);
public final TestRule globalTimeout = new Timeout(50L);

@Test
public void run1() throws InterruptedException {
logger.append("run1");
TimeoutRuleTest.run1Lock.lockInterruptibly();
TimeoutRuleTest.run1Lock.unlock();
}

@Test
public void run2() throws InterruptedException {
logger.append("run2");
Thread.currentThread().join();
}

@Test
public synchronized void run3() throws InterruptedException {
logger.append("run3");
wait();
}

@Test
public void testInfiniteLoop1() {
log += "ran1";
for (; ; ) {
public void run4() {
logger.append("run4");
while (!run4done) {
}
}
}

public static class HasGlobalTimeUnitTimeout {
public static final StringBuffer logger = new StringBuffer();

@Rule
public final TestRule globalTimeout = new Timeout(50, TimeUnit.MILLISECONDS);

@Test
public void run1() throws InterruptedException {
logger.append("run1");
TimeoutRuleTest.run1Lock.lockInterruptibly();
TimeoutRuleTest.run1Lock.unlock();
}

@Test
public void run2() throws InterruptedException {
logger.append("run2");
Thread.currentThread().join();
}

@Test
public void testInfiniteLoop2() {
log += "ran2";
for (; ; ) {
public synchronized void run3() throws InterruptedException {
logger.append("run3");
wait();
}

@Test
public void run4() {
logger.append("run4");
while (!run4done) {
}
}
}

@Ignore("For gump, for now")
@Test(timeout = 100)
public void globalTimeoutAvoidsInfiniteLoop() {
HasGlobalTimeout.log = "";
Result result = JUnitCore.runClasses(HasGlobalTimeout.class);
assertEquals(2, result.getFailureCount());
assertThat(HasGlobalTimeout.log, containsString("ran1"));
assertThat(HasGlobalTimeout.log, containsString("ran2"));
@Before public void before() {
run4done = false;
run1Lock.lock();
}

@After public void after() {
run4done = true;//to make sure that the thread won't continue at run4()
run1Lock.unlock();
}

@Test
public void timeUnitTimeout() throws InterruptedException {
HasGlobalTimeUnitTimeout.logger.setLength(0);
Result result = JUnitCore.runClasses(HasGlobalTimeUnitTimeout.class);
assertEquals(4, result.getFailureCount());
assertThat(HasGlobalTimeUnitTimeout.logger.toString(), containsString("run1"));
assertThat(HasGlobalTimeUnitTimeout.logger.toString(), containsString("run2"));
assertThat(HasGlobalTimeUnitTimeout.logger.toString(), containsString("run3"));
assertThat(HasGlobalTimeUnitTimeout.logger.toString(), containsString("run4"));
}

@Test
public void longTimeout() throws InterruptedException {
HasGlobalLongTimeout.logger.setLength(0);
Result result = JUnitCore.runClasses(HasGlobalLongTimeout.class);
assertEquals(4, result.getFailureCount());
assertThat(HasGlobalLongTimeout.logger.toString(), containsString("run1"));
assertThat(HasGlobalLongTimeout.logger.toString(), containsString("run2"));
assertThat(HasGlobalLongTimeout.logger.toString(), containsString("run3"));
assertThat(HasGlobalLongTimeout.logger.toString(), containsString("run4"));
}
}