From 0bec573229bdd725b1d4588bd3c1ad9ba38f29eb Mon Sep 17 00:00:00 2001 From: Stefan Birkner Date: Fri, 9 Feb 2018 10:21:48 +0100 Subject: [PATCH 1/2] Tidy up tests for TestWatcher - Make it easier to understand TestWatcher's behavior based on the tests. - Improve expressiveness of the test. - Add missing tests. --- .../org/junit/rules/LoggingTestWatcher.java | 5 + .../java/org/junit/rules/TestRuleTest.java | 153 ------- .../java/org/junit/rules/TestWatcherTest.java | 405 ++++++++++++------ 3 files changed, 284 insertions(+), 279 deletions(-) diff --git a/src/test/java/org/junit/rules/LoggingTestWatcher.java b/src/test/java/org/junit/rules/LoggingTestWatcher.java index f1c0ddafba9b..44d32a3e5a2c 100644 --- a/src/test/java/org/junit/rules/LoggingTestWatcher.java +++ b/src/test/java/org/junit/rules/LoggingTestWatcher.java @@ -25,6 +25,11 @@ protected void skipped(AssumptionViolatedException e, Description description) { log.append("skipped "); } + @Override + protected void skipped(org.junit.internal.AssumptionViolatedException e, Description description) { + log.append("deprecated skipped "); + } + @Override protected void starting(Description description) { log.append("starting "); diff --git a/src/test/java/org/junit/rules/TestRuleTest.java b/src/test/java/org/junit/rules/TestRuleTest.java index 26a6e58bbb40..55fbb85156ae 100644 --- a/src/test/java/org/junit/rules/TestRuleTest.java +++ b/src/test/java/org/junit/rules/TestRuleTest.java @@ -148,64 +148,6 @@ public void ignoreNonRules() { private static String log; - public static class OnFailureTest { - @Rule - public TestRule watcher = new TestWatcher() { - @Override - protected void failed(Throwable e, Description description) { - log += description + " " + e.getClass().getSimpleName(); - } - }; - - @Test - public void nothing() { - fail(); - } - } - - @Test - public void onFailure() { - log = ""; - Result result = JUnitCore.runClasses(OnFailureTest.class); - assertEquals(String.format("nothing(%s) AssertionError", OnFailureTest.class.getName()), log); - assertEquals(1, result.getFailureCount()); - } - - public static class WatchmanTest { - private static String watchedLog; - - @Rule - public TestRule watcher = new TestWatcher() { - @Override - protected void failed(Throwable e, Description description) { - watchedLog += description + " " - + e.getClass().getSimpleName() + "\n"; - } - - @Override - protected void succeeded(Description description) { - watchedLog += description + " " + "success!\n"; - } - }; - - @Test - public void fails() { - fail(); - } - - @Test - public void succeeds() { - } - } - - @Test - public void succeeded() { - WatchmanTest.watchedLog = ""; - JUnitCore.runClasses(WatchmanTest.class); - assertThat(WatchmanTest.watchedLog, containsString(String.format("fails(%s) AssertionError", WatchmanTest.class.getName()))); - assertThat(WatchmanTest.watchedLog, containsString(String.format("succeeds(%s) success!", WatchmanTest.class.getName()))); - } - public static class BeforesAndAfters { private static StringBuilder watchedLog = new StringBuilder(); @@ -437,101 +379,6 @@ public void methodIgnoreNonRules() { assertEquals(0, result.getFailureCount()); } - public static class MethodOnFailureTest { - private TestRule watchman = new TestWatcher() { - @Override - protected void failed(Throwable e, Description description) { - log += description + " " + e.getClass().getSimpleName(); - } - }; - - @Rule - public TestRule getWatchman() { - return watchman; - } - - @Test - public void nothing() { - fail(); - } - } - - @Test - public void methodOnFailure() { - log = ""; - Result result = JUnitCore.runClasses(MethodOnFailureTest.class); - assertEquals(String.format("nothing(%s) AssertionError", MethodOnFailureTest.class.getName()), log); - assertEquals(1, result.getFailureCount()); - } - - public static class MethodOnSkippedTest { - private TestRule watchman = new TestWatcher() { - @Override - protected void skipped(AssumptionViolatedException e, Description description) { - log += description + " " + e.getClass().getSimpleName(); - } - }; - - @Rule - public TestRule getWatchman() { - return watchman; - } - - @Test - public void nothing() { - Assume.assumeTrue(false); - } - } - - @Test - public void methodOnSkipped() { - log = ""; - Result result = JUnitCore.runClasses(MethodOnSkippedTest.class); - assertEquals(String.format("nothing(%s) AssumptionViolatedException", MethodOnSkippedTest.class.getName()), log); - assertEquals(0, result.getFailureCount()); - assertEquals(1, result.getRunCount()); - } - - public static class MethodWatchmanTest { - @SuppressWarnings("unused") - private static String watchedLog; - - private TestRule watchman = new TestWatcher() { - @Override - protected void failed(Throwable e, Description description) { - watchedLog += description + " " - + e.getClass().getSimpleName() + "\n"; - } - - @Override - protected void succeeded(Description description) { - watchedLog += description + " " + "success!\n"; - } - }; - - @Rule - public TestRule getWatchman() { - return watchman; - } - - @Test - public void fails() { - fail(); - } - - @Test - public void succeeds() { - } - } - - @Test - public void methodSucceeded() { - WatchmanTest.watchedLog = ""; - JUnitCore.runClasses(WatchmanTest.class); - assertThat(WatchmanTest.watchedLog, containsString(String.format("fails(%s) AssertionError", WatchmanTest.class.getName()))); - assertThat(WatchmanTest.watchedLog, containsString(String.format("succeeds(%s) success!", WatchmanTest.class.getName()))); - } - public static class BeforesAndAftersAreEnclosedByRule { private static StringBuilder log; diff --git a/src/test/java/org/junit/rules/TestWatcherTest.java b/src/test/java/org/junit/rules/TestWatcherTest.java index 100a13f540c8..c7fa5c070f94 100644 --- a/src/test/java/org/junit/rules/TestWatcherTest.java +++ b/src/test/java/org/junit/rules/TestWatcherTest.java @@ -1,192 +1,345 @@ package org.junit.rules; -import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.fail; +import static java.util.Arrays.asList; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.*; import static org.junit.Assume.assumeTrue; import static org.junit.experimental.results.PrintableResult.testResult; import static org.junit.experimental.results.ResultMatchers.failureCountIs; import static org.junit.experimental.results.ResultMatchers.hasFailureContaining; -import static org.junit.runner.JUnitCore.runClasses; + import org.junit.Rule; import org.junit.Test; import org.junit.experimental.results.PrintableResult; +import org.junit.experimental.runners.Enclosed; import org.junit.internal.AssumptionViolatedException; import org.junit.runner.Description; +import org.junit.runner.JUnitCore; +import org.junit.runner.Result; +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 java.util.List; +@RunWith(Enclosed.class) public class TestWatcherTest { - public static class ViolatedAssumptionTest { - private static StringBuilder watchedLog = new StringBuilder(); - @Rule - public TestRule watcher = new LoggingTestWatcher(watchedLog); + @RunWith(Parameterized.class) + public static class Callbacks { + + @Parameters(name = "{0}") + public static Object[][] parameters() { + return new Object[][] { + { + FailingTest.class, + "starting failed finished ", + asList("starting failed", "test failed", "failed failed", "finished failed") }, + { + InternalViolatedAssumptionTest.class, + "starting deprecated skipped finished ", + asList("starting failed", "don't run", "deprecated skipped failed", "finished failed") }, + { + SuccessfulTest.class, + "starting succeeded finished ", + asList("starting failed", "succeeded failed", "finished failed") }, + { + ViolatedAssumptionTest.class, + "starting skipped finished ", + asList("starting failed", "Test could not be skipped due to other failures", "skipped failed", "finished failed") } + }; + } + + @Parameter(0) + public Class testClass; + + @Parameter(1) + public String expectedCallbacks; + + @Parameter(2) + public List expectedFailures; + + private static TestRule selectedRule; //for injecting rule into test classes @Test - public void succeeds() { - assumeTrue(false); + public void correctCallbacksCalled() { + StringBuilder log = new StringBuilder(); + selectedRule = new LoggingTestWatcher(log); + JUnitCore.runClasses(testClass); + assertEquals(expectedCallbacks, log.toString()); } - } - @Test - public void neitherLogSuccessNorFailedForViolatedAssumption() { - ViolatedAssumptionTest.watchedLog = new StringBuilder(); - runClasses(ViolatedAssumptionTest.class); - assertThat(ViolatedAssumptionTest.watchedLog.toString(), - is("starting skipped finished ")); - } + @Test + public void resultHasAllFailuresThrownByCallbacks() { + selectedRule = new ErroneousTestWatcher(); + PrintableResult result = testResult(testClass); + assertThat(result, failureCountIs(expectedFailures.size())); + for (String expectedFailure: expectedFailures) { + assertThat(result, hasFailureContaining(expectedFailure)); + } + } - public static class InternalViolatedAssumptionTest { - private static StringBuilder watchedLog = new StringBuilder(); + @Test + public void testWatcherDoesNotModifyResult() { + selectedRule = new NoOpRule(); + Result resultNoOpRule = JUnitCore.runClasses(testClass); + selectedRule = new LoggingTestWatcher(new StringBuilder()); + Result resultTestWatcher = JUnitCore.runClasses(testClass); + assertEquals( + "was successful", + resultNoOpRule.wasSuccessful(), + resultTestWatcher.wasSuccessful()); + assertEquals( + "failure count", + resultNoOpRule.getFailureCount(), + resultTestWatcher.getFailureCount()); + assertEquals( + "ignore count", + resultNoOpRule.getIgnoreCount(), + resultTestWatcher.getIgnoreCount()); + assertEquals( + "run count", + resultNoOpRule.getRunCount(), + resultTestWatcher.getRunCount()); + } - @Rule - public TestRule watcher = new TestWatcher() { + private static class NoOpRule implements TestRule { + public Statement apply(Statement base, Description description) { + return base; + } + } + + private static class ErroneousTestWatcher extends TestWatcher { @Override - protected void starting(Description description) { - watchedLog.append("starting "); + protected void succeeded(Description description) { + throw new RuntimeException("succeeded failed"); } @Override - protected void finished(Description description) { - watchedLog.append("finished "); + protected void failed(Throwable e, Description description) { + throw new RuntimeException("failed failed"); } @Override + protected void skipped(org.junit.AssumptionViolatedException e, Description description) { + throw new RuntimeException("skipped failed"); + } + + @Override + @SuppressWarnings("deprecation") protected void skipped(AssumptionViolatedException e, Description description) { - watchedLog.append("skipped "); + throw new RuntimeException("deprecated skipped failed"); } - }; - @SuppressWarnings("deprecation") - @Test - public void succeeds() { - throw new AssumptionViolatedException("don't run"); + @Override + protected void starting(Description description) { + throw new RuntimeException("starting failed"); + } + + @Override + protected void finished(Description description) { + throw new RuntimeException("finished failed"); + } } - } - @Test - public void internalViolatedAssumption() { - InternalViolatedAssumptionTest.watchedLog = new StringBuilder(); - runClasses(InternalViolatedAssumptionTest.class); - assertThat(InternalViolatedAssumptionTest.watchedLog.toString(), - is("starting skipped finished ")); - } + public static class FailingTest { + @Rule + public TestRule rule = selectedRule; - public static class TestWatcherSkippedThrowsExceptionTest { - @Rule - public TestRule watcher = new TestWatcher() { - @Override - protected void skipped(AssumptionViolatedException e, Description description) { - throw new RuntimeException("watcher failure"); + @Test + public void test() { + fail("test failed"); } - }; + } - @SuppressWarnings("deprecation") - @Test - public void fails() { - throw new AssumptionViolatedException("test failure"); + public static class InternalViolatedAssumptionTest { + @Rule + public TestRule watcher = selectedRule; + + @SuppressWarnings("deprecation") + @Test + public void test() { + throw new AssumptionViolatedException("don't run"); + } } - } - @Test - public void testWatcherSkippedThrowsException() { - PrintableResult result = testResult(TestWatcherSkippedThrowsExceptionTest.class); - assertThat(result, failureCountIs(2)); - assertThat(result, hasFailureContaining("test failure")); - assertThat(result, hasFailureContaining("watcher failure")); + public static class SuccessfulTest { + @Rule + public TestRule watcher = selectedRule; + + @Test + public void test() { + } + } + + public static class ViolatedAssumptionTest { + @Rule + public TestRule watcher = selectedRule; + + @Test + public void test() { + assumeTrue(false); + } + } } - public static class FailingTest { - private static StringBuilder watchedLog = new StringBuilder(); + public static class CallbackArguments { + + public static class Succeeded { + private static Description catchedDescription; - @Rule - public TestRule watcher = new LoggingTestWatcher(watchedLog); + @Rule + public final TestRule watcher = new TestWatcher() { + @Override + protected void succeeded(Description description) { + catchedDescription = description; + } + }; + + @Test + public void test() { + } + } @Test - public void succeeds() { - fail(); + public void succeeded() { + JUnitCore.runClasses(Succeeded.class); + assertEquals("test(org.junit.rules.TestWatcherTest$CallbackArguments$Succeeded)", + Succeeded.catchedDescription.getDisplayName()); } - } - @Test - public void logFailingTest() { - FailingTest.watchedLog = new StringBuilder(); - runClasses(FailingTest.class); - assertThat(FailingTest.watchedLog.toString(), - is("starting failed finished ")); - } + public static class Failed { + private static Description catchedDescription; + private static Throwable catchedThrowable; - public static class TestWatcherFailedThrowsExceptionTest { - @Rule - public TestRule watcher = new TestWatcher() { - @Override - protected void failed(Throwable e, Description description) { - throw new RuntimeException("watcher failure"); + @Rule + public final TestRule watcher = new TestWatcher() { + @Override + protected void failed(Throwable e, Description description) { + catchedDescription = description; + catchedThrowable = e; + } + }; + + @Test + public void test() { + fail("test failed"); } - }; + } @Test - public void fails() { - throw new IllegalArgumentException("test failure"); + public void failed() { + JUnitCore.runClasses(Failed.class); + assertEquals("test failed", Failed.catchedThrowable.getMessage()); + assertEquals(AssertionError.class, Failed.catchedThrowable.getClass()); + assertEquals("test(org.junit.rules.TestWatcherTest$CallbackArguments$Failed)", + Failed.catchedDescription.getDisplayName()); } - } - @Test - public void testWatcherFailedThrowsException() { - PrintableResult result = testResult(TestWatcherFailedThrowsExceptionTest.class); - assertThat(result, failureCountIs(2)); - assertThat(result, hasFailureContaining("test failure")); - assertThat(result, hasFailureContaining("watcher failure")); - } + public static class Skipped { + private static Description catchedDescription; + private static org.junit.AssumptionViolatedException catchedException; - public static class TestWatcherStartingThrowsExceptionTest { - @Rule - public TestRule watcher = new TestWatcher() { - @Override - protected void starting(Description description) { - throw new RuntimeException("watcher failure"); + @Rule + public final TestRule watcher = new TestWatcher() { + @Override + protected void skipped(org.junit.AssumptionViolatedException e, Description description) { + catchedDescription = description; + catchedException = e; + } + }; + + @Test + public void test() { + assumeTrue("test skipped", false); } - }; + } @Test - public void fails() { - throw new IllegalArgumentException("test failure"); + public void skipped() { + JUnitCore.runClasses(Skipped.class); + assertEquals("test skipped", Skipped.catchedException.getMessage()); + assertEquals(org.junit.AssumptionViolatedException.class, Skipped.catchedException.getClass()); + assertEquals("test(org.junit.rules.TestWatcherTest$CallbackArguments$Skipped)", + Skipped.catchedDescription.getDisplayName()); } - } - @Test - public void testWatcherStartingThrowsException() { - PrintableResult result = testResult(TestWatcherStartingThrowsExceptionTest.class); - assertThat(result, failureCountIs(2)); - assertThat(result, hasFailureContaining("test failure")); - assertThat(result, hasFailureContaining("watcher failure")); - } + public static class DeprecatedSkipped { + private static Description catchedDescription; + private static AssumptionViolatedException catchedException; - public static class TestWatcherFailedAndFinishedThrowsExceptionTest { - @Rule - public TestRule watcher = new TestWatcher() { - @Override - protected void failed(Throwable e, Description description) { - throw new RuntimeException("watcher failed failure"); + @Rule + public final TestRule watcher = new TestWatcher() { + @Override + @SuppressWarnings("deprecation") + protected void skipped(AssumptionViolatedException e, Description description) { + catchedDescription = description; + catchedException = e; + } + }; + + @SuppressWarnings("deprecation") + @Test + public void test() { + throw new AssumptionViolatedException("test skipped"); } + } - @Override - protected void finished(Description description) { - throw new RuntimeException("watcher finished failure"); + @Test + public void deprecatedSkipped() { + JUnitCore.runClasses(DeprecatedSkipped.class); + assertEquals("test skipped", DeprecatedSkipped.catchedException.getMessage()); + assertEquals(AssumptionViolatedException.class, DeprecatedSkipped.catchedException.getClass()); + assertEquals("test(org.junit.rules.TestWatcherTest$CallbackArguments$DeprecatedSkipped)", + DeprecatedSkipped.catchedDescription.getDisplayName()); + } + + public static class Starting { + private static Description catchedDescription; + + @Rule + public final TestRule watcher = new TestWatcher() { + @Override + protected void starting(Description description) { + catchedDescription = description; + } + }; + + @Test + public void test() { } - }; + } @Test - public void fails() { - throw new IllegalArgumentException("test failure"); + public void starting() { + JUnitCore.runClasses(Starting.class); + assertEquals("test(org.junit.rules.TestWatcherTest$CallbackArguments$Starting)", + Starting.catchedDescription.getDisplayName()); } - } - @Test - public void testWatcherFailedAndFinishedThrowsException() { - PrintableResult result = testResult(TestWatcherFailedAndFinishedThrowsExceptionTest.class); - assertThat(result, failureCountIs(3)); - assertThat(result, hasFailureContaining("test failure")); - assertThat(result, hasFailureContaining("watcher failed failure")); - assertThat(result, hasFailureContaining("watcher finished failure")); + public static class Finished { + private static Description catchedDescription; + + @Rule + public final TestRule watcher = new TestWatcher() { + @Override + protected void finished(Description description) { + catchedDescription = description; + } + }; + + @Test + public void test() { + } + } + + @Test + public void finished() { + JUnitCore.runClasses(Finished.class); + assertEquals("test(org.junit.rules.TestWatcherTest$CallbackArguments$Finished)", + Finished.catchedDescription.getDisplayName()); + } } } From cf13399be099b42d8786b62c134108f854d54578 Mon Sep 17 00:00:00 2001 From: Stefan Birkner Date: Tue, 13 Feb 2018 04:14:57 +0100 Subject: [PATCH 2/2] Recommend to always set order for TestWatcher TestWatcher may see failed tests as successful and vice versa if it is not the outermost rule and another rule changes the result of a test (e.g. ErrorCollector or ExpectedException). Fixes #1436. --- .../java/org/junit/rules/TestWatcher.java | 8 ++- .../java/org/junit/rules/TestWatcherTest.java | 52 +++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/junit/rules/TestWatcher.java b/src/main/java/org/junit/rules/TestWatcher.java index b790dd304cc1..a28514dd2b2f 100644 --- a/src/main/java/org/junit/rules/TestWatcher.java +++ b/src/main/java/org/junit/rules/TestWatcher.java @@ -4,6 +4,7 @@ import java.util.List; import org.junit.AssumptionViolatedException; +import org.junit.Rule; import org.junit.runner.Description; import org.junit.runners.model.MultipleFailureException; import org.junit.runners.model.Statement; @@ -17,7 +18,7 @@ * public static class WatchmanTest { * private static String watchedLog; * - * @Rule + * @Rule(order = Integer.MIN_VALUE) * public TestWatcher watchman= new TestWatcher() { * @Override * protected void failed(Throwable e, Description description) { @@ -40,6 +41,11 @@ * } * } * + *

It is recommended to always set the {@link Rule#order() order} of the + * {@code TestWatcher} to {@code Integer.MIN_VALUE} so that it encloses all + * other rules. Otherwise it may see failed tests as successful and vice versa + * if some rule changes the result of a test (e.g. {@link ErrorCollector} or + * {@link ExpectedException}). * * @since 4.9 */ diff --git a/src/test/java/org/junit/rules/TestWatcherTest.java b/src/test/java/org/junit/rules/TestWatcherTest.java index c7fa5c070f94..e09e23cbb909 100644 --- a/src/test/java/org/junit/rules/TestWatcherTest.java +++ b/src/test/java/org/junit/rules/TestWatcherTest.java @@ -7,6 +7,7 @@ import static org.junit.experimental.results.PrintableResult.testResult; import static org.junit.experimental.results.ResultMatchers.failureCountIs; import static org.junit.experimental.results.ResultMatchers.hasFailureContaining; +import static org.junit.rules.ExpectedException.none; import org.junit.Rule; import org.junit.Test; @@ -342,4 +343,55 @@ public void finished() { Finished.catchedDescription.getDisplayName()); } } + + //The following tests check the information in TestWatcher's Javadoc + //regarding interplay with other rules. + public static class InterplayWithOtherRules { + private static StringBuilder log; + + public static class ExpectedExceptionTest { + @Rule(order = Integer.MIN_VALUE) + //the field name must be alphabetically lower than "thrown" in order + //to make the test failing if order is not set + public final TestRule a = new LoggingTestWatcher(log); + + @Rule + public final ExpectedException thrown = none(); + + @Test + public void testWithExpectedException() { + thrown.expect(RuntimeException.class); + throw new RuntimeException("expected exception"); + } + } + + @Test + public void expectedExceptionIsSeenAsSuccessfulTest() { + log = new StringBuilder(); + JUnitCore.runClasses(ExpectedExceptionTest.class); + assertEquals("starting succeeded finished ", log.toString()); + } + + public static class ErrorCollectorTest { + @Rule(order = Integer.MIN_VALUE) + //the field name must be alphabetically lower than "collector" in + //order to make the test failing if order is not set + public final TestRule a = new LoggingTestWatcher(log); + + @Rule + public final ErrorCollector collector = new ErrorCollector(); + + @Test + public void test() { + collector.addError(new RuntimeException("expected exception")); + } + } + + @Test + public void testIsSeenAsFailedBecauseOfCollectedError() { + log = new StringBuilder(); + JUnitCore.runClasses(ErrorCollectorTest.class); + assertEquals("starting failed finished ", log.toString()); + } + } }