Ensure All tests run all the time

Closes #2841

Author: krmahadevan

testng-core/src/main/java/org/testng/SuiteRunner.java

@@ -347,6 +347,10 @@ private void privateRun() {
       // -- cbeust
       invoker = tr.getInvoker();
 
+      // Add back the configuration listeners that may have gotten altered after
+      // our suite level listeners were invoked.
+      this.configuration.getConfigurationListeners().forEach(tr::addConfigurationListener);
+
       for (ITestNGMethod m : tr.getBeforeSuiteMethods()) {
         beforeSuiteMethods.put(m.getConstructorOrMethod().getMethod(), m);
       }

testng-core/src/main/java/org/testng/TestRunner.java

@@ -10,7 +10,6 @@
 import java.util.Date;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -1160,14 +1159,12 @@ public void setVerbose(int n) {
   // TODO: This method needs to be removed and we need to be leveraging addListener().
   // Investigate and fix this.
   void addTestListener(ITestListener listener) {
-    Optional<ITestListener> found =
+    boolean found =
         m_testListeners.stream()
-            .filter(iTestListener -> iTestListener.getClass().equals(listener.getClass()))
-            .findAny();
-    if (found.isPresent()) {
-      return;
+            .anyMatch(iTestListener -> iTestListener.getClass().equals(listener.getClass()));
+    if (!found) {
+      m_testListeners.add(listener);
     }
-    m_testListeners.add(listener);
   }
 
   public void addListener(ITestNGListener listener) {
@@ -1214,7 +1211,11 @@ public void addListener(ITestNGListener listener) {
   }
 
   void addConfigurationListener(IConfigurationListener icl) {
-    m_configurationListeners.add(icl);
+    boolean alreadyAdded =
+        m_configurationListeners.stream().anyMatch(each -> each.getClass().equals(icl.getClass()));
+    if (!alreadyAdded) {
+      m_configurationListeners.add(icl);
+    }
   }
 
   private void dumpInvokedMethods() {

testng-core/src/main/java/org/testng/internal/ClassBasedWrapper.java

@@ -23,7 +23,7 @@ public boolean equals(Object o) {
     if (this == o) return true;
     if (o == null || getClass() != o.getClass()) return false;
     ClassBasedWrapper<?> wrapper = (ClassBasedWrapper<?>) o;
-    return object.getClass().equals(wrapper.getClass());
+    return object.getClass().equals(wrapper.object.getClass());
   }
 
   @Override

testng-core/src/main/java/org/testng/internal/MethodHelper.java

@@ -45,7 +45,7 @@ public class MethodHelper {
    * @param finder annotation finder
    * @param unique true for unique methods, false otherwise
    * @param outExcludedMethods - A List of excluded {@link ITestNGMethod} methods.
-   * @return list of ordered methods
+   * @return an array of ordered methods
    */
   public static ITestNGMethod[] collectAndOrderMethods(
       List<ITestNGMethod> methods,
@@ -82,7 +82,7 @@ public static ITestNGMethod[] collectAndOrderMethods(
    *
    * @param m TestNG method
    * @param methods list of methods to search for depended upon methods
-   * @return list of methods that match the criteria
+   * @return an array of methods that match the criteria
    */
   protected static ITestNGMethod[] findDependedUponMethods(
       ITestNGMethod m, List<ITestNGMethod> methods) {
@@ -95,18 +95,14 @@ protected static ITestNGMethod[] findDependedUponMethods(
    *
    * @param m TestNG method
    * @param incoming list of methods to search for depended upon methods
-   * @return list of methods that match the criteria
+   * @return an array of methods that match the criteria
    */
   public static ITestNGMethod[] findDependedUponMethods(ITestNGMethod m, ITestNGMethod[] incoming) {
     ITestNGMethod[] methods =
         Arrays.stream(incoming)
             .filter(each -> !each.equals(m))
             .filter(each -> Objects.isNull(each.getRealClass().getEnclosingClass()))
             .toArray(ITestNGMethod[]::new);
-    if (methods.length == 0) {
-      return new ITestNGMethod[] {};
-    }
-
     String canonicalMethodName = calculateMethodCanonicalName(m);
 
     List<ITestNGMethod> vResult = Lists.newArrayList();

testng-core/src/test/groovy/test/groovy/SpockSample.groovy

@@ -14,4 +14,4 @@ class SpockSample extends Specification {
         then:
         stack.size() == 1
     }
-}
+}

testng-core/src/test/java/test/InvokedMethodNameListener.java

@@ -58,7 +58,9 @@ public void beforeInvocation(IInvokedMethod method, ITestResult testResult) {
   @Override
   public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
     List<String> methodNames =
-        mapping.computeIfAbsent(testResult.getMethod().getRealClass(), k -> Lists.newArrayList());
+        mapping.computeIfAbsent(
+            testResult.getMethod().getRealClass(),
+            k -> Collections.synchronizedList(Lists.newArrayList()));
     methodNames.add(method.getTestMethod().getMethodName());
     String name = getName(testResult);
     switch (testResult.getStatus()) {

testng-core/src/test/java/test/dataprovider/DataProviderTest.java

@@ -78,11 +78,12 @@ public void testDataProviderCanBeRetriedViaAnnotationTransformer() {
 
   @Test(description = "GITHUB-2819")
   public void testDataProviderRetryInstancesAreUniqueForEachDataDrivenTest() {
+    SimpleRetry.clearObjectIds();
     TestNG testng = create(TestClassWithMultipleRetryImplSample.class);
     DataProviderListenerForRetryAwareTests listener = new DataProviderListenerForRetryAwareTests();
     testng.addListener(listener);
     testng.run();
-    assertThat(SimpleRetry.getHashCodes()).hasSize(2);
+    assertThat(SimpleRetry.getObjectIds()).hasSize(2);
     // Without retrying itself we would have already invoked the listener once.
     assertThat(listener.getBeforeInvocations()).isEqualTo(6);
     assertThat(listener.getFailureInvocations()).isEqualTo(4);

testng-core/src/test/java/test/dataprovider/issue2819/SimpleRetry.java

@@ -3,19 +3,24 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.UUID;
 import org.testng.IDataProviderMethod;
 import org.testng.IRetryDataProvider;
 
 public class SimpleRetry implements IRetryDataProvider {
 
-  private static final Set<Integer> hashCodes = new HashSet<>();
+  private static final Set<String> objectIds = new HashSet<>();
 
-  public static Set<Integer> getHashCodes() {
-    return Collections.unmodifiableSet(hashCodes);
+  public static Set<String> getObjectIds() {
+    return Collections.unmodifiableSet(objectIds);
+  }
+
+  public static void clearObjectIds() {
+    objectIds.clear();
   }
 
   public SimpleRetry() {
-    hashCodes.add(hashCode());
+    objectIds.add(UUID.randomUUID().toString());
   }
 
   private int counter = 0;

testng-core/src/test/java/test/dependent/DependentTest.java

@@ -64,10 +64,8 @@ public void ensureDependsOnMethodsHonoursRegexPatternsAcrossClassesErrorConditio
       description = "GITHUB-141",
       expectedExceptions = TestNGException.class,
       expectedExceptionsMessageRegExp =
-          "\nMethod \"test.dependent.issue141.ErrorScenarioNestedSample.a\\(\\)\" "
-              + "depends on nonexistent "
-              + "method \"test.dependent.issue141.ErrorScenarioNestedSample\\$InnerTestClass"
-              + ".rambo.*")
+          "\ntest.dependent.issue141.ErrorScenarioNestedSample.a\\(\\) "
+              + "depends on nonexistent method .*")
   public void ensureDependsOnMethodsHonoursRegexPatternsNestedClassesErrorCondition() {
     TestNG testng = create(ErrorScenarioNestedSample.class);
     MethodNameCollector listener = new MethodNameCollector();
@@ -101,9 +99,7 @@ public void ensureDependsOnMethodsHonoursRegexPatternsDuplicateMatches() {
   @Test(
       description = "GITHUB-141",
       expectedExceptions = TestNGException.class,
-      expectedExceptionsMessageRegExp =
-          "\nMethod \"test.dependent.issue141.NestedTestClassSample\\$FirstSample.randomTest\\(\\)\" "
-              + "depends on nonexistent method .*")
+      expectedExceptionsMessageRegExp = "\n.* depends on nonexistent method .*")
   public void ensureDependsOnMethodsHonoursRegexPatternsDuplicateMatchesNestedClasses() {
     TestNG testng = create(NestedTestClassSample.class);
     MethodNameCollector listener = new MethodNameCollector();
@@ -117,7 +113,7 @@ public void ensureDependsOnMethodsHonoursRegexPatternsDuplicateMatchesNestedClas
       description = "GITHUB-141",
       expectedExceptions = TestNGException.class,
       expectedExceptionsMessageRegExp =
-          "\nMethod \"test.dependent.issue141.NestedTestClassSample2.randomTest\\(\\)\" depends on "
+          "\ntest.dependent.issue141.NestedTestClassSample2.randomTest\\(\\) depends on "
               + "nonexistent method .*")
   public void ensureDependsOnMethodsHonourRegexPatternsNestedClasses() {
     TestNG testng = create(NestedTestClassSample2.class);

testng-core/src/test/java/test/listeners/github1130/GitHub1130Test.java

@@ -2,7 +2,6 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-import java.util.ArrayList;
 import org.testng.ITestNGListener;
 import org.testng.TestListenerAdapter;
 import org.testng.TestNG;
@@ -22,19 +21,20 @@ public void classListenerShouldBeOnlyInstantiatedOnce() {
   }
 
   private void checkGithub1130(TestNG tng) {
-    MyListener.count = 0;
-    MyListener.beforeSuiteCount = new ArrayList<>();
-    MyListener.beforeClassCount = new ArrayList<>();
     TestListenerAdapter adapter = new TestListenerAdapter();
     tng.addListener((ITestNGListener) adapter);
     tng.run();
     assertThat(adapter.getFailedTests()).isEmpty();
     assertThat(adapter.getSkippedTests()).isEmpty();
-    assertThat(MyListener.beforeSuiteCount.size()).isEqualTo(1);
-    assertThat(MyListener.beforeClassCount.size()).isEqualTo(2);
-    assertThat(MyListener.beforeSuiteCount.get(0))
-        .isEqualTo(MyListener.beforeClassCount.get(0))
-        .isEqualTo(MyListener.beforeClassCount.get(1));
-    assertThat(MyListener.count).isEqualTo(1);
+    // We cannot prevent TestNG from instantiating a listener multiple times
+    // because TestNG can throw away listener instances if it finds them already registered
+    // So the assertion should basically be able to check that ONLY 1 instance of the listener
+    // is being used, even though TestNG created multiple instances of it.
+    assertThat(MyListener.instance).isNotNull();
+    assertThat(MyListener.instance.beforeSuiteCount.size()).isEqualTo(1);
+    assertThat(MyListener.instance.beforeClassCount.size()).isEqualTo(2);
+    assertThat(MyListener.instance.beforeSuiteCount.get(0))
+        .isEqualTo(MyListener.instance.beforeClassCount.get(0))
+        .isEqualTo(MyListener.instance.beforeClassCount.get(1));
   }
 }

testng-core/src/test/java/test/listeners/github1130/MyListener.java

@@ -11,12 +11,14 @@
 
 public class MyListener implements ISuiteListener, IClassListener {
 
-  public static int count = 0;
-  public static List<String> beforeSuiteCount = new ArrayList<>();
-  public static List<String> beforeClassCount = new ArrayList<>();
+  public static MyListener instance;
+  public List<String> beforeSuiteCount = new ArrayList<>();
+  public List<String> beforeClassCount = new ArrayList<>();
 
   public MyListener() {
-    count++;
+    if (instance == null) {
+      instance = this;
+    }
   }
 
   public void onStart(ISuite suite) {

testng-core/src/test/java/test/listeners/github1296/GitHub1296Test.java

@@ -2,6 +2,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.entry;
+import static test.listeners.github1296.MyConfigurationListener.getCalls;
 
 import org.testng.TestNG;
 import org.testng.annotations.Test;
@@ -12,7 +13,7 @@ public class GitHub1296Test extends SimpleBaseTest {
 
   @Test(description = "https://github.com/cbeust/testng/issues/1296")
   public void test_number_of_call_of_configuration_listener() {
-    MyConfigurationListener.CALLS.clear();
+    MyConfigurationListener.clearCalls();
     XmlSuite suite = createXmlSuite("Tests");
     createXmlTest(suite, "Test version", MyTest.class);
     createXmlTest(suite, "Test version 2", MyTest.class);
@@ -21,7 +22,7 @@ public void test_number_of_call_of_configuration_listener() {
 
     tng.run();
 
-    assertThat(MyConfigurationListener.CALLS)
+    assertThat(getCalls())
         .hasSize(3)
         .containsOnly(
             entry("Test version", 2), entry("Test version 2", 2), entry("Test version 3", 2));

testng-core/src/test/java/test/listeners/github1296/MyConfigurationListener.java

@@ -1,23 +1,30 @@
 package test.listeners.github1296;
 
-import java.util.HashMap;
 import java.util.Map;
+import java.util.Map.Entry;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
 import org.testng.IConfigurationListener;
 import org.testng.ITestResult;
 
 public class MyConfigurationListener implements IConfigurationListener {
 
-  public static final Map<String, Integer> CALLS = new HashMap<>();
+  private static final Map<String, AtomicInteger> CALLS = new ConcurrentHashMap<>();
+
+  public static void clearCalls() {
+    CALLS.clear();
+  }
+
+  public static Map<String, Integer> getCalls() {
+    return CALLS.entrySet().stream()
+        .collect(Collectors.toMap(Entry::getKey, each -> each.getValue().get()));
+  }
 
   @Override
   public void onConfigurationSuccess(ITestResult itr) {
     String xmlTestName = itr.getTestContext().getCurrentXmlTest().getName();
-    Integer count = CALLS.get(xmlTestName);
-    if (count == null) {
-      count = 0;
-    }
-    count++;
-    CALLS.put(xmlTestName, count);
+    CALLS.computeIfAbsent(xmlTestName, k -> new AtomicInteger()).incrementAndGet();
   }
 
   @Override

testng-core/src/test/java/test/reports/EmailableReporterTest.java

@@ -75,7 +75,7 @@ private void runTestViaMainMethod(String clazzName, String jvm) {
       if (jvm != null) {
         System.setProperty(jvm, filename);
       }
-      TestNG.main(args);
+      TestNG.privateMain(args, null);
     } catch (SecurityException t) {
       // Gobble Security exception
     } finally {

testng-core/src/test/java/test/yaml/YamlTest.java

@@ -1,8 +1,6 @@
 package test.yaml;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.File;
 import java.io.FileInputStream;
@@ -38,10 +36,9 @@ public Object[][] dp() {
           "Validate that the YamlParser accepts yaml files with a .yaml or a .yml file extension, but not other file types.")
   public void accept() {
     YamlParser yamlParser = new YamlParser();
-
-    assertTrue(yamlParser.accept("TestSuite.yml"));
-    assertTrue(yamlParser.accept("TestSuite.yaml"));
-    assertFalse(yamlParser.accept("TestSuite.xml"));
+    assertThat(yamlParser.accept("TestSuite.yml")).isTrue();
+    assertThat(yamlParser.accept("TestSuite.yaml")).isTrue();
+    assertThat(yamlParser.accept("TestSuite.xml")).isFalse();
   }
 
   @Test(dataProvider = "dp")

testng-core/testng-core-build.gradle.kts

@@ -37,9 +37,10 @@ dependencies {
     implementation(projects.testngReflectionUtils)
     implementation(projects.testngRunnerApi)
     implementation("org.webjars:jquery:_")
-
     testImplementation(projects.testngAsserts)
-    testImplementation("org.codehaus.groovy:groovy-all:_")
+    testImplementation("org.codehaus.groovy:groovy-all:_") {
+        exclude("org.testng", "testng")
+    }
     testImplementation("org.spockframework:spock-core:_")
     testImplementation("org.apache-extras.beanshell:bsh:_")
     testImplementation("org.mockito:mockito-core:_")
@@ -64,7 +65,6 @@ tasks.test {
     maxParallelForks = Runtime.getRuntime().availableProcessors().div(2)
     (testFramework.options as TestNGOptions).apply {
         suites("src/test/resources/testng.xml")
-        listeners.add("org.testng.reporters.FailedInformationOnConsoleReporter")
         maxHeapSize = "1500m"
     }
 }

versions.properties

@@ -104,7 +104,9 @@ version.org.assertj..assertj-core=3.22.0
 ##                    # available=3.23.0
 ##                    # available=3.23.1
 
-version.org.codehaus.groovy..groovy-all=3.0.10
+## DONOT alter the version of groovy here because we need the compatible
+## version with what spock needs
+version.org.codehaus.groovy..groovy-all=2.5.4
 ##                          # available=3.0.11
 ##                          # available=3.0.12
 ##                          # available=3.0.13
@@ -127,7 +129,11 @@ version.org.ops4j.pax.exam..pax-exam-testng=4.13.5
 
 version.org.ops4j.pax.url..pax-url-aether=2.6.12
 
-version.org.spockframework..spock-core=2.1-groovy-3.0
+## DONOT upgrade the version to the 2.x series because spock 2.x series
+## starts expecting adherance the JUnit5 engine and we are NOT
+## planning to support running JUnit5 tests in TestNG and so we don't have
+## JUnit5 compliant runner
+version.org.spockframework..spock-core=1.3-groovy-2.5
 ##                         # available=2.1-M1-groovy-2.5
 ##                         # available=2.1-M1-groovy-3.0
 ##                         # available=2.1-M2-groovy-2.5