From 05d629d4aa35f99d370a7250927c06233ab02dac Mon Sep 17 00:00:00 2001 From: Krishnan Mahadevan Date: Wed, 3 Apr 2024 20:44:46 +0530 Subject: [PATCH] Streamline dependencies for configurations Closes #3000 --- CHANGES.txt | 1 + .../org/testng/internal/MethodHelper.java | 52 ++++++++++++++++--- .../test/configuration/BeforeClassTest.java | 31 +++++++++++ .../issue3000/MyBaseTestSample.java | 21 ++++++++ .../configuration/issue3000/MyInterface.java | 10 ++++ .../issue3000/TestClassSample.java | 17 ++++++ 6 files changed, 125 insertions(+), 7 deletions(-) create mode 100644 testng-core/src/test/java/test/configuration/issue3000/MyBaseTestSample.java create mode 100644 testng-core/src/test/java/test/configuration/issue3000/MyInterface.java create mode 100644 testng-core/src/test/java/test/configuration/issue3000/TestClassSample.java diff --git a/CHANGES.txt b/CHANGES.txt index 8a80ab8d48..bc32fbcaad 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,5 +1,6 @@ Current (7.10.0) +Fixed: GITHUB-3000: Method predecessors lookup and/or method sorting is broken in certain inheritance and naming setups (Krishnan Mahadevan) Fixed: GITHUB-3095: Super class annotated with ITestNGListenerFactory makes derived test class throw TestNGException on execution (Krishnan Mahadevan) Fixed: GITHUB-3081: Discrepancy with combination of (Shared Thread pool and Method Interceptor) (Krishnan Mahadevan) Fixed: GITHUB-2381: Controlling the inclusion of the listener at runtime (Krishnan Mahadevan) 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 16f2dd6682..fe14680e8f 100644 --- a/testng-core/src/main/java/org/testng/internal/MethodHelper.java +++ b/testng-core/src/main/java/org/testng/internal/MethodHelper.java @@ -7,6 +7,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicReference; @@ -325,6 +326,7 @@ private static Graph topologicalSort( Map> testInstances = sortMethodsByInstance(methods); XmlTest xmlTest = null; + for (ITestNGMethod m : methods) { if (xmlTest == null) { xmlTest = m.getXmlTest(); @@ -358,13 +360,12 @@ private static Graph topologicalSort( !(m.isBeforeGroupsConfiguration() || m.isAfterGroupsConfiguration()); boolean isGroupAgnosticConfigMethod = !m.isTest() && anyConfigExceptGroupConfigs; if (isGroupAgnosticConfigMethod) { - String[] groupsDependedUpon = m.getGroupsDependedUpon(); - if (groupsDependedUpon.length > 0) { - for (String group : groupsDependedUpon) { - ITestNGMethod[] methodsThatBelongToGroup = - MethodGroupsHelper.findMethodsThatBelongToGroup(m, methods, group); - predecessors.addAll(Arrays.asList(methodsThatBelongToGroup)); - } + String[] groupsDependedUpon = + Optional.ofNullable(m.getGroupsDependedUpon()).orElse(new String[0]); + for (String group : groupsDependedUpon) { + ITestNGMethod[] methodsThatBelongToGroup = + MethodGroupsHelper.findMethodsThatBelongToGroup(m, methods, group); + predecessors.addAll(Arrays.asList(methodsThatBelongToGroup)); } } @@ -380,6 +381,31 @@ private static Graph topologicalSort( return result; } + private static Comparator bubbleUpIndependentMethodsFirst() { + return (a, b) -> { + boolean aIsIndependent = isIndependent(a); + boolean bIsIndependent = isIndependent(b); + if (aIsIndependent && bIsIndependent) { + // Both a and b are independent methods. So treat them as equal. + return 0; + } + if (aIsIndependent) { + // First method is independent. So a should come before b. + return -1; + } + if (bIsIndependent) { + // Second method is independent. So a should come after b. + return 1; + } + // Both a and b are dependent methods. So treat them as equal + return 0; + }; + } + + private static boolean isIndependent(ITestNGMethod tm) { + return tm.getMethodsDependedUpon().length == 0 && tm.getGroupsDependedUpon().length == 0; + } + /** * This method is used to create a map of test instances and their associated method(s) . Used to * decrease the scope to only a methods instance when trying to find method dependencies. @@ -442,6 +468,18 @@ private static List sortMethods( || m.isBeforeSuiteConfiguration() || m.isBeforeTestConfiguration(); MethodInheritance.fixMethodInheritance(allMethodsArray, before); + + // In-case the user has ended up using "dependsOn" on configurations, then sometimes + // TestNG ends up finding the configuration methods in such a way that, it can cause + // un-expected failures. This usually happens due to the fully qualified method names + // acting up. So let's re-order the methods such that, the independent configurations always + // bubble up to the top and the ones that have dependencies get pushed to the bottom. + // That way, when we do a topologicalSort sort, the logic would work fine. + + allMethodsArray = + Arrays.stream(allMethodsArray) + .sorted(bubbleUpIndependentMethodsFirst()) + .toArray(ITestNGMethod[]::new); } topologicalSort(allMethodsArray, sl, pl, comparator); diff --git a/testng-core/src/test/java/test/configuration/BeforeClassTest.java b/testng-core/src/test/java/test/configuration/BeforeClassTest.java index 38436b156f..912de2103d 100644 --- a/testng-core/src/test/java/test/configuration/BeforeClassTest.java +++ b/testng-core/src/test/java/test/configuration/BeforeClassTest.java @@ -4,6 +4,10 @@ import java.util.ArrayList; import java.util.List; +import java.util.stream.Collectors; +import org.testng.IReporter; +import org.testng.ISuite; +import org.testng.ITestResult; import org.testng.TestNG; import org.testng.annotations.Test; import org.testng.xml.XmlSuite; @@ -11,6 +15,7 @@ import test.SimpleBaseTest; import test.configuration.issue1035.InvocationTracker; import test.configuration.issue1035.MyFactory; +import test.configuration.issue3000.TestClassSample; public class BeforeClassTest extends SimpleBaseTest { @@ -48,4 +53,30 @@ public void ensureBeforeClassGetsCalledConcurrentlyWhenWorkingWithFactories() { previousThreadId = current.getThreadId(); } } + + @Test(description = "GITHUB-3000") + public void ensureIndependentConfigurationsAlwaysRunFirstWhenUsingDependencies() { + TestNG testng = create(TestClassSample.class); + testng.setVerbose(2); + + List failures = new ArrayList<>(); + testng.addListener( + new IReporter() { + @Override + public void generateReport( + List xmlSuites, List suites, String outputDirectory) { + List filtered = + suites.stream() + .flatMap(it -> it.getResults().values().stream()) + .flatMap( + it -> + it.getTestContext().getFailedConfigurations().getAllResults().stream()) + .collect(Collectors.toList()); + failures.addAll(filtered); + } + }); + testng.run(); + assertThat(testng.getStatus()).isZero(); + assertThat(failures).isEmpty(); + } } diff --git a/testng-core/src/test/java/test/configuration/issue3000/MyBaseTestSample.java b/testng-core/src/test/java/test/configuration/issue3000/MyBaseTestSample.java new file mode 100644 index 0000000000..ef2d6587db --- /dev/null +++ b/testng-core/src/test/java/test/configuration/issue3000/MyBaseTestSample.java @@ -0,0 +1,21 @@ +package test.configuration.issue3000; + +import org.testng.annotations.BeforeClass; + +abstract class MyBaseTestSample implements MyInterface { + protected Object dependency; + + public void setDependency(Object ignored) {} + + @BeforeClass + public void setupDependency() { + dependency = new Object(); + } + + // Had to add the "__" to this method (This is not how it looks like in the sample provided + // in the GitHub issue). A combination of the fully qualified method names is what causes + // this method to be first found in the configuration methods by TestNG and so it causes + // the issue. + @BeforeClass(dependsOnMethods = "setupDependency") + public void __setupAdditionalDependency_() {} +} diff --git a/testng-core/src/test/java/test/configuration/issue3000/MyInterface.java b/testng-core/src/test/java/test/configuration/issue3000/MyInterface.java new file mode 100644 index 0000000000..8e750a6fad --- /dev/null +++ b/testng-core/src/test/java/test/configuration/issue3000/MyInterface.java @@ -0,0 +1,10 @@ +package test.configuration.issue3000; + +@SuppressWarnings("unused") +interface MyInterface { + void setDependency(Object dependency); + + default Object getDependency() { + return null; + } +} diff --git a/testng-core/src/test/java/test/configuration/issue3000/TestClassSample.java b/testng-core/src/test/java/test/configuration/issue3000/TestClassSample.java new file mode 100644 index 0000000000..eb15b7e4fd --- /dev/null +++ b/testng-core/src/test/java/test/configuration/issue3000/TestClassSample.java @@ -0,0 +1,17 @@ +package test.configuration.issue3000; + +import static org.testng.Assert.assertNotNull; + +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +public class TestClassSample extends MyBaseTestSample { + + @BeforeClass + public void beforeClass() { + assertNotNull(dependency); + } + + @Test + public void test() {} +}