Update junit-runner to version 1.0.9 and test new experimental runner…

… logic

- Bumps junit-runner to version 1.0.9 to include new experimental runner logic
  - Bump removal version of `--default-parallel` to 1.3.0 (since this never made it into 1.0.0 and the deprecation cycle is to last 2 minor versions)
  - Don't pass -default-parallel down to the runner any more
  - Adds a test to insure that the deprecated `--default-parallel` option still works.
  - The old "ParallelMethods" Junit tests were really testing the "ParallelClassesAndMethods" logic.  Renamed them.
  - Created new "ParallelMethods" Junit tests both in testprojects and the Junit runner mock tests.
  - Updated the concurrency integration test to run both with the legacy and experimental runner.
  - Added caveat about using test sharding with the PARALLEL_METHODS
  - Cleanup comments that referenced the old -parallel-methods and deprecated -default-parallel option

Testing Done:
CI green at https://travis-ci.org/pantsbuild/pants/builds/134707560

Bugs closed: 3493

Reviewed at https://rbcommons.com/s/twitter/r/3925/

src/java/org/pantsbuild/tools/jar/JarBuilder.java

@@ -370,7 +370,7 @@ private static Source memorySource() {
   }
 
   /**
-   * Input stream that always insures that a non-empty stream ends with a newline.
+   * Input stream that always ensures that a non-empty stream ends with a newline.
    */
   private static class NewlineAppendingInputStream extends InputStream {
     private InputStream underlyingStream;

src/python/pants/backend/jvm/targets/java_tests.py

@@ -21,11 +21,11 @@ class JavaTests(JvmTarget):
   CONCURRENCY_SERIAL = 'SERIAL'
   CONCURRENCY_PARALLEL_CLASSES = 'PARALLEL_CLASSES'
   CONCURRENCY_PARALLEL_METHODS = 'PARALLEL_METHODS'
-  CONCURRENCY_PARALLEL_BOTH = 'PARALLEL_BOTH'
+  CONCURRENCY_PARALLEL_CLASSES_AND_METHODS = 'PARALLEL_CLASSES_AND_METHODS'
   VALID_CONCURRENCY_OPTS = [CONCURRENCY_SERIAL,
                             CONCURRENCY_PARALLEL_CLASSES,
                             CONCURRENCY_PARALLEL_METHODS,
-                            CONCURRENCY_PARALLEL_BOTH]
+                            CONCURRENCY_PARALLEL_CLASSES_AND_METHODS]
 
   def __init__(self, cwd=None, test_platform=None, payload=None, timeout=None,
                extra_jvm_options=None, extra_env_vars=None, concurrency=None,
@@ -43,8 +43,7 @@ def __init__(self, cwd=None, test_platform=None, payload=None, timeout=None,
     :param dict extra_env_vars: A map of environment variables to set when running the tests, e.g.
       { 'FOOBAR': 12 }. Using `None` as the value will cause the variable to be unset.
     :param string concurrency: One of 'SERIAL', 'PARALLEL_CLASSES', 'PARALLEL_METHODS',
-      or 'PARALLEL_BOTH'.  Overrides the setting of --test-junit-default-parallel or
-      --test-junit-parallel-methods options.
+      or 'PARALLEL_CLASSES_AND_METHODS'.  Overrides the setting of --test-junit-default-concurrency.
     :param int threads: Use the specified number of threads when running the test. Overrides
       the setting of --test-junit-parallel-threads.
     """

src/python/pants/backend/jvm/tasks/junit_run.py

@@ -80,7 +80,7 @@ def register_options(cls, register):
              help='Set the default concurrency mode for running tests not annotated with'
                   ' @TestParallel or @TestSerial.')
     register('--default-parallel', advanced=True, type=bool,
-             removal_hint='Use --concurrency instead.', removal_version='1.1.0',
+             removal_hint='Use --default-concurrency instead.', removal_version='1.3.0',
              help='Run classes without @TestParallel or @TestSerial annotations in parallel.')
     register('--parallel-threads', advanced=True, type=int, default=0,
              help='Number of threads to run tests in parallel. 0 for autoset.')
@@ -110,7 +110,7 @@ def register_options(cls, register):
     cls.register_jvm_tool(register,
                           'junit',
                           classpath=[
-                            JarDependency(org='org.pantsbuild', name='junit-runner', rev='1.0.7'),
+                            JarDependency(org='org.pantsbuild', name='junit-runner', rev='1.0.9'),
                           ],
                           main=JUnitRun._MAIN,
                           # TODO(John Sirois): Investigate how much less we can get away with.
@@ -186,25 +186,36 @@ def __init__(self, *args, **kwargs):
     if options.per_test_timer:
       self._args.append('-per-test-timer')
 
-    # TODO(zundel): Simply remove when --default_parallel finishes deprecation
     if options.default_parallel:
-      self._args.append('-default-parallel')
-
-    if options.default_concurrency == junit_tests.CONCURRENCY_PARALLEL_BOTH:
-      self.context.log.warn('--default-concurrency=PARALLEL_BOTH is experimental.')
-      self._args.append('-default-concurrency')
-      self._args.append('PARALLEL_BOTH')
-    elif options.default_concurrency == junit_tests.CONCURRENCY_PARALLEL_CLASSES:
+      # TODO(zundel): Remove when --default_parallel finishes deprecation
+      if options.default_concurrency != junit_tests.CONCURRENCY_SERIAL:
+        self.context.log.warn('--default-parallel overrides --default-concurrency')
       self._args.append('-default-concurrency')
       self._args.append('PARALLEL_CLASSES')
-    elif options.default_concurrency == junit_tests.CONCURRENCY_PARALLEL_METHODS:
-      self.context.log.warn('--default-concurrency=PARALLEL_METHODS is experimental.')
-      self._args.append('-default-concurrency')
-      self._args.append('PARALLEL_METHODS')
-    elif options.default_concurrency == junit_tests.CONCURRENCY_SERIAL:
-      # TODO(zundel): we can't do anything here yet while the --default-parallel
-      # option is in deprecation mode.
-      pass
+    else:
+      if options.default_concurrency == junit_tests.CONCURRENCY_PARALLEL_CLASSES_AND_METHODS:
+        if not options.use_experimental_runner:
+          self.context.log.warn(
+            '--default-concurrency=PARALLEL_CLASSES_AND_METHODS is experimental, use --use-experimental-runner.')
+        self._args.append('-default-concurrency')
+        self._args.append('PARALLEL_CLASSES_AND_METHODS')
+      elif options.default_concurrency == junit_tests.CONCURRENCY_PARALLEL_METHODS:
+        if not options.use_experimental_runner:
+          self.context.log.warn(
+            '--default-concurrency=PARALLEL_METHODS is experimental, use --use-experimental-runner.')
+        if options.test_shard:
+          # NB(zundel): The experimental junit runner doesn't support test sharding natively.  The
+          # legacy junit runner allows both methods and classes to run in parallel with this option.
+          self.context.log.warn(
+            '--default-concurrency=PARALLEL_METHODS with test sharding will run classes in parallel too.')
+        self._args.append('-default-concurrency')
+        self._args.append('PARALLEL_METHODS')
+      elif options.default_concurrency == junit_tests.CONCURRENCY_PARALLEL_CLASSES:
+        self._args.append('-default-concurrency')
+        self._args.append('PARALLEL_CLASSES')
+      elif options.default_concurrency == junit_tests.CONCURRENCY_SERIAL:
+        self._args.append('-default-concurrency')
+        self._args.append('SERIAL')
 
     self._args.append('-parallel-threads')
     self._args.append(str(options.parallel_threads))
@@ -214,6 +225,7 @@ def __init__(self, *args, **kwargs):
       self._args.append(options.test_shard)
 
     if options.use_experimental_runner:
+      self.context.log.info('Using experimental junit-runner logic.')
       self._args.append('-use-experimental-runner')
 
   def classpath(self, targets, classpath_product=None):
@@ -382,17 +394,17 @@ def _run_tests(self, tests_to_targets):
         # Override cmdline args with values from junit_test() target that specify concurrency:
         args = self._args + [u'-xmlreport']
 
-        # TODO(zundel): Combine these together into a single -concurrency choices style argument
-        if concurrency == junit_tests.CONCURRENCY_SERIAL:
+        if concurrency is not None:
           args = remove_arg(args, '-default-parallel')
-        elif concurrency == junit_tests.CONCURRENCY_PARALLEL_CLASSES:
-          args = ensure_arg(args, '-default-parallel')
-        elif concurrency == junit_tests.CONCURRENCY_PARALLEL_METHODS:
-          self.context.log.warn('Not implemented: parallel_methods')
-        elif concurrency == junit_tests.CONCURRENCY_PARALLEL_BOTH:
-          self.context.log.warn('specifying {} is experimental.'.format(concurrency))
-          args = ensure_arg(args, '-default-parallel')
-          args = ensure_arg(args, '-parallel-methods')
+          if concurrency == junit_tests.CONCURRENCY_SERIAL:
+            args = ensure_arg(args, '-default-concurrency', param='SERIAL')
+          elif concurrency == junit_tests.CONCURRENCY_PARALLEL_CLASSES:
+            args = ensure_arg(args, '-default-concurrency', param='PARALLEL_CLASSES')
+          elif concurrency == junit_tests.CONCURRENCY_PARALLEL_METHODS:
+            args = ensure_arg(args, '-default-concurrency', param='PARALLEL_METHODS')
+          elif concurrency == junit_tests.CONCURRENCY_PARALLEL_CLASSES_AND_METHODS:
+            args = ensure_arg(args, '-default-concurrency', param='PARALLEL_CLASSES_AND_METHODS')
+
         if threads is not None:
           args = remove_arg(args, '-parallel-threads', has_param=True)
           args += ['-parallel-threads', str(threads)]

testprojects/tests/java/org/pantsbuild/testproject/matcher/MatcherTest.java

@@ -13,7 +13,7 @@
 import static org.junit.Assert.assertTrue;
 
 /**
- * This test insures that the junit and hamcrest classes work for a junit test.
+ * This test ensures that the junit and hamcrest classes work for a junit test.
  */
 public class MatcherTest {
 

testprojects/tests/java/org/pantsbuild/testproject/parallel/BUILD

@@ -33,7 +33,7 @@ junit_tests(name='annotated-parallel',
 
 # Even though these tests are run with 'parallel_classes' concurrency, they are annotated
 # with @TestSerial, so they should run serially, even when even when
-# --test-junit-default-concurrency={PARALLEL_CLASSES, PARALLEL_METHODS, PARALLEL_BOTH} is set
+# --test-junit-default-concurrency={PARALLEL_CLASSES, PARALLEL_METHODS, PARALLEL_CLASSES_AND_METHODS} is set
 # See: https://github.com/pantsbuild/pants/issues/3209
 junit_tests(name='annotated-serial',
   sources=globs('AnnotatedSerialTest*.java'),

testprojects/tests/java/org/pantsbuild/testproject/parallelclassesandmethods/BUILD

@@ -0,0 +1,21 @@
+# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+junit_tests(name='parallelclassesandmethods',
+  sources=globs('*.java'),
+  dependencies=[
+    '3rdparty:junit',
+  ],
+  concurrency='PARALLEL_CLASSES_AND_METHODS',
+  threads=4,
+)
+
+# This target runs the same tests as the one above, but doesn't have the concurrency settings.
+# Relies on the test.junit options being set as follows:
+#   --test-junit-default-concurrency=PARALLEL_CLASSES_AND_METHODS --test-junit-parallel-threads=4
+junit_tests(name='cmdline',
+  sources=globs('*.java'),
+  dependencies=[
+    '3rdparty:junit',
+  ],
+)

testprojects/tests/java/org/pantsbuild/testproject/parallelclassesandmethods/ParallelClassesAndMethodsDefaultParallelTest1.java

@@ -0,0 +1,50 @@
+// Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+// Licensed under the Apache License, Version 2.0 (see LICENSE).
+package org.pantsbuild.testproject.parallelclassesandmethods;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.junit.Test;
+
+import static org.junit.Assert.assertTrue;
+
+/**
+ * This test is designed to exercise the test.junit task argument:
+ * --test-junit-default-concurrency=PARALLEL_CLASSES_AND_METHODS
+ * <P>
+ * There is a similar test under tests/java/ to test the junit-runner standalone.
+ * <p>
+ * For all methods in ParallelClassesAndMethodsDefaultParallelTest1 and
+ * ParallelClassesAndMethodsDefaultParallelTest2 to succeed all of the test methods must be
+ * running at the same time. Also, both classes must be running at the same time.
+ * Intended to test the flags:
+ * <pre>
+ * --test-junit-default-concurrency=PARALLEL_CLASSES_AND_METHODS--test-junit-parallel-threads=4
+ * <pre>
+ * when running with just these two classes as specs.
+ * <p>
+ * Runs in on the order of 10 milliseconds locally, but it may take longer on a CI machine to spin
+ * up 4 threads, so it has a generous timeout set.
+ */
+public class ParallelClassesAndMethodsDefaultParallelTest1 {
+  private static final int NUM_CONCURRENT_TESTS = 4;
+  private static final int RETRY_TIMEOUT_MS = 3000;
+  private static CountDownLatch latch = new CountDownLatch(NUM_CONCURRENT_TESTS);
+
+  @Test
+  public void pbdptest11() throws Exception {
+    awaitLatch("pbdptest11");
+  }
+
+  @Test
+  public void pbdptest12() throws Exception {
+    awaitLatch("pbdptest12");
+  }
+
+  static void awaitLatch(String methodName) throws Exception {
+    System.out.println("start " + methodName);
+    latch.countDown();
+    assertTrue(latch.await(RETRY_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+    System.out.println("end " + methodName);
+  }
+}

testprojects/tests/java/org/pantsbuild/testproject/parallelclassesandmethods/ParallelClassesAndMethodsDefaultParallelTest2.java

@@ -0,0 +1,21 @@
+// Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+// Licensed under the Apache License, Version 2.0 (see LICENSE).
+package org.pantsbuild.testproject.parallelclassesandmethods;
+
+import org.junit.Test;
+
+/**
+ * See {@link ParallelClassesAndMethodsDefaultParallelTest1}
+ */
+public class ParallelClassesAndMethodsDefaultParallelTest2 {
+
+  @Test
+  public void pbdptest21() throws Exception {
+    ParallelClassesAndMethodsDefaultParallelTest1.awaitLatch("pbdptest21");
+  }
+
+  @Test
+  public void pbdptest22() throws Exception {
+    ParallelClassesAndMethodsDefaultParallelTest1.awaitLatch("pbdptest22");
+  }
+}

testprojects/tests/java/org/pantsbuild/testproject/parallelmethods/BUILD

@@ -2,19 +2,19 @@
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
 junit_tests(name='parallelmethods',
-  sources=globs('ParallelMethodsDefaultParallel*.java'),
+  sources=globs('*.java'),
   dependencies=[
     '3rdparty:junit',
   ],
-  concurrency='PARALLEL_BOTH',
+  concurrency='PARALLEL_METHODS',
   threads=4,
 )
 
 # This target runs the same tests as the one above, but doesn't have the concurrency settings.
 # Relies on the test.junit options being set as follows:
-#   --test-junit-default-concurrency=PARALLEL_BOTH --test-junit-parallel-threads=4
+#   --test-junit-default-concurrency=PARALLEL_METHODS --test-junit-parallel-threads=4
 junit_tests(name='cmdline',
-  sources=globs('ParallelMethodsDefaultParallel*.java'),
+  sources=globs('*.java'),
   dependencies=[
     '3rdparty:junit',
   ],

testprojects/tests/java/org/pantsbuild/testproject/parallelmethods/ParallelMethodsDefaultParallelTest1.java

@@ -4,6 +4,7 @@
 
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.junit.Test;
 
 import static org.junit.Assert.assertTrue;
@@ -15,7 +16,8 @@
  * There is a similar test under tests/java/ to test the junit-runner standalone.
  * <p>
  * For all methods in ParallelMethodsDefaultParallelTest1 and ParallelMethodsDefaultParallelTest2
- * to succeed all of the test methods must be running at the same time. Intended to test the flags:
+ * to succeed all of the test methods in each class must be running at the same time. But both
+ * classes should not run in parallel.  Intended to test the flags:
  * <pre>
  * --test-junit-default-concurrency=PARALLEL_METHODS --test-junit-parallel-threads=4
  * <pre>
@@ -25,9 +27,10 @@
  * up 4 threads, so it has a generous timeout set.
  */
 public class ParallelMethodsDefaultParallelTest1 {
-  private static final int NUM_CONCURRENT_TESTS = 4;
-  private static final int RETRY_TIMEOUT_MS = 3000;
+  private static final int NUM_CONCURRENT_TESTS = 2;
+  private static final int WAIT_TIMEOUT_MS = 1000;
   private static CountDownLatch latch = new CountDownLatch(NUM_CONCURRENT_TESTS);
+  private static AtomicInteger numRunning = new AtomicInteger(0);
 
   @Test
   public void pmdptest11() throws Exception {
@@ -40,9 +43,21 @@ public void pmdptest12() throws Exception {
   }
 
   static void awaitLatch(String methodName) throws Exception {
+    // NB(zundel): this test currently doesn't ensure that both classes run all methods in
+    // parallel, it only es that at least two methods get started and that no more than
+    // 2 methods run at a time. A better test would show that methods are run in parallel
+    // in each class.
+
     System.out.println("start " + methodName);
     latch.countDown();
-    assertTrue(latch.await(RETRY_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+    // Make sure that we wait for at least 2 methods to get started to ensure there is some
+    // parallelism going on.
+    assertTrue(latch.await(WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+    numRunning.incrementAndGet();
+    Thread.sleep(WAIT_TIMEOUT_MS);
+    // Make sure no more than 2 tests have been started concurrently
+    assertTrue(numRunning.get() <= NUM_CONCURRENT_TESTS);
+    numRunning.decrementAndGet();
     System.out.println("end " + methodName);
   }
 }

tests/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerTest.java

@@ -196,12 +196,20 @@ public void testConcurrencyParallelClasses() throws Exception {
   }
 
   @Test
-  public void testConcurrencyParallelClassesAndMethods() throws Exception {
+  public void testConcurrencyParallelMethods() throws Exception {
     invokeConsoleRunner("ParallelMethodsDefaultParallelTest1 ParallelMethodsDefaultParallelTest2"
-            + " -default-concurrency PARALLEL_CLASSES_AND_METHODS -parallel-threads 4");
+        + " -default-concurrency PARALLEL_METHODS -parallel-threads 4");
     assertEquals("pmdptest11 pmdptest12 pmdptest21 pmdptest22", TestRegistry.getCalledTests());
   }
 
+  @Test
+  public void testConcurrencyParallelClassesAndMethods() throws Exception {
+    invokeConsoleRunner("ParallelClassesAndMethodsDefaultParallelTest1"
+        + " ParallelClassesAndMethodsDefaultParallelTest2"
+        + " -default-concurrency PARALLEL_CLASSES_AND_METHODS -parallel-threads 4");
+    assertEquals("pbdptest11 pbdptest12 pbdptest21 pbdptest22", TestRegistry.getCalledTests());
+  }
+
   @Test
   public void testConcurrencySerial() throws Exception {
     invokeConsoleRunner("SerialTest1 SerialTest2"

tests/java/org/pantsbuild/tools/junit/lib/AnnotatedSerialTest1.java

@@ -20,7 +20,7 @@
  * To properly exercise this function, both test classes must be running at the same time with
  * the flags:
  * <pre>
- *  -default-parallel -parallel-threads 2
+ *  -default-concurrency PARALLEL_CLASSES -parallel-threads 2
  * </pre>
  * when running with just these two classes as specs.
  * <p>