From ceccb5aee25149ddca28b247280ac1467545776f Mon Sep 17 00:00:00 2001 From: Krishnan Mahadevan Date: Tue, 29 Nov 2022 20:13:22 +0530 Subject: [PATCH] Ensure All tests run all the time Closes #2841 --- .../src/main/java/org/testng/SuiteRunner.java | 4 ++++ .../src/main/java/org/testng/TestRunner.java | 17 +++++++------- .../testng/internal/ClassBasedWrapper.java | 2 +- .../org/testng/internal/MethodHelper.java | 10 +++----- .../groovy/test/groovy/SpockSample.groovy | 2 +- .../java/test/InvokedMethodNameListener.java | 4 +++- .../test/dataprovider/DataProviderTest.java | 3 ++- .../dataprovider/issue2819/SimpleRetry.java | 13 +++++++---- .../java/test/dependent/DependentTest.java | 12 ++++------ .../listeners/github1130/GitHub1130Test.java | 20 ++++++++-------- .../test/listeners/github1130/MyListener.java | 10 ++++---- .../listeners/github1296/GitHub1296Test.java | 5 ++-- .../github1296/MyConfigurationListener.java | 23 ++++++++++++------- .../test/reports/EmailableReporterTest.java | 2 +- .../src/test/java/test/yaml/YamlTest.java | 9 +++----- testng-core/testng-core-build.gradle.kts | 6 ++--- versions.properties | 10 ++++++-- 17 files changed, 85 insertions(+), 67 deletions(-) diff --git a/testng-core/src/main/java/org/testng/SuiteRunner.java b/testng-core/src/main/java/org/testng/SuiteRunner.java index 80e9e1d60a..bbb1ed826c 100644 --- a/testng-core/src/main/java/org/testng/SuiteRunner.java +++ b/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); } diff --git a/testng-core/src/main/java/org/testng/TestRunner.java b/testng-core/src/main/java/org/testng/TestRunner.java index 0d34d51957..c1f5ef673c 100644 --- a/testng-core/src/main/java/org/testng/TestRunner.java +++ b/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 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() { diff --git a/testng-core/src/main/java/org/testng/internal/ClassBasedWrapper.java b/testng-core/src/main/java/org/testng/internal/ClassBasedWrapper.java index a46c0675f2..7c43793f49 100644 --- a/testng-core/src/main/java/org/testng/internal/ClassBasedWrapper.java +++ b/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 diff --git a/testng-core/src/main/java/org/testng/internal/MethodHelper.java b/testng-core/src/main/java/org/testng/internal/MethodHelper.java index d8812aa563..6b1a5904e7 100644 --- a/testng-core/src/main/java/org/testng/internal/MethodHelper.java +++ b/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 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 methods) { @@ -95,7 +95,7 @@ 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 = @@ -103,10 +103,6 @@ public static ITestNGMethod[] findDependedUponMethods(ITestNGMethod m, ITestNGMe .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 vResult = Lists.newArrayList(); diff --git a/testng-core/src/test/groovy/test/groovy/SpockSample.groovy b/testng-core/src/test/groovy/test/groovy/SpockSample.groovy index 826541383f..d6d3709f22 100644 --- a/testng-core/src/test/groovy/test/groovy/SpockSample.groovy +++ b/testng-core/src/test/groovy/test/groovy/SpockSample.groovy @@ -14,4 +14,4 @@ class SpockSample extends Specification { then: stack.size() == 1 } -} \ No newline at end of file +} diff --git a/testng-core/src/test/java/test/InvokedMethodNameListener.java b/testng-core/src/test/java/test/InvokedMethodNameListener.java index 40e5ab5845..9d731a7a55 100644 --- a/testng-core/src/test/java/test/InvokedMethodNameListener.java +++ b/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 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()) { diff --git a/testng-core/src/test/java/test/dataprovider/DataProviderTest.java b/testng-core/src/test/java/test/dataprovider/DataProviderTest.java index 12636d46ff..a94208cd64 100644 --- a/testng-core/src/test/java/test/dataprovider/DataProviderTest.java +++ b/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); diff --git a/testng-core/src/test/java/test/dataprovider/issue2819/SimpleRetry.java b/testng-core/src/test/java/test/dataprovider/issue2819/SimpleRetry.java index 7c3ca99f38..417e8105e3 100644 --- a/testng-core/src/test/java/test/dataprovider/issue2819/SimpleRetry.java +++ b/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 hashCodes = new HashSet<>(); + private static final Set objectIds = new HashSet<>(); - public static Set getHashCodes() { - return Collections.unmodifiableSet(hashCodes); + public static Set 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; diff --git a/testng-core/src/test/java/test/dependent/DependentTest.java b/testng-core/src/test/java/test/dependent/DependentTest.java index 4ee5fbf8c9..65acbc12d7 100644 --- a/testng-core/src/test/java/test/dependent/DependentTest.java +++ b/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); diff --git a/testng-core/src/test/java/test/listeners/github1130/GitHub1130Test.java b/testng-core/src/test/java/test/listeners/github1130/GitHub1130Test.java index ac3ab28b23..47e7edd1cb 100644 --- a/testng-core/src/test/java/test/listeners/github1130/GitHub1130Test.java +++ b/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)); } } diff --git a/testng-core/src/test/java/test/listeners/github1130/MyListener.java b/testng-core/src/test/java/test/listeners/github1130/MyListener.java index 856f8bb9c5..43b75d3988 100644 --- a/testng-core/src/test/java/test/listeners/github1130/MyListener.java +++ b/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 beforeSuiteCount = new ArrayList<>(); - public static List beforeClassCount = new ArrayList<>(); + public static MyListener instance; + public List beforeSuiteCount = new ArrayList<>(); + public List beforeClassCount = new ArrayList<>(); public MyListener() { - count++; + if (instance == null) { + instance = this; + } } public void onStart(ISuite suite) { diff --git a/testng-core/src/test/java/test/listeners/github1296/GitHub1296Test.java b/testng-core/src/test/java/test/listeners/github1296/GitHub1296Test.java index 06ca551f94..d3bd0cba95 100644 --- a/testng-core/src/test/java/test/listeners/github1296/GitHub1296Test.java +++ b/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)); diff --git a/testng-core/src/test/java/test/listeners/github1296/MyConfigurationListener.java b/testng-core/src/test/java/test/listeners/github1296/MyConfigurationListener.java index 05f17f34df..9d94ed808f 100644 --- a/testng-core/src/test/java/test/listeners/github1296/MyConfigurationListener.java +++ b/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 CALLS = new HashMap<>(); + private static final Map CALLS = new ConcurrentHashMap<>(); + + public static void clearCalls() { + CALLS.clear(); + } + + public static Map 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 diff --git a/testng-core/src/test/java/test/reports/EmailableReporterTest.java b/testng-core/src/test/java/test/reports/EmailableReporterTest.java index a4ed54a13b..beffc5390d 100644 --- a/testng-core/src/test/java/test/reports/EmailableReporterTest.java +++ b/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 { diff --git a/testng-core/src/test/java/test/yaml/YamlTest.java b/testng-core/src/test/java/test/yaml/YamlTest.java index 701111e3c5..92887ac1bf 100644 --- a/testng-core/src/test/java/test/yaml/YamlTest.java +++ b/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") diff --git a/testng-core/testng-core-build.gradle.kts b/testng-core/testng-core-build.gradle.kts index aea422557e..6924f99e15 100644 --- a/testng-core/testng-core-build.gradle.kts +++ b/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" } } diff --git a/versions.properties b/versions.properties index 1b7e30df5a..dd090a106a 100644 --- a/versions.properties +++ b/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