Skip to content

Commit be97321

Browse files
baflQABartek Florczakjuherr
authored
Fix: issue 3231 retry infinite loop (#3251)
Following was done - Add getParameterIndex() to ITestResult interface. - Use parameterIndex in BaseTestMethod to identify test instances for RetryAnalyzer. - This ensures a stable key even if parameters are mutated. - Also fixes shared retry limit for rows with equal parameters. - Add regression tests Issue3231Test and RetryLimitTest. --------- Co-authored-by: Bartek Florczak <b.florczak@sap.com> Co-authored-by: Julien Herr <julien@herr.fr>
1 parent bad4cb5 commit be97321

File tree

10 files changed

+222
-62
lines changed

10 files changed

+222
-62
lines changed

‎CHANGES.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
Current (7.12.0)
2+
Fixed: GITHUB-3231: TestNG retry is going into infinite loop when the data provider returned object is modified before failure (Bartek Florczak)
23
Update: Updated GitHub Actions test matrix to include JDK 25 and JDK 26 EA (Bartek Florczak)
34
Fixed: GITHUB-3236: DataProvider parameters are not refreshed on retry when cacheDataForTestRetries=false (Bartek Florczak)
45
Fixed: GITHUB-3227: assertEqualsNoOrder false positive when collection has same size and actual Collection is subset of expected collection (Krishnan Mahadevan)

‎testng-core-api/src/main/java/org/testng/ITestResult.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ public interface ITestResult extends IAttributes, Comparable<ITestResult> {
2222
int SUCCESS_PERCENTAGE_FAILURE = 4;
2323
int STARTED = 16;
2424

25+
default int getParameterIndex() {
26+
return -1;
27+
}
28+
2529
/** @return The status of this result, using one of the constants above. */
2630
int getStatus();
2731

‎testng-core/src/main/java/org/testng/internal/BaseTestMethod.java‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,8 @@
1010
import java.util.Set;
1111
import java.util.UUID;
1212
import java.util.concurrent.Callable;
13-
import java.util.concurrent.ConcurrentHashMap;
1413
import java.util.concurrent.ConcurrentLinkedQueue;
1514
import java.util.concurrent.atomic.AtomicInteger;
16-
import java.util.function.Function;
1715
import java.util.regex.Pattern;
1816
import java.util.stream.Collectors;
1917
import org.testng.IClass;
@@ -843,13 +841,8 @@ private IRetryAnalyzer getRetryAnalyzerConsideringMethodParameters(ITestResult t
843841
});
844842
}
845843

846-
private final Map<IObject.IdentifiableArrayObject, IObject.IdentifiableArrayObject> parameters =
847-
new ConcurrentHashMap<>();
848-
849-
private String parameterId(ITestResult itr) {
850-
IObject.IdentifiableArrayObject parameter =
851-
new IObject.IdentifiableArrayObject(itr.getParameters());
852-
return parameters.computeIfAbsent(parameter, Function.identity()).getInstanceId();
844+
private static String parameterId(ITestResult itr) {
845+
return Integer.toString(itr.getParameterIndex());
853846
}
854847

855848
private static boolean isNotParameterisedTest(ITestResult tr) {

‎testng-core/src/main/java/org/testng/internal/IObject.java‎

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package org.testng.internal;
22

3-
import java.util.Arrays;
43
import java.util.Objects;
54
import java.util.Optional;
65
import java.util.UUID;
@@ -114,36 +113,4 @@ public int hashCode() {
114113
return Objects.hash(instanceId);
115114
}
116115
}
117-
118-
/**
119-
* A wrapper class that wraps around an array and associates a unique Id that can be used as a key
120-
* for the array.
121-
*/
122-
class IdentifiableArrayObject {
123-
124-
private final String instanceId = UUID.randomUUID().toString();
125-
126-
private final Object[] parameters;
127-
128-
public IdentifiableArrayObject(Object[] parameters) {
129-
this.parameters = parameters;
130-
}
131-
132-
public String getInstanceId() {
133-
return instanceId;
134-
}
135-
136-
@Override
137-
public boolean equals(Object object) {
138-
if (this == object) return true;
139-
if (object == null || getClass() != object.getClass()) return false;
140-
IdentifiableArrayObject that = (IdentifiableArrayObject) object;
141-
return Arrays.equals(parameters, that.parameters);
142-
}
143-
144-
@Override
145-
public int hashCode() {
146-
return Arrays.hashCode(parameters);
147-
}
148-
}
149116
}

‎testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -592,9 +592,8 @@ private boolean shouldRetryTestMethod(
592592
// pass both paramValues and paramIndex to be thread safe in case parallel=true + dataprovider.
593593
private ITestResult invokeMethod(
594594
TestMethodArguments arguments, XmlSuite suite, FailureContext failureContext) {
595-
TestResult testResult = TestResult.newEmptyTestResult();
596-
testResult.setParameters(arguments.getParameterValues());
597-
testResult.setParameterIndex(arguments.getParametersIndex());
595+
TestResult testResult =
596+
TestResult.newTestResult(arguments.getParameterValues(), arguments.getParametersIndex());
598597
testResult.setHost(m_testContext.getHost());
599598

600599
GroupConfigMethodArguments cfgArgs =

‎testng-core/src/test/java/test/retryAnalyzer/RetryAnalyzerTest.java‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,12 @@
88
import java.util.List;
99
import java.util.Map;
1010
import java.util.Set;
11+
import java.util.concurrent.atomic.AtomicInteger;
1112
import java.util.function.Function;
1213
import java.util.stream.Collectors;
1314
import org.testng.Assert;
15+
import org.testng.IInvokedMethod;
16+
import org.testng.IInvokedMethodListener;
1417
import org.testng.ITestResult;
1518
import org.testng.TestListenerAdapter;
1619
import org.testng.TestNG;
@@ -40,9 +43,58 @@
4043
import test.retryAnalyzer.issue2684.SampleTestClassWithGroupConfigs;
4144
import test.retryAnalyzer.issue2798.HashCodeAwareRetryAnalyzer;
4245
import test.retryAnalyzer.issue2798.TestClassSample;
46+
import test.retryAnalyzer.issue3231.InvocationCountRetrySample;
47+
import test.retryAnalyzer.issue3231.MutationSample;
48+
import test.retryAnalyzer.issue3231.RetryLimitSample;
4349

4450
public class RetryAnalyzerTest extends SimpleBaseTest {
4551

52+
@Test
53+
public void testRetryWithInvocationCount() {
54+
InvocationCountRetrySample.invocationCount.set(0);
55+
TestNG tng = create(InvocationCountRetrySample.class);
56+
tng.run();
57+
// 2 invocations * 2 attempts each = 4 total calls
58+
assertThat(InvocationCountRetrySample.invocationCount.get()).isEqualTo(4);
59+
}
60+
61+
@Test(description = "GITHUB-3231")
62+
public void testMutationDoesNotCauseInfiniteRetryLoop() {
63+
MutationSample.guardCounter.set(0);
64+
AtomicInteger invCount = new AtomicInteger(0);
65+
TestNG tng = create(MutationSample.class);
66+
tng.addListener(
67+
new IInvokedMethodListener() {
68+
@Override
69+
public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
70+
if (method.isTestMethod()) {
71+
invCount.incrementAndGet();
72+
}
73+
}
74+
});
75+
tng.run();
76+
assertThat(invCount.get()).isEqualTo(4);
77+
}
78+
79+
@Test(description = "GITHUB-3231")
80+
public void testEachRowHasItsOwnRetryLimit() {
81+
AtomicInteger invCount = new AtomicInteger(0);
82+
TestNG tng = create(RetryLimitSample.class);
83+
tng.addListener(
84+
new IInvokedMethodListener() {
85+
@Override
86+
public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
87+
if (method.isTestMethod()) {
88+
invCount.incrementAndGet();
89+
}
90+
}
91+
});
92+
tng.run();
93+
// Each row: 1 original + 1 retry = 2 attempts.
94+
// 2 rows * 2 attempts = 4 total invocations.
95+
assertThat(invCount.get()).isEqualTo(4);
96+
}
97+
4698
@Test(description = "GITHUB-2798")
4799
public void ensureNoDuplicateRetryAnalyzerInstancesAreCreated() {
48100
create(TestClassSample.class).run();

‎testng-core/src/test/java/test/retryAnalyzer/issue3231/InvocationCountRetrySample.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package test.retryAnalyzer.issue3231;
2+
3+
import java.util.concurrent.atomic.AtomicInteger;
4+
import org.testng.Assert;
5+
import org.testng.IRetryAnalyzer;
6+
import org.testng.ITestResult;
7+
import org.testng.annotations.Test;
8+
9+
public class InvocationCountRetrySample {
10+
11+
public static class MyRetry implements IRetryAnalyzer {
12+
private int count = 0;
13+
14+
@Override
15+
public boolean retry(ITestResult result) {
16+
return count++ < 1; // Retry once
17+
}
18+
}
19+
20+
public static final AtomicInteger invocationCount = new AtomicInteger(0);
21+
22+
@Test(invocationCount = 2, retryAnalyzer = MyRetry.class)
23+
public void test() {
24+
invocationCount.incrementAndGet();
25+
Assert.fail("fail");
26+
}
27+
}

‎testng-core/src/test/java/test/retryAnalyzer/issue3231/MutationSample.java‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
package test.retryAnalyzer.issue3231;
2+
3+
import java.util.Objects;
4+
import java.util.UUID;
5+
import java.util.concurrent.atomic.AtomicInteger;
6+
import org.testng.Assert;
7+
import org.testng.IRetryAnalyzer;
8+
import org.testng.ITestResult;
9+
import org.testng.annotations.DataProvider;
10+
import org.testng.annotations.Test;
11+
12+
public class MutationSample {
13+
14+
public static class Custom {
15+
private UUID id;
16+
17+
public Custom(UUID id) {
18+
this.id = id;
19+
}
20+
21+
public UUID getId() {
22+
return id;
23+
}
24+
25+
public void setId(UUID id) {
26+
this.id = id;
27+
}
28+
29+
@Override
30+
public boolean equals(Object o) {
31+
if (this == o) return true;
32+
if (o == null || getClass() != o.getClass()) return false;
33+
Custom custom = (Custom) o;
34+
return Objects.equals(id, custom.id);
35+
}
36+
37+
@Override
38+
public int hashCode() {
39+
return Objects.hash(id);
40+
}
41+
}
42+
43+
public static class MyRetry implements IRetryAnalyzer {
44+
private int retryCount = 0;
45+
private int maxRetryCount = 3;
46+
47+
@Override
48+
public boolean retry(ITestResult result) {
49+
if (retryCount < maxRetryCount) {
50+
retryCount++;
51+
return true;
52+
}
53+
return false;
54+
}
55+
}
56+
57+
public static final AtomicInteger guardCounter = new AtomicInteger(0);
58+
59+
@DataProvider(name = "dpCustomObject")
60+
public Object[][] dpCustomObject() {
61+
return new Object[][] {{new Custom(UUID.randomUUID())}};
62+
}
63+
64+
@Test(dataProvider = "dpCustomObject", retryAnalyzer = MyRetry.class)
65+
public void willNotStopAfter3FailuresCustom(Custom newObject) {
66+
if (guardCounter.incrementAndGet() > 100) {
67+
return;
68+
}
69+
newObject.setId(UUID.randomUUID()); // Mutate!
70+
Assert.fail();
71+
}
72+
}

‎testng-core/src/test/java/test/retryAnalyzer/issue3231/RetryLimitSample.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package test.retryAnalyzer.issue3231;
2+
3+
import org.testng.Assert;
4+
import org.testng.IRetryAnalyzer;
5+
import org.testng.ITestResult;
6+
import org.testng.annotations.DataProvider;
7+
import org.testng.annotations.Test;
8+
9+
public class RetryLimitSample {
10+
11+
public static class MyRetry implements IRetryAnalyzer {
12+
private int count = 0;
13+
14+
@Override
15+
public boolean retry(ITestResult result) {
16+
count++;
17+
return count < 2; // Retry once (total 2 attempts)
18+
}
19+
}
20+
21+
@DataProvider(name = "dp")
22+
public Object[][] dp() {
23+
return new Object[][] {{"a"}, {"a"}};
24+
}
25+
26+
@Test(dataProvider = "dp", retryAnalyzer = MyRetry.class)
27+
public void test(String s) {
28+
Assert.fail("fail");
29+
}
30+
}

0 commit comments

Comments
 (0)