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.

Author: stefanbirkner

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 @@
  *     }
  * }
  * </pre>
+ * <p>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
  */

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());
+        }
+    }
 }