ConsoleRunner bugfix for @TestSerial and other test cleanups

This change started as just adding tests to show that parallelization is working correctly under Parameterized and BurstJUnit4 test runners, but ended up also finding a bug.

- Fix bug in ConsoleRunnerImpl
- Don't run parallel methods when test class is annotated with @TestSerial or @TestParallel
- Fix testing bug in all Concurrency related tests -  reset the concurrency parameters before each test permutation
- Fix testing bug in ParallelMethodsDefaultParallelTest1: only wait for 2 tests to start
- Fix testing bug that -default-concurrency flag was overwritten in ConsoleRunnerTestBase
- Add MockParameterizedTest and friends to show tests running parallel under the Parameterized test runner library
- Add MockBurstTest and friends to show tests running parallel under the Burst library
- Use assumeThat() to skip test permutations that won't work with specified tests

Testing Done:
Added several new unit tests

CI running at https://travis-ci.org/pantsbuild/pants/builds/140590946

Bugs closed: 3604

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

3rdparty/jvm/com/squareup/burst/BUILD

@@ -0,0 +1,8 @@
+# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+jar_library(name='burst-junit4',
+  jars=[
+    jar(org='com.squareup.burst', name='burst-junit4', rev='1.1.0'),
+  ],
+)

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

@@ -43,6 +43,8 @@
 import org.kohsuke.args4j.Option;
 import org.kohsuke.args4j.spi.StringArrayOptionHandler;
 import org.pantsbuild.args4j.InvalidCmdLineArgumentException;
+import org.pantsbuild.junit.annotations.TestParallel;
+import org.pantsbuild.junit.annotations.TestSerial;
 import org.pantsbuild.tools.junit.impl.experimental.ConcurrentComputer;
 import org.pantsbuild.tools.junit.withretry.AllDefaultPossibilitiesBuilderWithRetry;
 
@@ -568,6 +570,13 @@ private boolean legacyShouldRunParallelMethods(Class<?> clazz) {
       return false;
     }
 
+    // TestSerial and TestParallel take precedence over the default concurrency command
+    // line parameter
+    if (clazz.isAnnotationPresent(TestSerial.class)
+        || clazz.isAnnotationPresent(TestParallel.class)) {
+      return false;
+    }
+
     return this.defaultConcurrency.shouldRunMethodsParallel();
   }
 

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

@@ -10,14 +10,27 @@
 import org.hamcrest.CoreMatchers;
 import org.junit.Assert;
 import org.junit.Test;
+import org.pantsbuild.tools.junit.lib.AnnotatedParallelTest1;
+import org.pantsbuild.tools.junit.lib.AnnotatedSerialTest1;
 import org.pantsbuild.tools.junit.lib.ConsoleRunnerTestBase;
 import org.pantsbuild.tools.junit.lib.FlakyTest;
+import org.pantsbuild.tools.junit.lib.MockBurstParallelClassesAndMethodsTest1;
+import org.pantsbuild.tools.junit.lib.MockBurstParallelMethodsTest;
+import org.pantsbuild.tools.junit.lib.MockParameterizedParallelClassesAndMethodsTest1;
+import org.pantsbuild.tools.junit.lib.MockParameterizedParallelMethodsTest;
 import org.pantsbuild.tools.junit.lib.MockTest4;
+import org.pantsbuild.tools.junit.lib.ParallelClassesAndMethodsDefaultParallelTest1;
+import org.pantsbuild.tools.junit.lib.ParallelMethodsDefaultParallelTest1;
+import org.pantsbuild.tools.junit.lib.ParallelTest1;
+import org.pantsbuild.tools.junit.lib.SerialTest1;
 import org.pantsbuild.tools.junit.lib.TestRegistry;
 
+import static org.hamcrest.CoreMatchers.anyOf;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.core.Is.is;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeThat;
 
 /**
 /**
@@ -104,8 +117,8 @@ public void testShardedTesting34() {
 
   @Test
   public void testFlakyTests() {
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.SERIAL));
     FlakyTest.reset();
-
     try {
       invokeConsoleRunner("FlakyTest -num-retries 2 -default-concurrency SERIAL");
       fail("Should have failed with RuntimeException due to FlakyTest.methodAlwaysFails");
@@ -156,9 +169,11 @@ public void testConsoleOutput() {
 
   @Test
   public void testOutputDir() throws Exception {
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.PARALLEL_CLASSES));
+
     String outdir = temporary.newFolder("testOutputDir").getAbsolutePath();
-    invokeConsoleRunner("MockTest4 -parallel-threads 1 -default-concurrency PARALLEL_CLASSES"
-        + " -xmlreport -outdir " + outdir);
+    invokeConsoleRunner("MockTest4 -parallel-threads 1 -default-concurrency PARALLEL_CLASSES "
+        + "-xmlreport -outdir " + outdir);
     Assert.assertEquals("test41 test42", TestRegistry.getCalledTests());
 
     String testClassName = MockTest4.class.getCanonicalName();
@@ -170,40 +185,50 @@ public void testOutputDir() throws Exception {
 
   @Test
   public void testParallelAnnotation() throws Exception {
+    AnnotatedParallelTest1.reset();
     invokeConsoleRunner("AnnotatedParallelTest1 AnnotatedParallelTest2 -parallel-threads 2");
     assertEquals("aptest1 aptest2", TestRegistry.getCalledTests());
   }
 
   @Test
   public void testSerialAnnotation() throws Exception {
-    invokeConsoleRunner("AnnotatedSerialTest1 AnnotatedSerialTest2 "
-        + "-default-concurrency PARALLEL_CLASSES -parallel-threads 2");
+    AnnotatedSerialTest1.reset();
+    invokeConsoleRunner("AnnotatedSerialTest1 AnnotatedSerialTest2 -parallel-threads 2");
     assertEquals("astest1 astest2", TestRegistry.getCalledTests());
   }
 
   /* LEGACY, remove after -default-parallel argument is removed */
   @Test
   public void testParallelDefaultParallel() throws Exception {
+    ParallelTest1.reset();
     invokeConsoleRunner("ParallelTest1 ParallelTest2 -parallel-threads 2 -default-parallel");
     assertEquals("ptest1 ptest2", TestRegistry.getCalledTests());
   }
 
   @Test
   public void testConcurrencyParallelClasses() throws Exception {
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.PARALLEL_CLASSES));
+    ParallelTest1.reset();
     invokeConsoleRunner("ParallelTest1 ParallelTest2 "
         + "-parallel-threads 2 -default-concurrency PARALLEL_CLASSES");
     assertEquals("ptest1 ptest2", TestRegistry.getCalledTests());
   }
 
   @Test
   public void testConcurrencyParallelMethods() throws Exception {
+    // These tests only work with the experimental runner
+    assumeThat(parameters.useExperimentalRunner, is(true));
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.PARALLEL_METHODS));
+    ParallelMethodsDefaultParallelTest1.reset();
     invokeConsoleRunner("ParallelMethodsDefaultParallelTest1 ParallelMethodsDefaultParallelTest2"
         + " -default-concurrency PARALLEL_METHODS -parallel-threads 4");
     assertEquals("pmdptest11 pmdptest12 pmdptest21 pmdptest22", TestRegistry.getCalledTests());
   }
 
   @Test
   public void testConcurrencyParallelClassesAndMethods() throws Exception {
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.PARALLEL_CLASSES_AND_METHODS));
+    ParallelClassesAndMethodsDefaultParallelTest1.reset();
     invokeConsoleRunner("ParallelClassesAndMethodsDefaultParallelTest1"
         + " ParallelClassesAndMethodsDefaultParallelTest2"
         + " -default-concurrency PARALLEL_CLASSES_AND_METHODS -parallel-threads 4");
@@ -212,11 +237,74 @@ public void testConcurrencyParallelClassesAndMethods() throws Exception {
 
   @Test
   public void testConcurrencySerial() throws Exception {
-    invokeConsoleRunner("SerialTest1 SerialTest2"
-            + " -default-concurrency SERIAL -parallel-threads 4");
+    // This test only works when concurrency is serial
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.SERIAL));
+    SerialTest1.reset();
+    invokeConsoleRunner("SerialTest1 SerialTest2");
     assertEquals("stest1 stest2", TestRegistry.getCalledTests());
   }
 
+
+  @Test
+  public void testBurst() {
+    invokeConsoleRunner("MockBurstTest");
+    assertEquals("btest1:BOTTOM btest1:CHARM btest1:DOWN btest1:STRANGE btest1:TOP btest1:UP",
+        TestRegistry.getCalledTests());
+  }
+
+  @Test
+  public void testConcurrencyParameterizedParallelMethods() {
+    // These tests only work with the experimental runner
+    assumeThat(parameters.useExperimentalRunner, is(true));
+    // Requires parallel methods
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.PARALLEL_METHODS));
+    MockParameterizedParallelMethodsTest.reset();
+    invokeConsoleRunner("MockParameterizedParallelMethodsTest -parallel-threads 3");
+    assertEquals("ppmtest1:one ppmtest1:three ppmtest1:two", TestRegistry.getCalledTests());
+  }
+
+  @Test
+  public void testConcurrencyParameterizedParallelClassesAndMethods() {
+    // These tests only work with the experimental runner
+    assumeThat(parameters.useExperimentalRunner, is(true));
+    // Requires parallel methods
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.PARALLEL_CLASSES_AND_METHODS));
+    MockParameterizedParallelClassesAndMethodsTest1.reset();
+    invokeConsoleRunner("MockParameterizedParallelClassesAndMethodsTest1"
+        + " MockParameterizedParallelClassesAndMethodsTest2 -parallel-threads 5");
+    assertEquals("ppcamtest1:param1 ppcamtest1:param2 ppcamtest1:param3"
+            + " ppcamtest2:arg1 ppcamtest2:arg2",
+        TestRegistry.getCalledTests());
+  }
+
+  @Test
+  public void testConcurrencyBurstParallelMethods() {
+    // These tests only work with the experimental runner
+    assumeThat(parameters.useExperimentalRunner, is(true));
+    // Requires parallel methods
+    assumeThat(parameters.defaultConcurrency, anyOf(is(Concurrency.PARALLEL_METHODS),
+        is(Concurrency.PARALLEL_CLASSES_AND_METHODS)));
+    MockBurstParallelMethodsTest.reset();
+    invokeConsoleRunner("MockBurstParallelMethodsTest -parallel-threads 6");
+    assertEquals("bpmtest1:BOTTOM bpmtest1:CHARM bpmtest1:DOWN bpmtest1:STRANGE "
+        + "bpmtest1:TOP bpmtest1:UP",
+        TestRegistry.getCalledTests());
+  }
+
+  @Test
+  public void testConcurrencyBurstParallelClassesAndMethods() {
+    // These tests only work with the experimental runner
+    assumeThat(parameters.useExperimentalRunner, is(true));
+    // Requires parallel methods
+    assumeThat(parameters.defaultConcurrency, is(Concurrency.PARALLEL_CLASSES_AND_METHODS));
+    MockBurstParallelClassesAndMethodsTest1.reset();
+    invokeConsoleRunner("MockBurstParallelClassesAndMethodsTest1"
+        + " MockBurstParallelClassesAndMethodsTest2 -parallel-threads 5");
+    assertEquals("bpcamtest1:BLUE bpcamtest1:RED"
+        + " bpcamtest2:APPLE bpcamtest2:BANANA bpcamtest2:CHERRY",
+        TestRegistry.getCalledTests());
+  }
+
   @Test
   public void testMockJUnit3Test() throws Exception {
     invokeConsoleRunner("MockJUnit3Test");

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

@@ -32,6 +32,10 @@ public class AnnotatedParallelTest1 {
   private static final int RETRY_TIMEOUT_MS = 3000;
   private static CountDownLatch latch = new CountDownLatch(NUM_CONCURRENT_TESTS);
 
+  public static void reset() {
+    latch = new CountDownLatch(NUM_CONCURRENT_TESTS);
+  }
+
   @Test
   public void aptest1() throws Exception {
     awaitLatch("aptest1");

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

@@ -33,6 +33,10 @@ public class AnnotatedSerialTest1 {
   private static final int WAIT_TIMEOUT_MS = 1000;
   private static final AtomicBoolean waiting = new AtomicBoolean(false);
 
+  public static void reset() {
+    waiting.set(false);
+  }
+
   @Test
   public void astest1() throws Exception {
     awaitLatch("astest1");

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

@@ -4,6 +4,7 @@
 java_library(
   name='test-dep',
   dependencies=[
+    '3rdparty/jvm/com/squareup/burst:burst-junit4',
     '3rdparty:guava',
     '3rdparty:junit',
     '3rdparty/jvm/commons-io',

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

@@ -22,12 +22,13 @@
 @RunWith(Parameterized.class)
 public abstract class ConsoleRunnerTestBase {
   private static final String DEFAULT_CONCURRENCY_FLAG = "-default-concurrency";
+  private static final String DEFAULT_PARALLEL_FLAG = "-default-parallel";
   private static final String USE_EXPERIMENTAL_RUNNER_FLAG = "-use-experimental-runner";
   private static final String PARALLEL_THREADS_FLAG = "-parallel-threads";
 
   private static final String DEFAULT_TEST_PACKGE = "org.pantsbuild.tools.junit.lib.";
 
-  private TestParameters parameters;
+  protected TestParameters parameters;
 
   protected enum TestParameters {
     LEGACY_SERIAL(false, null),
@@ -107,8 +108,11 @@ protected void invokeConsoleRunner(String argsString) {
 
     // Tack on extra parameters from the Parameterized runner
     if (!testArgs.contains(DEFAULT_CONCURRENCY_FLAG) && parameters.defaultConcurrency != null) {
-      testArgs.add(DEFAULT_CONCURRENCY_FLAG);
-      testArgs.add(parameters.defaultConcurrency.name());
+      if (!testArgs.contains(DEFAULT_CONCURRENCY_FLAG)
+          && !testArgs.contains(DEFAULT_PARALLEL_FLAG)) {
+        testArgs.add(DEFAULT_CONCURRENCY_FLAG);
+        testArgs.add(parameters.defaultConcurrency.name());
+      }
       if (!testArgs.contains(PARALLEL_THREADS_FLAG)) {
         testArgs.add(PARALLEL_THREADS_FLAG);
         testArgs.add("8");

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

@@ -0,0 +1,43 @@
+// Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+// Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+package org.pantsbuild.tools.junit.lib;
+
+import com.squareup.burst.BurstJUnit4;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import static org.junit.Assert.assertTrue;
+
+@RunWith(BurstJUnit4.class)
+public class MockBurstParallelClassesAndMethodsTest1 {
+  private static final int NUM_CONCURRENT_TESTS = 5;
+  private static final int RETRY_TIMEOUT_MS = 1000;
+  private static CountDownLatch latch = new CountDownLatch(NUM_CONCURRENT_TESTS);
+
+  private final ColorType colorType;
+  public enum ColorType {
+    RED, BLUE
+  }
+
+  public static void reset() {
+    latch = new CountDownLatch(NUM_CONCURRENT_TESTS);
+  }
+
+  public MockBurstParallelClassesAndMethodsTest1(ColorType colorType) {
+    this.colorType = colorType;
+  }
+
+  @Test
+  public void bpcamtest1() throws Exception {
+    awaitLatch("bpcamtest1:" + colorType.name());
+  }
+
+  static void awaitLatch(String methodName) throws Exception {
+    TestRegistry.registerTestCall(methodName);
+    latch.countDown();
+    assertTrue(latch.await(RETRY_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+  }
+}

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

@@ -0,0 +1,25 @@
+// Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+// Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+package org.pantsbuild.tools.junit.lib;
+
+import com.squareup.burst.BurstJUnit4;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(BurstJUnit4.class)
+public class MockBurstParallelClassesAndMethodsTest2 {
+  private final FruitType fruitType;
+  public enum FruitType {
+    APPLE, BANANA, CHERRY
+  }
+
+  public MockBurstParallelClassesAndMethodsTest2(FruitType fruitType) {
+    this.fruitType = fruitType;
+  }
+
+  @Test
+  public void bpcamtest1() throws Exception {
+    MockBurstParallelClassesAndMethodsTest1.awaitLatch("bpcamtest2:" + fruitType.name());
+  }
+}

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

@@ -0,0 +1,43 @@
+// Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+// Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+package org.pantsbuild.tools.junit.lib;
+
+import com.squareup.burst.BurstJUnit4;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import static org.junit.Assert.assertTrue;
+
+@RunWith(BurstJUnit4.class)
+public class MockBurstParallelMethodsTest {
+  private static final int NUM_CONCURRENT_TESTS = 6;
+  private static final int RETRY_TIMEOUT_MS = 1000;
+  private static CountDownLatch latch = new CountDownLatch(NUM_CONCURRENT_TESTS);
+
+  private final QuarkType quarkType;
+  public enum QuarkType {
+    UP, DOWN, STRANGE, CHARM, TOP, BOTTOM
+  }
+
+  public static void reset() {
+    latch = new CountDownLatch(NUM_CONCURRENT_TESTS);
+  }
+
+  public MockBurstParallelMethodsTest(QuarkType quarkType) {
+    this.quarkType = quarkType;
+  }
+
+  @Test
+  public void bpmtest1() throws Exception {
+    awaitLatch("bpmtest1:" + quarkType.name());
+  }
+
+  static void awaitLatch(String methodName) throws Exception {
+    TestRegistry.registerTestCall(methodName);
+    latch.countDown();
+    assertTrue(latch.await(RETRY_TIMEOUT_MS, TimeUnit.MILLISECONDS));
+  }
+}