Merge pull request #22 from Nordstrom/pr/add-listener-sync

sbabcoc · web-flow · commit 5c5749e72e48 · 2018-11-14T11:11:37.000-08:00
Fix ServiceLoader threading issues
diff --git a/src/main/java/com/nordstrom/automation/junit/CreateTestClass.java b/src/main/java/com/nordstrom/automation/junit/CreateTestClass.java
@@ -23,17 +23,10 @@
  */
 @SuppressWarnings("squid:S1118")
 public class CreateTestClass {
-    private static final ServiceLoader<TestClassWatcher> classWatcherLoader;
-    private static final ServiceLoader<TestClassWatcher2> classWatcher2Loader;
     private static final Logger LOGGER = LoggerFactory.getLogger(CreateTestClass.class);
     private static final Map<TestClass, Object> TESTCLASS_TO_RUNNER = new ConcurrentHashMap<>();
     private static final Map<Object, TestClass> METHOD_TO_TESTCLASS = new ConcurrentHashMap<>();
     
-    static {
-        classWatcherLoader = ServiceLoader.load(TestClassWatcher.class);
-        classWatcher2Loader = ServiceLoader.load(TestClassWatcher2.class);
-    }
-    
     /**
      * Interceptor for the {@link org.junit.runners.ParentRunner#createTestClass createTestClass} method.
      * 
@@ -52,10 +45,10 @@ public static TestClass intercept(@This final Object runner, @SuperCall final Ca
             METHOD_TO_TESTCLASS.put(method, testClass);
         }
         
-        for (TestClassWatcher watcher : classWatcherLoader) {
+        for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
             watcher.testClassCreated(testClass, runner);
         }
-        for (TestClassWatcher2 watcher : classWatcher2Loader) {
+        for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
             watcher.testClassCreated(testClass, runner);
         }
         
@@ -93,10 +86,10 @@ private static RunnerScheduler createRunnerScheduler(final TestClass testClass,
             
             public void schedule(Runnable childStatement) {
                 if (scheduled.compareAndSet(false, true)) {
-                    for (TestClassWatcher watcher : classWatcherLoader) {
+                    for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
                         watcher.testClassStarted(testClass);
                     }
-                    for (TestClassWatcher2 watcher : classWatcher2Loader) {
+                    for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
                         watcher.testClassStarted(testClass, runner);
                     }
                 }
@@ -113,10 +106,10 @@ public void schedule(Runnable childStatement) {
             }
 
             public void finished() {
-                for (TestClassWatcher watcher : classWatcherLoader) {
+                for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
                     watcher.testClassFinished(testClass);
                 }
-                for (TestClassWatcher2 watcher : classWatcher2Loader) {
+                for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
                     watcher.testClassFinished(testClass, runner);
                 }
             }
diff --git a/src/main/java/com/nordstrom/automation/junit/LifecycleHooks.java b/src/main/java/com/nordstrom/automation/junit/LifecycleHooks.java
@@ -7,13 +7,14 @@
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.ServiceLoader;
 import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArraySet;
+
 import org.junit.Test;
 import org.junit.runner.Description;
 import org.junit.runner.notification.RunListener;
@@ -125,16 +126,9 @@ static synchronized JUnitConfig getConfig() {
      */
     @SuppressWarnings("squid:S1118")
     public static class Run {
-        private static final ServiceLoader<RunListener> runListenerLoader;
-        private static final ServiceLoader<RunnerWatcher> runnerWatcherLoader;
-        private static final Set<RunNotifier> NOTIFIERS = new HashSet<>();
+        private static final Set<RunNotifier> NOTIFIERS = new CopyOnWriteArraySet<>();
         private static final Map<Object, Object> CHILD_TO_PARENT = new ConcurrentHashMap<>();
         
-        static {
-            runListenerLoader = ServiceLoader.load(RunListener.class);
-            runnerWatcherLoader = ServiceLoader.load(RunnerWatcher.class);
-        }
-        
         /**
          * Interceptor for the {@link org.junit.runners.ParentRunner#run run} method.
          * 
@@ -153,19 +147,19 @@ public static void intercept(@This final Object runner, @SuperCall final Callabl
             
             if (NOTIFIERS.add(notifier)) {
                 Description description = invoke(runner, "getDescription");
-                for (RunListener listener : runListenerLoader) {
+                for (RunListener listener : ServiceLoader.load(RunListener.class)) {
                     notifier.addListener(listener);
                     listener.testRunStarted(description);
                 }
             }
             
-            for (RunnerWatcher watcher : runnerWatcherLoader) {
+            for (RunnerWatcher watcher : ServiceLoader.load(RunnerWatcher.class)) {
                 watcher.runStarted(runner);
             }
             
             callProxy(proxy);
             
-            for (RunnerWatcher watcher : runnerWatcherLoader) {
+            for (RunnerWatcher watcher : ServiceLoader.load(RunnerWatcher.class)) {
                 watcher.runFinished(runner);
             }
         }
@@ -188,13 +182,8 @@ static Object getParentOf(Object child) {
     @SuppressWarnings("squid:S1118")
     public static class CreateTest {
         
-        private static final ServiceLoader<TestObjectWatcher> objectWatcherLoader;
         private static final Map<Object, TestClass> TARGET_TO_TESTCLASS = new ConcurrentHashMap<>();
         
-        static {
-            objectWatcherLoader = ServiceLoader.load(TestObjectWatcher.class);
-        }
-        
         /**
          * Interceptor for the {@link org.junit.runners.BlockJUnit4ClassRunner#createTest createTest} method.
          * 
@@ -210,7 +199,7 @@ public static Object intercept(@This final Object runner,
             TARGET_TO_TESTCLASS.put(testObj, getTestClassOf(runner));
             applyTimeout(testObj);
             
-            for (TestObjectWatcher watcher : objectWatcherLoader) {
+            for (TestObjectWatcher watcher : ServiceLoader.load(TestObjectWatcher.class)) {
                 watcher.testObjectCreated(testObj, TARGET_TO_TESTCLASS.get(testObj));
             }
             
diff --git a/src/main/java/com/nordstrom/automation/junit/RetryHandler.java b/src/main/java/com/nordstrom/automation/junit/RetryHandler.java
@@ -21,13 +21,8 @@
  */
 public class RetryHandler {
 
-    private static final ServiceLoader<JUnitRetryAnalyzer> retryAnalyzerLoader;
     private static final Logger LOGGER = LoggerFactory.getLogger(RetryHandler.class);
     
-    static {
-        retryAnalyzerLoader = ServiceLoader.load(JUnitRetryAnalyzer.class);
-    }
-    
     private RetryHandler() {
         throw new AssertionError("RetryHandler is a static utility class that cannot be instantiated");
     }
@@ -129,7 +124,7 @@ static int getMaxRetry(Object runner, final FrameworkMethod method) {
      * @return {@code true} if test should be retried; otherwise {@code false}
      */
     static boolean isRetriable(final FrameworkMethod method, final Throwable thrown) {
-        for (JUnitRetryAnalyzer analyzer : retryAnalyzerLoader) {
+        for (JUnitRetryAnalyzer analyzer : ServiceLoader.load(JUnitRetryAnalyzer.class)) {
             if (analyzer.retry(method, thrown)) {
                 return true;
             }
diff --git a/src/main/java/com/nordstrom/automation/junit/RunReflectiveCall.java b/src/main/java/com/nordstrom/automation/junit/RunReflectiveCall.java
@@ -27,17 +27,9 @@
 @SuppressWarnings("squid:S1118")
 public class RunReflectiveCall {
     
-    private static final ServiceLoader<MethodWatcher> methodWatcherLoader;
-    private static final ServiceLoader<RunWatcher> runWatcherLoader;
-    
     private static final Map<FrameworkMethod, Object> METHOD_TO_TARGET = new ConcurrentHashMap<>();
     private static final Map<TestClass, AtomicTest> TESTCLASS_TO_ATOMICTEST = new ConcurrentHashMap<>();
   
-    static {
-        methodWatcherLoader = ServiceLoader.load(MethodWatcher.class);
-        runWatcherLoader = ServiceLoader.load(RunWatcher.class);
-    }
-    
     /**
      * Interceptor for the {@link org.junit.internal.runners.model.ReflectiveCallable#runReflectiveCall
      * runReflectiveCall} method.
@@ -77,7 +69,7 @@ public static Object intercept(@This final Object callable, @SuperCall final Cal
 
         Object result = null;
         Throwable thrown = null;
-        for (MethodWatcher watcher : methodWatcherLoader) {
+        for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
             watcher.beforeInvocation(target, method, params);
         }
 
@@ -86,7 +78,7 @@ public static Object intercept(@This final Object callable, @SuperCall final Cal
         } catch (Throwable t) {
             thrown = t;
         } finally {
-            for (MethodWatcher watcher : methodWatcherLoader) {
+            for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
                 watcher.afterInvocation(target, method, thrown);
             }
         }
@@ -108,7 +100,7 @@ public static Object intercept(@This final Object callable, @SuperCall final Cal
     static void fireTestStarted(TestClass testClass, Runnable runnable) {
         AtomicTest atomicTest = createAtomicTest(testClass, runnable);
         if (atomicTest != null) {
-            for (RunWatcher watcher : runWatcherLoader) {
+            for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
                 watcher.testStarted(atomicTest.getIdentity(), atomicTest.getTestClass());
             }
         }
@@ -122,7 +114,7 @@ static void fireTestStarted(TestClass testClass, Runnable runnable) {
     static void fireTestFinished(TestClass testClass) {
         AtomicTest atomicTest = TESTCLASS_TO_ATOMICTEST.get(testClass);
         if (atomicTest != null) {
-            for (RunWatcher watcher : runWatcherLoader) {
+            for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
                 notifyIfTestFailed(watcher, atomicTest);
                 watcher.testFinished(atomicTest.getIdentity(), atomicTest.getTestClass());
             }
@@ -155,7 +147,7 @@ private static void notifyIfTestFailed(RunWatcher watcher, AtomicTest atomicTest
      */
     static void fireTestIgnored(Object runner, FrameworkMethod method) {
         TestClass testClass = getTestClassOf(runner);
-        for (RunWatcher watcher : runWatcherLoader) {
+        for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
             watcher.testIgnored(method, testClass);
         }
     }
@@ -183,7 +175,7 @@ public static Object getTargetFor(FrameworkMethod method) {
     public static Optional<MethodWatcher> getAttachedWatcher(
                     Class<? extends MethodWatcher> watcherType) {
         Objects.requireNonNull(watcherType, "[watcherType] must be non-null");
-        for (MethodWatcher watcher : methodWatcherLoader) {
+        for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
             if (watcher.getClass() == watcherType) {
                 return Optional.of(watcher);
             }

Original file line number	Diff line number	Diff line change
`@@ -23,17 +23,10 @@`
`23`	`23`	`*/`
`24`	`24`	`@SuppressWarnings("squid:S1118")`
`25`	`25`	`public class CreateTestClass {`
`26`		`- private static final ServiceLoader<TestClassWatcher> classWatcherLoader;`
`27`		`- private static final ServiceLoader<TestClassWatcher2> classWatcher2Loader;`
`28`	`26`	`private static final Logger LOGGER = LoggerFactory.getLogger(CreateTestClass.class);`
`29`	`27`	`private static final Map<TestClass, Object> TESTCLASS_TO_RUNNER = new ConcurrentHashMap<>();`
`30`	`28`	`private static final Map<Object, TestClass> METHOD_TO_TESTCLASS = new ConcurrentHashMap<>();`
`31`	`29`
`32`		`- static {`
`33`		`- classWatcherLoader = ServiceLoader.load(TestClassWatcher.class);`
`34`		`- classWatcher2Loader = ServiceLoader.load(TestClassWatcher2.class);`
`35`		`- }`
`36`		`-`
`37`	`30`	`/**`
`38`	`31`	`* Interceptor for the {@link org.junit.runners.ParentRunner#createTestClass createTestClass} method.`
`39`	`32`	`*`
`@@ -52,10 +45,10 @@ public static TestClass intercept(@This final Object runner, @SuperCall final Ca`
`52`	`45`	`METHOD_TO_TESTCLASS.put(method, testClass);`
`53`	`46`	`}`
`54`	`47`
`55`		`- for (TestClassWatcher watcher : classWatcherLoader) {`
	`48`	`+ for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {`
`56`	`49`	`watcher.testClassCreated(testClass, runner);`
`57`	`50`	`}`
`58`		`- for (TestClassWatcher2 watcher : classWatcher2Loader) {`
	`51`	`+ for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {`
`59`	`52`	`watcher.testClassCreated(testClass, runner);`
`60`	`53`	`}`
`61`	`54`
`@@ -93,10 +86,10 @@ private static RunnerScheduler createRunnerScheduler(final TestClass testClass,`
`93`	`86`
`94`	`87`	`public void schedule(Runnable childStatement) {`
`95`	`88`	`if (scheduled.compareAndSet(false, true)) {`
`96`		`- for (TestClassWatcher watcher : classWatcherLoader) {`
	`89`	`+ for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {`
`97`	`90`	`watcher.testClassStarted(testClass);`
`98`	`91`	`}`
`99`		`- for (TestClassWatcher2 watcher : classWatcher2Loader) {`
	`92`	`+ for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {`
`100`	`93`	`watcher.testClassStarted(testClass, runner);`
`101`	`94`	`}`
`102`	`95`	`}`
`@@ -113,10 +106,10 @@ public void schedule(Runnable childStatement) {`
`113`	`106`	`}`
`114`	`107`
`115`	`108`	`public void finished() {`
`116`		`- for (TestClassWatcher watcher : classWatcherLoader) {`
	`109`	`+ for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {`
`117`	`110`	`watcher.testClassFinished(testClass);`
`118`	`111`	`}`
`119`		`- for (TestClassWatcher2 watcher : classWatcher2Loader) {`
	`112`	`+ for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {`
`120`	`113`	`watcher.testClassFinished(testClass, runner);`
`121`	`114`	`}`
`122`	`115`	`}`