[junit-runner] extracted refactoring from sec mgr change (pantsbuild#…

…6860)

While working on pantsbuild#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 pantsbuild#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',