Update in response to code review commends by dsaff and Tibor17

Author: kcooney

Diff for: src/main/java/org/junit/runner/notification/RunListener.java

@@ -53,17 +53,17 @@
 public class RunListener {
 
     /**
-     * Called before any tests have been run. This may not necessarily
-     * be called by the same thread as started the test 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. This may not necessarily
-     * be called by the same thread as started the test run.
+     * 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
      */
@@ -96,9 +96,8 @@ public void testFinished(Description description) throws Exception {
      * {@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}. This call
-     * may or may not be called in the same thread as the failing listener was
-     * called
+     * 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
      */

Diff for: src/main/java/org/junit/runner/notification/RunNotifier.java

@@ -199,7 +199,7 @@ public void pleaseStop() {
      */
     public void addFirstListener(RunListener listener) {
         if (listener == null) {
-            throw new NullPointerException("Cannot remove a null listener");
+            throw new NullPointerException("Cannot add a null listener");
         }
         fListeners.add(0, SynchronizedRunListener.wrapIfNotThreadSafe(listener));
     }

Diff for: src/main/java/org/junit/runner/notification/SynchronizedRunListener.java

@@ -7,6 +7,12 @@
  * Thread-safe decorator for {@link RunListener} implementations that synchronizes
  * calls to the delegate.
  *
+ * <p>This class synchronizes all listener calls on a global monitor. 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

Diff for: src/test/java/org/junit/runner/notification/ConcurrentRunNotifierTest.java

@@ -3,7 +3,13 @@
 import org.junit.Test;
 import org.junit.runner.Description;
 
-import java.util.concurrent.*;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -58,16 +64,16 @@ public void run() {
     }
 
     private static class ExaminedListener extends RunListener {
-        volatile boolean useMe = false;
+        final boolean throwFromTestStarted;
         volatile boolean hasTestFailure = false;
 
-        ExaminedListener(boolean useMe) {
-            this.useMe = useMe;
+        ExaminedListener(boolean throwFromTestStarted) {
+            this.throwFromTestStarted = throwFromTestStarted;
         }
 
         @Override
         public void testStarted(Description description) throws Exception {
-            if (!useMe) {
+            if (!throwFromTestStarted) {
                 throw new Exception();
             }
         }
@@ -78,94 +84,88 @@ public void testFailure(Failure failure) throws Exception {
         }
     }
 
-    @Test
-    public void reportConcurrentFailuresAfterAddListener() throws Exception {
-        int totalListenersFailures = 0;
+    private abstract class AbstractConcurrentFailuresTest {
 
-        final ExaminedListener[] examinedListeners = new ExaminedListener[1000];
-        for (int i = 0; i < examinedListeners.length; ++i) {
-            boolean fail = StrictMath.random() >= 0.5d;
-            if (fail) {
-                ++totalListenersFailures;
-            }
-            examinedListeners[i] = new ExaminedListener(!fail);
-        }
+        protected abstract void addListener(ExaminedListener listener);
 
-        final CyclicBarrier trigger = new CyclicBarrier(2);
-        final AtomicBoolean condition = new AtomicBoolean(true);
+        public void test() throws Exception {
+            int totalListenersFailures = 0;
 
-        ExecutorService notificationsPool = Executors.newFixedThreadPool(4);
-        notificationsPool.submit(new Callable<Void>() {
-            public Void call() throws Exception {
-                trigger.await();
-                while (condition.get()) {
-                    notifier.fireTestStarted(null);
+            Random random = new Random(42);
+            ExaminedListener[] examinedListeners = new ExaminedListener[1000];
+            for (int i = 0; i < examinedListeners.length; ++i) {
+                boolean fail = random.nextDouble() >= 0.5d;
+                if (fail) {
+                    ++totalListenersFailures;
                 }
-                notifier.fireTestStarted(null);
-                return null;
+                examinedListeners[i] = new ExaminedListener(!fail);
             }
-        });
-
-        trigger.await();
-
-        for (ExaminedListener examinedListener : examinedListeners) {
-            notifier.addListener(examinedListener);
-        }
 
-        notificationsPool.shutdown();
-        condition.set(false);
-        assertTrue(notificationsPool.awaitTermination(TIMEOUT, TimeUnit.SECONDS));
+            final AtomicBoolean condition = new AtomicBoolean(true);
+            final CyclicBarrier trigger = new CyclicBarrier(2);
+            final CountDownLatch latch = new CountDownLatch(10);
+
+            ExecutorService notificationsPool = Executors.newFixedThreadPool(4);
+            notificationsPool.submit(new Callable<Void>() {
+                public Void call() throws Exception {
+                    trigger.await();
+                    while (condition.get()) {
+                        notifier.fireTestStarted(null);
+                        latch.countDown();
+                    }
+                    notifier.fireTestStarted(null);
+                    return null;
+                }
+            });
 
-        if (totalListenersFailures != 0) {
-            // If no listener failures, then all the listeners do not report any failure.
-            int countTestFailures = examinedListeners.length - countReportedTestFailures(examinedListeners);
-            assertThat(totalListenersFailures, is(countTestFailures));
-        }
-    }
+            // Wait for callable to start
+            trigger.await(TIMEOUT, TimeUnit.SECONDS);
 
-    @Test
-    public void reportConcurrentFailuresAfterAddFirstListener() throws Exception {
-        int totalListenersFailures = 0;
+            // Wait for callable to fire a few events
+            latch.await(TIMEOUT, TimeUnit.SECONDS);
 
-        final ExaminedListener[] examinedListeners = new ExaminedListener[1000];
-        for (int i = 0; i < examinedListeners.length; ++i) {
-            boolean fail = StrictMath.random() >= 0.5d;
-            if (fail) {
-                ++totalListenersFailures;
+            for (ExaminedListener examinedListener : examinedListeners) {
+              addListener(examinedListener);
             }
-            examinedListeners[i] = new ExaminedListener(!fail);
-        }
 
-        final CyclicBarrier trigger = new CyclicBarrier(2);
-        final AtomicBoolean condition = new AtomicBoolean(true);
+            notificationsPool.shutdown();
+            condition.set(false);
+            assertTrue(notificationsPool.awaitTermination(TIMEOUT, TimeUnit.SECONDS));
 
-        ExecutorService notificationsPool = Executors.newFixedThreadPool(4);
-        notificationsPool.submit(new Callable<Void>() {
-            public Void call() throws Exception {
-                trigger.await();
-                while (condition.get()) {
-                    notifier.fireTestStarted(null);
-                }
-                notifier.fireTestStarted(null);
-                return null;
+            if (totalListenersFailures != 0) {
+                // If no listener failures, then all the listeners do not report any failure.
+                int countTestFailures = examinedListeners.length - countReportedTestFailures(examinedListeners);
+                assertThat(totalListenersFailures, is(countTestFailures));
             }
-        });
-
-        trigger.await();
-
-        for (ExaminedListener examinedListener : examinedListeners) {
-            notifier.addFirstListener(examinedListener);
         }
+    }
 
-        notificationsPool.shutdown();
-        condition.set(false);
-        assertTrue(notificationsPool.awaitTermination(TIMEOUT, TimeUnit.SECONDS));
+    /**
+     * Verifies that listeners added while tests are run concurrently are
+     * notified about test failures.
+     */
+    @Test
+    public void reportConcurrentFailuresAfterAddListener() throws Exception {
+        new AbstractConcurrentFailuresTest() {
+            @Override
+            protected void addListener(ExaminedListener listener) {
+                notifier.addListener(listener);
+            }
+        }.test();
+    }
 
-        if (totalListenersFailures != 0) {
-            // If no listener failures, then all the listeners do not report any failure.
-            int countTestFailures = examinedListeners.length - countReportedTestFailures(examinedListeners);
-            assertThat(totalListenersFailures, is(countTestFailures));
-        }
+    /**
+     * Verifies that listeners added with addFirstListener() while tests are run concurrently are
+     * notified about test failures.
+     */
+    @Test
+    public void reportConcurrentFailuresAfterAddFirstListener() throws Exception {
+        new AbstractConcurrentFailuresTest() {
+            @Override
+            protected void addListener(ExaminedListener listener) {
+                notifier.addFirstListener(listener);
+            }
+        }.test();
     }
 
     private static int countReportedTestFailures(ExaminedListener[] listeners) {

Diff for: src/test/java/org/junit/runner/notification/SynchronizedRunListenerTest.java

@@ -29,9 +29,9 @@ private static class MethodSignature {
         private final List<Class<?>> parameterTypes;
 
         public MethodSignature(Method method) {
-            this.method= method;
-            name= method.getName();
-            parameterTypes= Arrays.asList(method.getParameterTypes());
+            this.method = method;
+            name = method.getName();
+            parameterTypes = Arrays.asList(method.getParameterTypes());
         }
 
         @Override
@@ -67,8 +67,8 @@ private Set<MethodSignature> getAllDeclaredMethods(Class<?> type) {
 
     @Test
     public void overridesAllMethodsInRunListener() {
-        Set<MethodSignature> runListenerMethods= getAllDeclaredMethods(RunListener.class);
-        Set<MethodSignature> synchronizedRunListenerMethods= getAllDeclaredMethods(
+        Set<MethodSignature> runListenerMethods = getAllDeclaredMethods(RunListener.class);
+        Set<MethodSignature> synchronizedRunListenerMethods = getAllDeclaredMethods(
                 SynchronizedRunListener.class);
 
         assertTrue(synchronizedRunListenerMethods.containsAll(runListenerMethods));
@@ -78,7 +78,7 @@ private static class NamedListener extends RunListener {
         private final String name;
 
         public NamedListener(String name) {
-            this.name= name;
+            this.name = name;
         }
 
         @Override
@@ -94,7 +94,7 @@ public boolean equals(Object obj) {
             if (!(obj instanceof NamedListener)) {
                 return false;
             }
-            NamedListener that= (NamedListener) obj;
+            NamedListener that = (NamedListener) obj;
             return this.name.equals(that.name);
         }
     }
@@ -149,7 +149,7 @@ public void hashCodeDelegates() {
 
     @Test
     public void wrapIfNotThreadSafeShouldNotWrapThreadSafeListeners() {
-        ThreadSafeRunListener listener= new ThreadSafeRunListener();;
+        ThreadSafeRunListener listener = new ThreadSafeRunListener();;
         assertSame(listener, SynchronizedRunListener.wrapIfNotThreadSafe(listener));
     }
 
@@ -160,8 +160,8 @@ public void wrapIfNotThreadSafeShouldNotWrapThreadSafeListeners() {
      */
     @Test
     public void wrapIfNotThreadSafeShouldWrapNonThreadSafeListeners() {
-        NamedListener listener= new NamedListener("name");
-        RunListener wrappedListener= SynchronizedRunListener.wrapIfNotThreadSafe(listener);
+        NamedListener listener = new NamedListener("name");
+        RunListener wrappedListener = SynchronizedRunListener.wrapIfNotThreadSafe(listener);
         assertThat(wrappedListener, instanceOf(SynchronizedRunListener.class));
     }
 }