Skip to content

Commit

Permalink
Streamline dependencies for configurations
Browse files Browse the repository at this point in the history
Closes #3000
  • Loading branch information
krmahadevan committed Apr 4, 2024
1 parent 2e5d8bf commit 05d629d
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 7 deletions.
1 change: 1 addition & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
52 changes: 45 additions & 7 deletions testng-core/src/main/java/org/testng/internal/MethodHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -325,6 +326,7 @@ private static Graph<ITestNGMethod> topologicalSort(
Map<Object, List<ITestNGMethod>> testInstances = sortMethodsByInstance(methods);

XmlTest xmlTest = null;

for (ITestNGMethod m : methods) {
if (xmlTest == null) {
xmlTest = m.getXmlTest();
Expand Down Expand Up @@ -358,13 +360,12 @@ private static Graph<ITestNGMethod> 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));
}
}

Expand All @@ -380,6 +381,31 @@ private static Graph<ITestNGMethod> topologicalSort(
return result;
}

private static Comparator<ITestNGMethod> 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.
Expand Down Expand Up @@ -442,6 +468,18 @@ private static List<ITestNGMethod> 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);
Expand Down
31 changes: 31 additions & 0 deletions testng-core/src/test/java/test/configuration/BeforeClassTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@

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;
import org.testng.xml.XmlSuite.ParallelMode;
import test.SimpleBaseTest;
import test.configuration.issue1035.InvocationTracker;
import test.configuration.issue1035.MyFactory;
import test.configuration.issue3000.TestClassSample;

public class BeforeClassTest extends SimpleBaseTest {

Expand Down Expand Up @@ -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<ITestResult> failures = new ArrayList<>();
testng.addListener(
new IReporter() {
@Override
public void generateReport(
List<XmlSuite> xmlSuites, List<ISuite> suites, String outputDirectory) {
List<ITestResult> 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();
}
}
Original file line number Diff line number Diff line change
@@ -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_() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package test.configuration.issue3000;

@SuppressWarnings("unused")
interface MyInterface {
void setDependency(Object dependency);

default Object getDependency() {
return null;
}
}
Original file line number Diff line number Diff line change
@@ -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() {}
}

0 comments on commit 05d629d

Please sign in to comment.