[junit-runner] extracted refactoring from sec mgr change (#6860)

While working on #6203, I moved some things around that were not directly related to that change. These are those bits.

This flattens some nested ifs and adds some helper fns to create places that #6203 can extend from.

src/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImpl.java

@@ -40,6 +40,7 @@
 import org.junit.runner.notification.RunListener;
 import org.junit.runner.notification.RunNotifier;
 import org.junit.runners.model.InitializationError;
+import org.junit.runners.model.RunnerBuilder;
 import org.kohsuke.args4j.Argument;
 import org.kohsuke.args4j.CmdLineException;
 import org.kohsuke.args4j.CmdLineParser;
@@ -518,8 +519,7 @@ private int runConcurrentTests(JUnitCore core, SpecSet specSet, Concurrency conc
       throws InitializationError {
     Computer junitComputer = new ConcurrentComputer(concurrency, parallelThreads);
     Class<?>[] classes = specSet.extract(concurrency).classes();
-    CustomAnnotationBuilder builder =
-        new CustomAnnotationBuilder(numRetries, swappableErr.getOriginal());
+    RunnerBuilder builder = createCustomBuilder(swappableErr.getOriginal());
     Runner suite = junitComputer.getSuite(builder, classes);
     return core.run(Request.runner(suite)).getFailureCount();
   }
@@ -533,26 +533,32 @@ private int runLegacy(Collection<Spec> parsedTests, JUnitCore core) throws Initi
     if (this.parallelThreads > 1) {
       ConcurrentCompositeRequestRunner concurrentRunner = new ConcurrentCompositeRequestRunner(
           requests, this.defaultConcurrency, this.parallelThreads);
-      if (failFast) {
-        return core.run(new FailFastRunner(concurrentRunner)).getFailureCount();
-      } else {
-        return core.run(concurrentRunner).getFailureCount();
-      }
+      Runner runner = maybeWithFailFastRunner(concurrentRunner);
+      return core.run(runner).getFailureCount();
     }
 
     int failures = 0;
     Result result;
     for (Request request : requests) {
-      if (failFast) {
-        result = core.run(new FailFastRunner(request.getRunner()));
-      } else {
-        result = core.run(request);
-      }
+      result = core.run(runnerFor(request));
       failures += result.getFailureCount();
     }
     return failures;
   }
 
+  private Runner runnerFor(Request request) {
+    Runner reqRunner = request.getRunner();
+    return maybeWithFailFastRunner(reqRunner);
+  }
+
+  private Runner maybeWithFailFastRunner(Runner runner) {
+    if (failFast) {
+      return new FailFastRunner(runner);
+    } else {
+      return runner;
+    }
+  }
+
   private List<Request> legacyParseRequests(PrintStream err, Collection<Spec> specs) {
     Set<TestMethod> testMethods = Sets.newLinkedHashSet();
     Set<Class<?>> classes = Sets.newLinkedHashSet();
@@ -570,24 +576,19 @@ private List<Request> legacyParseRequests(PrintStream err, Collection<Spec> spec
     if (!classes.isEmpty()) {
       if (this.perTestTimer || this.parallelThreads > 1) {
         for (Class<?> clazz : classes) {
-          if (legacyShouldRunParallelMethods(clazz)) {
-            if (ScalaTestUtil.isScalaTestTest(clazz)) {
-              // legacy and scala doesn't work easily.  just adding the class
-              requests.add(new AnnotatedClassRequest(clazz, numRetries, err));
-            } else {
-              testMethods.addAll(TestMethod.fromClass(clazz));
-            }
+          // legacy doesn't support scala test tests, so those are run as classes.
+          if (legacyShouldRunParallelMethods(clazz) && !ScalaTestUtil.isScalaTestTest(clazz)) {
+            testMethods.addAll(TestMethod.fromClass(clazz));
           } else {
-            requests.add(new AnnotatedClassRequest(clazz, numRetries, err));
+            requests.add(createAnnotatedClassRequest(err, clazz));
           }
         }
       } else {
         // The code below does what the original call
         // requests.add(Request.classes(classes.toArray(new Class<?>[classes.size()])));
         // does, except that it instantiates our own builder, needed to support retries.
         try {
-          CustomAnnotationBuilder builder =
-              new CustomAnnotationBuilder(numRetries, err);
+          RunnerBuilder builder = createCustomBuilder(err);
           Runner suite = new Computer().getSuite(
               builder, classes.toArray(new Class<?>[classes.size()]));
           requests.add(Request.runner(suite));
@@ -598,12 +599,20 @@ private List<Request> legacyParseRequests(PrintStream err, Collection<Spec> spec
       }
     }
     for (TestMethod testMethod : testMethods) {
-      requests.add(new AnnotatedClassRequest(testMethod.clazz, numRetries, err)
+      requests.add(createAnnotatedClassRequest(err, testMethod.clazz)
           .filterWith(Description.createTestDescription(testMethod.clazz, testMethod.name)));
     }
     return requests;
   }
 
+  private AnnotatedClassRequest createAnnotatedClassRequest(PrintStream err, Class<?> clazz) {
+    return new AnnotatedClassRequest(clazz, numRetries, err);
+  }
+
+  private RunnerBuilder createCustomBuilder(PrintStream original) {
+    return new CustomAnnotationBuilder(numRetries, original);
+  }
+
   private boolean legacyShouldRunParallelMethods(Class<?> clazz) {
     if (!Util.isRunnable(clazz)) {
       return false;
@@ -799,10 +808,7 @@ public void setNumRetries(int numRetries) {
     CmdLineParser parser = new CmdLineParser(options);
     try {
       parser.parseArgument(args);
-    } catch (CmdLineException e) {
-      parser.printUsage(System.err);
-      exit(1);
-    } catch (InvalidCmdLineArgumentException e) {
+    } catch (CmdLineException | InvalidCmdLineArgumentException e) {
       parser.printUsage(System.err);
       exit(1);
     }

src/java/org/pantsbuild/tools/junit/impl/SpecParser.java

@@ -75,16 +75,20 @@ Collection<Spec> parse() throws SpecException {
    * @throws SpecException if the method passed in is not an executable test method
    */
   private Optional<Spec> getOrCreateSpec(String className, String specString) throws SpecException {
-    try {
-      Class<?> clazz = getClass().getClassLoader().loadClass(className);
-      if (Util.isTestClass(clazz)) {
-        if (!specs.containsKey(clazz)) {
-          Spec newSpec = new Spec(clazz);
-          specs.put(clazz, newSpec);
-        }
-        return Optional.of(specs.get(clazz));
+    Class<?> clazz = loadClassOrThrow(className, specString);
+    if (Util.isTestClass(clazz)) {
+      if (!specs.containsKey(clazz)) {
+        Spec newSpec = new Spec(clazz);
+        specs.put(clazz, newSpec);
       }
-      return Optional.absent();
+      return Optional.of(specs.get(clazz));
+    }
+    return Optional.absent();
+  }
+
+  private Class<?> loadClassOrThrow(final String className, String specString) {
+    try {
+      return getClass().getClassLoader().loadClass(className);
     } catch (ClassNotFoundException | NoClassDefFoundError e) {
       throw new SpecException(specString,
           String.format("Class %s not found in classpath.", className), e);
@@ -94,9 +98,10 @@ private Optional<Spec> getOrCreateSpec(String className, String specString) thro
       throw new SpecException(specString,
           String.format("Error linking %s.", className), e);
       // See the comment below for justification.
-    } catch (RuntimeException e) {
+    } catch (Exception e) {
       // The class may fail with some variant of RTE in its static initializers, trap these
-      // and dump the bad spec in question to help narrow down issue.
+      // and dump the bad spec in question along with the underlying error message to help
+      // narrow down issue.
       throw new SpecException(specString,
           String.format("Error initializing %s.",className), e);
     }

tests/java/org/pantsbuild/tools/junit/impl/BUILD

@@ -2,7 +2,6 @@
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
 junit_tests(
-  name='junit',
   dependencies=[
     '3rdparty:guava',
     '3rdparty:junit',