Skip to content

Commit f669c80

Browse files
sbabcocsbabcoc
committed
Eliminate use of static ServiceLoader collections and synchronization
1 parent 69a1892 commit f669c80

File tree

4 files changed

+37
-90
lines changed

4 files changed

+37
-90
lines changed

‎src/main/java/com/nordstrom/automation/junit/CreateTestClass.java‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,17 +23,10 @@
2323
*/
2424
@SuppressWarnings("squid:S1118")
2525
public class CreateTestClass {
26-
private static final ServiceLoader<TestClassWatcher> classWatcherLoader;
27-
private static final ServiceLoader<TestClassWatcher2> classWatcher2Loader;
2826
private static final Logger LOGGER = LoggerFactory.getLogger(CreateTestClass.class);
2927
private static final Map<TestClass, Object> TESTCLASS_TO_RUNNER = new ConcurrentHashMap<>();
3028
private static final Map<Object, TestClass> METHOD_TO_TESTCLASS = new ConcurrentHashMap<>();
3129

32-
static {
33-
classWatcherLoader = ServiceLoader.load(TestClassWatcher.class);
34-
classWatcher2Loader = ServiceLoader.load(TestClassWatcher2.class);
35-
}
36-
3730
/**
3831
* Interceptor for the {@link org.junit.runners.ParentRunner#createTestClass createTestClass} method.
3932
*
@@ -52,10 +45,10 @@ public static TestClass intercept(@This final Object runner, @SuperCall final Ca
5245
METHOD_TO_TESTCLASS.put(method, testClass);
5346
}
5447

55-
for (TestClassWatcher watcher : classWatcherLoader) {
48+
for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
5649
watcher.testClassCreated(testClass, runner);
5750
}
58-
for (TestClassWatcher2 watcher : classWatcher2Loader) {
51+
for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
5952
watcher.testClassCreated(testClass, runner);
6053
}
6154

@@ -93,10 +86,10 @@ private static RunnerScheduler createRunnerScheduler(final TestClass testClass,
9386

9487
public void schedule(Runnable childStatement) {
9588
if (scheduled.compareAndSet(false, true)) {
96-
for (TestClassWatcher watcher : classWatcherLoader) {
89+
for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
9790
watcher.testClassStarted(testClass);
9891
}
99-
for (TestClassWatcher2 watcher : classWatcher2Loader) {
92+
for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
10093
watcher.testClassStarted(testClass, runner);
10194
}
10295
}
@@ -113,10 +106,10 @@ public void schedule(Runnable childStatement) {
113106
}
114107

115108
public void finished() {
116-
for (TestClassWatcher watcher : classWatcherLoader) {
109+
for (TestClassWatcher watcher : ServiceLoader.load(TestClassWatcher.class)) {
117110
watcher.testClassFinished(testClass);
118111
}
119-
for (TestClassWatcher2 watcher : classWatcher2Loader) {
112+
for (TestClassWatcher2 watcher : ServiceLoader.load(TestClassWatcher2.class)) {
120113
watcher.testClassFinished(testClass, runner);
121114
}
122115
}

‎src/main/java/com/nordstrom/automation/junit/LifecycleHooks.java‎

Lines changed: 14 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@
77
import java.lang.reflect.Field;
88
import java.lang.reflect.InvocationTargetException;
99
import java.lang.reflect.Method;
10-
import java.util.HashSet;
1110
import java.util.List;
1211
import java.util.Map;
1312
import java.util.ServiceLoader;
1413
import java.util.Set;
1514
import java.util.concurrent.Callable;
1615
import java.util.concurrent.ConcurrentHashMap;
16+
import java.util.concurrent.CopyOnWriteArraySet;
17+
1718
import org.junit.Test;
1819
import org.junit.runner.Description;
1920
import org.junit.runner.notification.RunListener;
@@ -125,16 +126,9 @@ static synchronized JUnitConfig getConfig() {
125126
*/
126127
@SuppressWarnings("squid:S1118")
127128
public static class Run {
128-
private static final ServiceLoader<RunListener> runListenerLoader;
129-
private static final ServiceLoader<RunnerWatcher> runnerWatcherLoader;
130-
private static final Set<RunNotifier> NOTIFIERS = new HashSet<>();
129+
private static final Set<RunNotifier> NOTIFIERS = new CopyOnWriteArraySet<>();
131130
private static final Map<Object, Object> CHILD_TO_PARENT = new ConcurrentHashMap<>();
132131

133-
static {
134-
runListenerLoader = ServiceLoader.load(RunListener.class);
135-
runnerWatcherLoader = ServiceLoader.load(RunnerWatcher.class);
136-
}
137-
138132
/**
139133
* Interceptor for the {@link org.junit.runners.ParentRunner#run run} method.
140134
*
@@ -151,28 +145,22 @@ public static void intercept(@This final Object runner, @SuperCall final Callabl
151145
CHILD_TO_PARENT.put(child, runner);
152146
}
153147

154-
synchronized(NOTIFIERS) {
155-
if (NOTIFIERS.add(notifier)) {
156-
Description description = invoke(runner, "getDescription");
157-
for (RunListener listener : runListenerLoader) {
158-
notifier.addListener(listener);
159-
listener.testRunStarted(description);
160-
}
148+
if (NOTIFIERS.add(notifier)) {
149+
Description description = invoke(runner, "getDescription");
150+
for (RunListener listener : ServiceLoader.load(RunListener.class)) {
151+
notifier.addListener(listener);
152+
listener.testRunStarted(description);
161153
}
162154
}
163155

164-
synchronized(runnerWatcherLoader) {
165-
for (RunnerWatcher watcher : runnerWatcherLoader) {
166-
watcher.runStarted(runner);
167-
}
156+
for (RunnerWatcher watcher : ServiceLoader.load(RunnerWatcher.class)) {
157+
watcher.runStarted(runner);
168158
}
169159

170160
callProxy(proxy);
171161

172-
synchronized(runnerWatcherLoader) {
173-
for (RunnerWatcher watcher : runnerWatcherLoader) {
174-
watcher.runFinished(runner);
175-
}
162+
for (RunnerWatcher watcher : ServiceLoader.load(RunnerWatcher.class)) {
163+
watcher.runFinished(runner);
176164
}
177165
}
178166

@@ -194,13 +182,8 @@ static Object getParentOf(Object child) {
194182
@SuppressWarnings("squid:S1118")
195183
public static class CreateTest {
196184

197-
private static final ServiceLoader<TestObjectWatcher> objectWatcherLoader;
198185
private static final Map<Object, TestClass> TARGET_TO_TESTCLASS = new ConcurrentHashMap<>();
199186

200-
static {
201-
objectWatcherLoader = ServiceLoader.load(TestObjectWatcher.class);
202-
}
203-
204187
/**
205188
* Interceptor for the {@link org.junit.runners.BlockJUnit4ClassRunner#createTest createTest} method.
206189
*
@@ -216,10 +199,8 @@ public static Object intercept(@This final Object runner,
216199
TARGET_TO_TESTCLASS.put(testObj, getTestClassOf(runner));
217200
applyTimeout(testObj);
218201

219-
synchronized(objectWatcherLoader) {
220-
for (TestObjectWatcher watcher : objectWatcherLoader) {
221-
watcher.testObjectCreated(testObj, TARGET_TO_TESTCLASS.get(testObj));
222-
}
202+
for (TestObjectWatcher watcher : ServiceLoader.load(TestObjectWatcher.class)) {
203+
watcher.testObjectCreated(testObj, TARGET_TO_TESTCLASS.get(testObj));
223204
}
224205

225206
return testObj;

‎src/main/java/com/nordstrom/automation/junit/RetryHandler.java‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,8 @@
2121
*/
2222
public class RetryHandler {
2323

24-
private static final ServiceLoader<JUnitRetryAnalyzer> retryAnalyzerLoader;
2524
private static final Logger LOGGER = LoggerFactory.getLogger(RetryHandler.class);
2625

27-
static {
28-
retryAnalyzerLoader = ServiceLoader.load(JUnitRetryAnalyzer.class);
29-
}
30-
3126
private RetryHandler() {
3227
throw new AssertionError("RetryHandler is a static utility class that cannot be instantiated");
3328
}
@@ -129,11 +124,9 @@ static int getMaxRetry(Object runner, final FrameworkMethod method) {
129124
* @return {@code true} if test should be retried; otherwise {@code false}
130125
*/
131126
static boolean isRetriable(final FrameworkMethod method, final Throwable thrown) {
132-
synchronized(retryAnalyzerLoader) {
133-
for (JUnitRetryAnalyzer analyzer : retryAnalyzerLoader) {
134-
if (analyzer.retry(method, thrown)) {
135-
return true;
136-
}
127+
for (JUnitRetryAnalyzer analyzer : ServiceLoader.load(JUnitRetryAnalyzer.class)) {
128+
if (analyzer.retry(method, thrown)) {
129+
return true;
137130
}
138131
}
139132
return false;

‎src/main/java/com/nordstrom/automation/junit/RunReflectiveCall.java‎

Lines changed: 14 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -27,17 +27,9 @@
2727
@SuppressWarnings("squid:S1118")
2828
public class RunReflectiveCall {
2929

30-
private static final ServiceLoader<MethodWatcher> methodWatcherLoader;
31-
private static final ServiceLoader<RunWatcher> runWatcherLoader;
32-
3330
private static final Map<FrameworkMethod, Object> METHOD_TO_TARGET = new ConcurrentHashMap<>();
3431
private static final Map<TestClass, AtomicTest> TESTCLASS_TO_ATOMICTEST = new ConcurrentHashMap<>();
3532

36-
static {
37-
methodWatcherLoader = ServiceLoader.load(MethodWatcher.class);
38-
runWatcherLoader = ServiceLoader.load(RunWatcher.class);
39-
}
40-
4133
/**
4234
* Interceptor for the {@link org.junit.internal.runners.model.ReflectiveCallable#runReflectiveCall
4335
* runReflectiveCall} method.
@@ -77,21 +69,17 @@ public static Object intercept(@This final Object callable, @SuperCall final Cal
7769

7870
Object result = null;
7971
Throwable thrown = null;
80-
synchronized(methodWatcherLoader) {
81-
for (MethodWatcher watcher : methodWatcherLoader) {
82-
watcher.beforeInvocation(target, method, params);
83-
}
72+
for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
73+
watcher.beforeInvocation(target, method, params);
8474
}
8575

8676
try {
8777
result = LifecycleHooks.callProxy(proxy);
8878
} catch (Throwable t) {
8979
thrown = t;
9080
} finally {
91-
synchronized(methodWatcherLoader) {
92-
for (MethodWatcher watcher : methodWatcherLoader) {
93-
watcher.afterInvocation(target, method, thrown);
94-
}
81+
for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
82+
watcher.afterInvocation(target, method, thrown);
9583
}
9684
}
9785

@@ -112,10 +100,8 @@ public static Object intercept(@This final Object callable, @SuperCall final Cal
112100
static void fireTestStarted(TestClass testClass, Runnable runnable) {
113101
AtomicTest atomicTest = createAtomicTest(testClass, runnable);
114102
if (atomicTest != null) {
115-
synchronized(runWatcherLoader) {
116-
for (RunWatcher watcher : runWatcherLoader) {
117-
watcher.testStarted(atomicTest.getIdentity(), atomicTest.getTestClass());
118-
}
103+
for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
104+
watcher.testStarted(atomicTest.getIdentity(), atomicTest.getTestClass());
119105
}
120106
}
121107
}
@@ -128,11 +114,9 @@ static void fireTestStarted(TestClass testClass, Runnable runnable) {
128114
static void fireTestFinished(TestClass testClass) {
129115
AtomicTest atomicTest = TESTCLASS_TO_ATOMICTEST.get(testClass);
130116
if (atomicTest != null) {
131-
synchronized(runWatcherLoader) {
132-
for (RunWatcher watcher : runWatcherLoader) {
133-
notifyIfTestFailed(watcher, atomicTest);
134-
watcher.testFinished(atomicTest.getIdentity(), atomicTest.getTestClass());
135-
}
117+
for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
118+
notifyIfTestFailed(watcher, atomicTest);
119+
watcher.testFinished(atomicTest.getIdentity(), atomicTest.getTestClass());
136120
}
137121
}
138122
}
@@ -163,10 +147,8 @@ private static void notifyIfTestFailed(RunWatcher watcher, AtomicTest atomicTest
163147
*/
164148
static void fireTestIgnored(Object runner, FrameworkMethod method) {
165149
TestClass testClass = getTestClassOf(runner);
166-
synchronized(runWatcherLoader) {
167-
for (RunWatcher watcher : runWatcherLoader) {
168-
watcher.testIgnored(method, testClass);
169-
}
150+
for (RunWatcher watcher : ServiceLoader.load(RunWatcher.class)) {
151+
watcher.testIgnored(method, testClass);
170152
}
171153
}
172154

@@ -193,11 +175,9 @@ public static Object getTargetFor(FrameworkMethod method) {
193175
public static Optional<MethodWatcher> getAttachedWatcher(
194176
Class<? extends MethodWatcher> watcherType) {
195177
Objects.requireNonNull(watcherType, "[watcherType] must be non-null");
196-
synchronized(methodWatcherLoader) {
197-
for (MethodWatcher watcher : methodWatcherLoader) {
198-
if (watcher.getClass() == watcherType) {
199-
return Optional.of(watcher);
200-
}
178+
for (MethodWatcher watcher : ServiceLoader.load(MethodWatcher.class)) {
179+
if (watcher.getClass() == watcherType) {
180+
return Optional.of(watcher);
201181
}
202182
}
203183
return Optional.empty();

0 commit comments

Comments
 (0)