[TestNG] Don't fail if TestNG runs in parallel (#2591)

* [TestNG] Don't fail if TestNG runs in parallel

* Add comment / trigger test

* bump up timeouts in JobLimiterTest

* frivolous change for retest

src/com/facebook/buck/testrunner/BaseRunner.java

@@ -28,6 +28,7 @@
 import java.io.StringWriter;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.List;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
@@ -82,7 +83,7 @@ private static String removeCRIfNeeded(String text) {
    * The test result file is written as XML to avoid introducing a dependency on JSON (see class
    * overview).
    */
-  protected void writeResult(String testClassName, List<TestResult> results)
+  protected void writeResult(String testClassName, Collection<TestResult> results)
       throws IOException, ParserConfigurationException, TransformerException {
     // XML writer logic taken from:
     // http://www.genedavis.com/library/xml/java_dom_xml_creation.jsp

src/com/facebook/buck/testrunner/TestNGRunner.java

@@ -25,12 +25,12 @@
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
-import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import org.testng.IAnnotationTransformer;
@@ -51,7 +51,7 @@
 import org.testng.reporters.SuiteHTMLReporter;
 import org.testng.reporters.XMLReporter;
 
-/** Class that runs a set of TestNG tests and writes the results to a directory. */
+/** Class that runs a set of TestNG tests and outputs the results to a directory. */
 public final class TestNGRunner extends BaseRunner {
 
   @Override
@@ -60,11 +60,13 @@ public void run() throws Throwable {
 
       Class<?> testClass = Class.forName(className);
 
-      List<TestResult> results;
+      Collection<TestResult> results;
       if (!mightBeATestClass(testClass)) {
         results = Collections.emptyList();
       } else {
-        results = new ArrayList<>();
+        // TestNG can run a test class's tests in parallel via DataProvider.
+        // Use concurrency-safe collection to avoid comodification errors. 
+        results = new ConcurrentLinkedQueue<>();
         TestNG testng = new TestNG();
         testng.setUseDefaultListeners(false);
         testng.addListener(new FilteringAnnotationTransformer(results));
@@ -155,9 +157,9 @@ private static String getTestMethodNameWithParameters(ITestResult iTestResult) {
   }
 
   public class FilteringAnnotationTransformer implements IAnnotationTransformer {
-    final List<TestResult> results;
+    final Collection<TestResult> results;
 
-    FilteringAnnotationTransformer(List<TestResult> results) {
+    FilteringAnnotationTransformer(Collection<TestResult> results) {
       this.results = results;
     }
 
@@ -199,13 +201,13 @@ public void transform(
   }
 
   private static class TestListener implements ITestListener, IConfigurationListener {
-    private final List<TestResult> results;
+    private final Collection<TestResult> results;
     private boolean mustRestoreStdoutAndStderr;
     private PrintStream originalOut, originalErr, stdOutStream, stdErrStream;
     private ByteArrayOutputStream rawStdOutBytes, rawStdErrBytes;
     private Map<IClass, Throwable> failedConfigurationTestClasses = new HashMap<>();
 
-    public TestListener(List<TestResult> results) {
+    public TestListener(Collection<TestResult> results) {
       this.results = results;
     }
 

test/com/facebook/buck/util/concurrent/JobLimiterTest.java

@@ -68,8 +68,8 @@ public void testLimitsJobs() throws Exception {
     }
 
     // Two jobs should start.
-    assertTrue(jobStarted.tryAcquire(100, TimeUnit.MILLISECONDS));
-    assertTrue(jobStarted.tryAcquire(100, TimeUnit.MILLISECONDS));
+    assertTrue(jobStarted.tryAcquire(200, TimeUnit.MILLISECONDS));
+    assertTrue(jobStarted.tryAcquire(200, TimeUnit.MILLISECONDS));
 
     assertEquals(2, jobsRunning.get());