Merge remote-tracking branch 'upstream/master'

Conflicts:
	src/main/java/org/junit/runner/notification/RunListener.java

Author: dlam

src/main/java/org/junit/runner/Result.java

@@ -65,6 +65,7 @@ public boolean wasSuccessful() {
         return getFailureCount() == 0;
     }
 
+    @RunListener.ThreadSafe
     private class Listener extends RunListener {
         @Override
         public void testRunStarted(Description description) throws Exception {

src/main/java/org/junit/runner/notification/RunListener.java

@@ -1,15 +1,22 @@
 package org.junit.runner.notification;
 
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
 import org.junit.internal.AssumptionViolatedException;
 import org.junit.runner.Description;
 import org.junit.runner.Result;
 
 /**
- * If you need to respond to the events during a test run, extend <code>RunListener</code>
- * and override the appropriate methods. If a listener throws an exception while processing a
- * test event, it will be removed for the remainder of the test run.
- * <p>
- * For example, suppose you have a <code>Cowbell</code>
+ * Register an instance of this class with {@link RunNotifier} to be notified
+ * of events that occur during a test run. All of the methods in this class
+ * are abstract and have no implementation; override one or more methods to
+ * receive events.
+ *
+ * <p>For example, suppose you have a <code>Cowbell</code>
  * class that you want to make a noise whenever a test fails. You could write:
  * <pre>
  * public class RingingListener extends RunListener {
@@ -18,8 +25,8 @@
  *    }
  * }
  * </pre>
- * <p>
- * To invoke your listener, you need to run your tests through <code>JUnitCore</code>.
+ *
+ * <p>To invoke your listener, you need to run your tests through <code>JUnitCore</code>.
  * <pre>
  * public void main(String... args) {
  *    JUnitCore core= new JUnitCore();
@@ -28,21 +35,35 @@
  * }
  * </pre>
  *
+ * <p>If a listener throws an exception for a test event, the other listeners will
+ * have their {@link RunListener#testFailure(Failure)} called with a {@code Description}
+ * of {@link Description#TEST_MECHANISM} to indicate the failure.
+ *
+ * <p>By default, JUnit will synchronize calls to your listener. If your listener
+ * is thread-safe and you want to allow JUnit to call your listener from
+ * multiple threads when tests are run in parallel, you can annotate your
+ * test class with {@link RunListener.ThreadSafe}.
+ *
+ * <p>Listener methods will be called from the same thread as is running
+ * the test, unless otherwise indicated by the method Javadoc
+ *
  * @see org.junit.runner.JUnitCore
  * @since 4.0
  */
 public class RunListener {
 
     /**
-     * Called before any tests have been run.
+     * Called before any tests have been run. This may be called on an
+     * arbitrary thread.
      *
      * @param description describes the tests to be run
      */
     public void testRunStarted(Description description) throws Exception {
     }
 
     /**
-     * Called when all tests have finished
+     * Called when all tests have finished. This may be called on an
+     * arbitrary thread.
      *
      * @param result the summary of the test run, including all the tests that failed
      */
@@ -67,7 +88,16 @@ public void testFinished(Description description) throws Exception {
     }
 
     /**
-     * Called when an atomic test fails.
+     * Called when an atomic test fails, or when a listener throws an exception.
+     *
+     * <p>In the case of a failure of an atomic test, this method will be called
+     * with the same {@code Description} passed to
+     * {@link #testStarted(Description)}, from the same thread that called
+     * {@link #testStarted(Description)}.
+     *
+     * <p>In the case of a listener throwing an exception, this will be called with
+     * a {@code Description} of {@link Description#TEST_MECHANISM}, and may be called
+     * on an arbitrary thread.
      *
      * @param failure describes the test that failed and the exception that was thrown
      */
@@ -92,6 +122,20 @@ public void testAssumptionFailure(Failure failure) {
      */
     public void testIgnored(Description description) throws Exception {
     }
-}
 
 
+    /**
+     * Indicates a {@code RunListener} that can have its methods called
+     * concurrently. This implies that the class is thread-safe (i.e. no set of
+     * listener calls can put the listener into an invalid state, even if those
+     * listener calls are being made by multiple threads without
+     * synchronization).
+     *
+     * @since 4.12
+     */
+    @Documented
+    @Target(ElementType.TYPE)
+    @Retention(RetentionPolicy.RUNTIME)
+    public @interface ThreadSafe {
+    }
+}

src/main/java/org/junit/runner/notification/RunNotifier.java

@@ -3,9 +3,9 @@
 import static java.util.Arrays.asList;
 
 import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Iterator;
+import java.util.Collection;
 import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import org.junit.internal.AssumptionViolatedException;
 import org.junit.runner.Description;
@@ -21,24 +21,39 @@
  * @since 4.0
  */
 public class RunNotifier {
-    private final List<RunListener> fListeners =
-            Collections.synchronizedList(new ArrayList<RunListener>());
+    private final List<RunListener> fListeners = new CopyOnWriteArrayList<RunListener>();
     private volatile boolean fPleaseStop = false;
 
     /**
      * Internal use only
      */
     public void addListener(RunListener listener) {
-        fListeners.add(listener);
+        if (listener == null) {
+            throw new NullPointerException("Cannot add a null listener");
+        }
+        fListeners.add(wrapIfNotThreadSafe(listener));
     }
 
     /**
      * Internal use only
      */
     public void removeListener(RunListener listener) {
-        fListeners.remove(listener);
+        if (listener == null) {
+            throw new NullPointerException("Cannot remove a null listener");
+        }
+        fListeners.remove(wrapIfNotThreadSafe(listener));
+    }
+
+    /**
+     * Wraps the given listener with {@link SynchronizedRunListener} if
+     * it is not annotated with {@link RunListener.ThreadSafe}.
+     */
+    RunListener wrapIfNotThreadSafe(RunListener listener) {
+        return listener.getClass().isAnnotationPresent(RunListener.ThreadSafe.class) ?
+                listener : new SynchronizedRunListener(listener, this);
     }
 
+
     private abstract class SafeNotifier {
         private final List<RunListener> fCurrentListeners;
 
@@ -51,21 +66,18 @@ private abstract class SafeNotifier {
         }
 
         void run() {
-            synchronized (fListeners) {
-                List<RunListener> safeListeners = new ArrayList<RunListener>();
-                List<Failure> failures = new ArrayList<Failure>();
-                for (Iterator<RunListener> all = fCurrentListeners.iterator(); all
-                        .hasNext(); ) {
-                    try {
-                        RunListener listener = all.next();
-                        notifyListener(listener);
-                        safeListeners.add(listener);
-                    } catch (Exception e) {
-                        failures.add(new Failure(Description.TEST_MECHANISM, e));
-                    }
+            int capacity = fCurrentListeners.size();
+            ArrayList<RunListener> safeListeners = new ArrayList<RunListener>(capacity);
+            ArrayList<Failure> failures = new ArrayList<Failure>(capacity);
+            for (RunListener listener : fCurrentListeners) {
+                try {
+                    notifyListener(listener);
+                    safeListeners.add(listener);
+                } catch (Exception e) {
+                    failures.add(new Failure(Description.TEST_MECHANISM, e));
                 }
-                fireTestFailures(safeListeners, failures);
             }
+            fireTestFailures(safeListeners, failures);
         }
 
         abstract protected void notifyListener(RunListener each) throws Exception;
@@ -80,8 +92,6 @@ public void fireTestRunStarted(final Description description) {
             protected void notifyListener(RunListener each) throws Exception {
                 each.testRunStarted(description);
             }
-
-            ;
         }.run();
     }
 
@@ -94,8 +104,6 @@ public void fireTestRunFinished(final Result result) {
             protected void notifyListener(RunListener each) throws Exception {
                 each.testRunFinished(result);
             }
-
-            ;
         }.run();
     }
 
@@ -114,8 +122,6 @@ public void fireTestStarted(final Description description) throws StoppedByUserE
             protected void notifyListener(RunListener each) throws Exception {
                 each.testStarted(description);
             }
-
-            ;
         }.run();
     }
 
@@ -133,14 +139,11 @@ private void fireTestFailures(List<RunListener> listeners,
         if (!failures.isEmpty()) {
             new SafeNotifier(listeners) {
                 @Override
-                protected void notifyListener(RunListener listener)
-                        throws Exception {
+                protected void notifyListener(RunListener listener) throws Exception {
                     for (Failure each : failures) {
                         listener.testFailure(each);
                     }
                 }
-
-                ;
             }.run();
         }
     }
@@ -158,8 +161,6 @@ public void fireTestAssumptionFailed(final Failure failure) {
             protected void notifyListener(RunListener each) throws Exception {
                 each.testAssumptionFailure(failure);
             }
-
-            ;
         }.run();
     }
 
@@ -190,8 +191,6 @@ public void fireTestFinished(final Description description) {
             protected void notifyListener(RunListener each) throws Exception {
                 each.testFinished(description);
             }
-
-            ;
         }.run();
     }
 
@@ -209,6 +208,9 @@ public void pleaseStop() {
      * Internal use only. The Result's listener must be first.
      */
     public void addFirstListener(RunListener listener) {
-        fListeners.add(0, listener);
+        if (listener == null) {
+            throw new NullPointerException("Cannot add a null listener");
+        }
+        fListeners.add(0, wrapIfNotThreadSafe(listener));
     }
-}
+}

src/main/java/org/junit/runner/notification/SynchronizedRunListener.java

@@ -0,0 +1,103 @@
+package org.junit.runner.notification;
+
+import org.junit.runner.Description;
+import org.junit.runner.Result;
+
+/**
+ * Thread-safe decorator for {@link RunListener} implementations that synchronizes
+ * calls to the delegate.
+ *
+ * <p>This class synchronizes all listener calls on a RunNotifier instance. This is done because
+ * prior to JUnit 4.12, all listeners were called in a synchronized block in RunNotifier,
+ * so no two listeners were ever called concurrently. If we instead made the methods here
+ * sychronized, clients that added multiple listeners that called common code might see
+ * issues due to the reduced synchronization.
+ *
+ * @author Tibor Digana (tibor17)
+ * @author Kevin Cooney (kcooney)
+ * @since 4.12
+ *
+ * @see RunNotifier
+ */
+@RunListener.ThreadSafe
+final class SynchronizedRunListener extends RunListener {
+    private final RunListener fListener;
+    private final Object fMonitor;
+
+    SynchronizedRunListener(RunListener listener, Object monitor) {
+        fListener = listener;
+        fMonitor = monitor;
+    }
+
+    @Override
+    public void testRunStarted(Description description) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testRunStarted(description);
+        }
+    }
+
+    @Override
+    public void testRunFinished(Result result) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testRunFinished(result);
+        }
+    }
+
+    @Override
+    public void testStarted(Description description) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testStarted(description);
+        }
+    }
+
+    @Override
+    public void testFinished(Description description) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testFinished(description);
+        }
+    }
+
+    @Override
+    public void testFailure(Failure failure) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testFailure(failure);
+        }
+    }
+
+    @Override
+    public void testAssumptionFailure(Failure failure) {
+        synchronized (fMonitor) {
+            fListener.testAssumptionFailure(failure);
+        }
+    }
+
+    @Override
+    public void testIgnored(Description description) throws Exception {
+        synchronized (fMonitor) {
+            fListener.testIgnored(description);
+        }
+    }
+
+    @Override
+    public int hashCode() {
+        return fListener.hashCode();
+    }
+
+    @Override
+    public boolean equals(Object other) {
+        if (this == other) {
+            return true;
+        }
+        if (!(other instanceof SynchronizedRunListener)) {
+            return false;
+        }
+        SynchronizedRunListener that = (SynchronizedRunListener) other;
+        
+        return fListener.equals(that.fListener);
+    }
+
+    @Override
+    public String toString() {
+        return fListener.toString() + " (with synchronization wrapper)";
+    }
+}