Skip to content
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

Time Watcher #552

Merged
merged 9 commits into from
Dec 3, 2012
Merged
Show file tree
Hide file tree
Changes from 3 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
140 changes: 140 additions & 0 deletions src/main/java/org/junit/rules/TimeWatcher.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
package org.junit.rules;

import org.junit.internal.AssumptionViolatedException;
import org.junit.runner.Description;

/**
* The TimeWatcher Rule notifies one of own protected methods of the time spent by a test.<p/>
* Override them to get the time in nanoseconds. For example, this class will keep logging the
* time spent by each passing, failing and skipped test:
*
* <pre>
* public static class TimeWatcherTest {
* private static final Logger logger = Logger.getLogger(&quot;&quot;);
Copy link
Member

Choose a reason for hiding this comment

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

The indentation looks off here and on the following lines.

*
* private static void logInfo(String testName, String status, long nanos) {
* logger.info(String.format(&quot;Test %s %s, spent %d microseconds&quot;,
* testName, status, TimeWatcher.toMicros(nanos)));
* }
*
* &#064;Rule
* public TimeWatcher timeWatcher= new TimeWatcher() {
* &#064;Override
* protected void succeeded(long nanos, Description description) {
* logInfo(description.getMethodName(), &quot;succeeded&quot;, nanos);
* }
*
* &#064;Override
* protected void failed(long nanos, Throwable e, Description description) {
* logInfo(description.getMethodName(), &quot;failed&quot;, nanos);
* }
*
* &#064;Override
* protected void skipped(long nanos, AssumptionViolatedException e, Description description) {
* logInfo(description.getMethodName(), &quot;skipped&quot;, nanos);
* }
*
* &#064;Override
* protected void finished(long nanos, Description description) {
* logInfo(description.getMethodName(), &quot;finished&quot;, nanos);
* }
* };
*
* &#064;Test
* public void succeeds() {
* }
*
* &#064;Test
* public void fails() {
* fail();
* }
*
* &#064;Test
* public void skips() {
* assumeTrue(false);
* }
* }
* </pre>
*
* @author tibor17
* @since 4.12
*/
public class TimeWatcher extends TestWatcher {
private volatile long startNanos = 0L;
private volatile long endNanos = 0L;

/**
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reason to repeat the Javadoc of TestWatcher here. Can we remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Never mind. We should keep the Javadoc since these are new methods.

* Invoked when a test succeeds
*/
protected void succeeded(long nanos, Description description) {
}

/**
* Invoked when a test fails
*/
protected void failed(long nanos, Throwable e, Description description) {
}

/**
* Invoked when a test is skipped due to a failed assumption.
*/
protected void skipped(long nanos, AssumptionViolatedException e, Description description) {
}

/**
* Invoked when a test method finishes (whether passing or failing)
*/
protected void finished(long nanos, Description description) {
}

/**
* @param nanos time in nanoseconds
* @return time converted to microseconds
*/
public static long toMicros(long nanos) {
return nanos / (1000);
Copy link
Member

Choose a reason for hiding this comment

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

Can we use java.util.concurrent.TimeUnit do do these convertions? Maybe we can even get rid of the methods completely?

}

/**
* @param nanos time in nanoseconds
* @return time converted to milliseconds
*/
public static long toMillis(long nanos) {
return nanos / (1000 * 1000);
}

/**
* @param nanos time in nanoseconds
* @return time converted to seconds
*/
public static long toSeconds(long nanos) {
return nanos / (1000 * 1000 * 1000);
}

private long getNanos() {
return endNanos - startNanos;
}

@Override final protected void succeeded(Description description) {
endNanos = System.nanoTime();
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 should get rid of the duplication by extracting this line into a private method. For symmetry, I would extract the assignment to startNanos in starting() as well.

succeeded(getNanos(), description);
}

@Override final protected void failed(Throwable e, Description description) {
endNanos = System.nanoTime();
failed(getNanos(), e, description);
}

@Override final protected void skipped(AssumptionViolatedException e, Description description) {
endNanos = System.nanoTime();
skipped(getNanos(), e, description);
}

@Override final protected void starting(Description description) {
startNanos = System.nanoTime();
}

@Override final protected void finished(Description description) {
finished(getNanos(), description);
}
}
4 changes: 3 additions & 1 deletion src/test/java/org/junit/tests/AllTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.junit.tests.experimental.rules.TempFolderRuleTest;
import org.junit.tests.experimental.rules.TemporaryFolderUsageTest;
import org.junit.tests.experimental.rules.TestRuleTest;
import org.junit.tests.experimental.rules.TimeWatcherTest;
import org.junit.tests.experimental.rules.TimeoutRuleTest;
import org.junit.tests.experimental.rules.VerifierRuleTest;
import org.junit.tests.experimental.theories.AllMembersSupplierTest;
Expand Down Expand Up @@ -160,7 +161,8 @@
BlockJUnit4ClassRunnerTest.class,
MethodSorterTest.class,
TestedOnSupplierTest.class,
StacktracePrintingMatcherTest.class
StacktracePrintingMatcherTest.class,
TimeWatcherTest.class
})
public class AllTests {
public static Test suite() {
Expand Down
139 changes: 139 additions & 0 deletions src/test/java/org/junit/tests/experimental/rules/TimeWatcherTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package org.junit.tests.experimental.rules;

import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.internal.AssumptionViolatedException;
import org.junit.rules.TimeWatcher;
import org.junit.runner.Description;
import org.junit.runner.JUnitCore;
import org.junit.runner.Result;

import java.util.logging.Logger;

import static org.hamcrest.core.Is.is;
import static org.hamcrest.core.IsEqual.equalTo;
import static org.hamcrest.core.IsNot.not;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assume.assumeTrue;
import static org.junit.Assert.fail;

/**
* @author tibor17
* @since 4.12
*/
public class TimeWatcherTest {
public static enum TestStatus {PASSED, FAILED, SKIPPED }

public static abstract class AbstractTimeWatcherTest {
private static final Logger logger = Logger.getLogger("");

public static long timeSpent;
public static long timeSpentIfFinished;
public static String testName;
public static String testNameIfFinished;
public static TestStatus status;

private static void logInfo(String name, String status, long nanos) {
Copy link
Member

Choose a reason for hiding this comment

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

This method is never called, is it? If not, let's please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in a comment. Reason is that we declare here that the Javadoc works.
Unfortunately we are not allowed to print strings within test round. Or are we? If we are, then i would uncomment the usage.

Choose a reason for hiding this comment

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

Instead of logging, append output to a StringBuilder which can then be asserted against in each test.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the logging adds anything to the test, does it? IMHO logging is just one example of how to use this new rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As i discussed similar issue with David, he likes explanatory tests. Yes, logger was an example of use.
I will remove it, and then comment again if you like it more.
Okay.

logger.info(String.format("Test '%s' %s, spent %d microseconds",
name, status, TimeWatcher.toMicros(nanos)));
}

@Rule
public final TimeWatcher timeWatcher = new TimeWatcher() {
@Override
protected void succeeded(long nanos, Description description) {
timeSpent = nanos;
status = TestStatus.PASSED;
Copy link
Member

Choose a reason for hiding this comment

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

I liked SUCCEEDED better because it is in line with succeeded and SuccessfulTest.

testName = description.getMethodName();
//logInfo(testName, status.name(), timeSpent);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove commented out code (same below).

}

@Override
protected void failed(long nanos, Throwable e, Description description) {
timeSpent = nanos;
status = TestStatus.FAILED;
testName = description.getMethodName();
//logInfo(testName, status.name(), timeSpent);
}

@Override
protected void skipped(long nanos, AssumptionViolatedException e, Description description) {
timeSpent = nanos;
status = TestStatus.SKIPPED;
testName = description.getMethodName();
//logInfo(testName, status.name(), timeSpent);
}

@Override
protected void finished(long nanos, Description description) {
timeSpentIfFinished = nanos;
testNameIfFinished = description.getMethodName();
//logInfo(testNameIfFinished, "finished", timeSpentIfFinished);
}
};
}

public static class SuccessfulTest extends AbstractTimeWatcherTest {
@Test
public void successfulTest() {
}
}

public static class FailedTest extends AbstractTimeWatcherTest {
@Test
public void failedTest() {
fail();
}
}

public static class SkippedTest extends AbstractTimeWatcherTest {
@Test
public void skippedTest() {
assumeTrue(false);
}
}

@Before
public void init() {
AbstractTimeWatcherTest.testName = null;
AbstractTimeWatcherTest.testNameIfFinished = null;
AbstractTimeWatcherTest.status = null;
AbstractTimeWatcherTest.timeSpent = 0;
AbstractTimeWatcherTest.timeSpentIfFinished = 0;
}

@Test
public void succeeded() {
Result result = JUnitCore.runClasses(SuccessfulTest.class);
assertEquals(0, result.getFailureCount());
assertThat(AbstractTimeWatcherTest.testName, is(equalTo("successfulTest")));
Copy link
Member

Choose a reason for hiding this comment

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

Probably a matter of taste... I would leave out equalTo as it is redundant and not more readable than simply is(...).

Choose a reason for hiding this comment

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

Yes, that's actually what I meant with my comment. Reading "x is five" is perfectly clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the hamcrest implementation is() behaves as is(equalTo())

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what we meant. Do you think is(equalTo(...)) reads better?

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
What I mean is(...) is better and more simple, typically narrative code.

assertThat(AbstractTimeWatcherTest.testName, is(equalTo(AbstractTimeWatcherTest.testNameIfFinished)));
assertThat(AbstractTimeWatcherTest.status, is(equalTo(TestStatus.PASSED)));
assertThat(AbstractTimeWatcherTest.timeSpent, is(not(0L)));
Copy link
Member

Choose a reason for hiding this comment

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

assertTrue("timeSpent > 0", AbstractTimeWatcherTest.timeSpent > 0) (since greaterThan is not part of Hamcrest core)?

assertThat(AbstractTimeWatcherTest.timeSpent, is(equalTo(AbstractTimeWatcherTest.timeSpentIfFinished)));
}

@Test
public void failed() {
Result result = JUnitCore.runClasses(FailedTest.class);
assertEquals(1, result.getFailureCount());
assertThat(AbstractTimeWatcherTest.testName, is(equalTo("failedTest")));
assertThat(AbstractTimeWatcherTest.testName, is(equalTo(AbstractTimeWatcherTest.testNameIfFinished)));
assertThat(AbstractTimeWatcherTest.status, is(equalTo(TestStatus.FAILED)));
assertThat(AbstractTimeWatcherTest.timeSpent, is(not(0L)));
assertThat(AbstractTimeWatcherTest.timeSpent, is(equalTo(AbstractTimeWatcherTest.timeSpentIfFinished)));
}

@Test
public void skipped() {
Result result = JUnitCore.runClasses(SkippedTest.class);
assertEquals(0, result.getFailureCount());
assertThat(AbstractTimeWatcherTest.testName, is(equalTo("skippedTest")));
assertThat(AbstractTimeWatcherTest.testName, is(equalTo(AbstractTimeWatcherTest.testNameIfFinished)));
assertThat(AbstractTimeWatcherTest.status, is(equalTo(TestStatus.SKIPPED)));
assertThat(AbstractTimeWatcherTest.timeSpent, is(not(0L)));
assertThat(AbstractTimeWatcherTest.timeSpent, is(equalTo(AbstractTimeWatcherTest.timeSpentIfFinished)));
}
}