Skip to content

Commit 0fb354b

Browse files
lhotarilhotari
committed
[fix][test] Simplify BetweenTestClassesListenerAdapter and fix issue with BeforeTest/AfterTest annotations (#24304)
### Motivation It wasn't clear how test class changes should be detected with TestNG. The solution was recently revisited in #24258. However problems remain after adding that solution. BeforeTest/AfterTest annotation usage won't work and cause false reports for detected leaks. It turns out that it's possible to detect a test change with ITestListener's `onFinish` callback. In TestNG, it's possible to group multiple test classes into a single test, but this is only when using TestNG's XML configuration for test suites. maven-surefire-plugin will create a separate TestNG test for each test class, so that's why it's the correct solution for BetweenTestClassesListenerAdapter. ### Modifications - Remove previous logic to detect test class change and instead rely on ITestListener's `onFinish` callback - Adapt the current listener implementations to the new base class (cherry picked from commit 965ef5c)
1 parent 57c4b9b commit 0fb354b

File tree

6 files changed

+75
-101
lines changed

6 files changed

+75
-101
lines changed

‎buildtools/src/main/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapter.java‎

Lines changed: 15 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -19,88 +19,33 @@
1919
package org.apache.pulsar.tests;
2020

2121
import java.util.Arrays;
22+
import java.util.List;
23+
import java.util.stream.Collectors;
2224
import org.slf4j.Logger;
2325
import org.slf4j.LoggerFactory;
24-
import org.testng.IClass;
25-
import org.testng.IClassListener;
26-
import org.testng.IConfigurationListener;
2726
import org.testng.ITestClass;
27+
import org.testng.ITestContext;
28+
import org.testng.ITestListener;
2829
import org.testng.ITestNGMethod;
29-
import org.testng.ITestResult;
3030

3131
/**
32-
* TestNG listener adapter that detects when execution finishes in a test class, including AfterClass methods.
33-
* TestNG's IClassListener.onAfterClass method is called before AfterClass methods are executed,
34-
* which is why this solution is needed.
32+
* TestNG listener adapter that detects when execution finishes for a test class,
33+
* assuming that a single test class is run in each context.
34+
* This is the case when running tests with maven-surefire-plugin.
3535
*/
36-
abstract class BetweenTestClassesListenerAdapter implements IClassListener, IConfigurationListener {
36+
abstract class BetweenTestClassesListenerAdapter implements ITestListener {
3737
private static final Logger log = LoggerFactory.getLogger(BetweenTestClassesListenerAdapter.class);
38-
volatile Class<?> currentTestClass;
39-
volatile int remainingAfterClassMethodCount;
4038

4139
@Override
42-
public final void onBeforeClass(ITestClass testClass) {
43-
// for parameterized tests for the same class, the onBeforeClass method is called for each instance
44-
// so we need to check if the test class is the same as for the previous call before resetting the counter
45-
if (testClass.getRealClass() != currentTestClass) {
46-
// find out how many parameterized instances of the test class are expected
47-
Object[] instances = testClass.getInstances(false);
48-
int instanceCount = instances != null && instances.length != 0 ? instances.length : 1;
49-
// expect invocations of all annotated and enabled after class methods
50-
int annotatedAfterClassMethodCount = (int) Arrays.stream(testClass.getAfterClassMethods())
51-
.filter(ITestNGMethod::getEnabled)
52-
.count();
53-
// additionally expect invocations of the "onAfterClass" listener method in this class
54-
int expectedMethodCountForEachInstance = 1 + annotatedAfterClassMethodCount;
55-
// multiple by the number of instances
56-
remainingAfterClassMethodCount = instanceCount * expectedMethodCountForEachInstance;
57-
currentTestClass = testClass.getRealClass();
58-
}
59-
}
60-
61-
@Override
62-
public final void onAfterClass(ITestClass testClass) {
63-
handleAfterClassMethodCalled(testClass);
64-
}
65-
66-
@Override
67-
public final void onConfigurationSuccess(ITestResult tr) {
68-
handleAfterClassConfigurationMethodCompletion(tr);
69-
}
70-
71-
@Override
72-
public final void onConfigurationSkip(ITestResult tr) {
73-
handleAfterClassConfigurationMethodCompletion(tr);
74-
}
75-
76-
@Override
77-
public final void onConfigurationFailure(ITestResult tr) {
78-
handleAfterClassConfigurationMethodCompletion(tr);
79-
}
80-
81-
private void handleAfterClassConfigurationMethodCompletion(ITestResult tr) {
82-
if (tr.getMethod().isAfterClassConfiguration() && !tr.wasRetried()) {
83-
handleAfterClassMethodCalled(tr.getTestClass());
84-
}
85-
}
86-
87-
private void handleAfterClassMethodCalled(IClass testClass) {
88-
if (currentTestClass != testClass.getRealClass()) {
89-
log.error("Unexpected test class: {}. Expected: {}", testClass.getRealClass(), currentTestClass);
90-
return;
91-
}
92-
remainingAfterClassMethodCount--;
93-
if (remainingAfterClassMethodCount == 0) {
94-
onBetweenTestClasses(testClass);
95-
} else if (remainingAfterClassMethodCount < 0) {
96-
// unexpected case, log it for easier debugging if this causes test failures
97-
log.error("Remaining after class method count is negative: {} for test class: {}",
98-
remainingAfterClassMethodCount, testClass.getRealClass());
99-
}
40+
public final void onFinish(ITestContext context) {
41+
List<ITestClass> testClasses =
42+
Arrays.stream(context.getAllTestMethods()).map(ITestNGMethod::getTestClass).distinct()
43+
.collect(Collectors.toList());
44+
onBetweenTestClasses(testClasses);
10045
}
10146

10247
/**
103-
* Call back hook for adding logic when test execution has completely finished for a test class.
48+
* Call back hook for adding logic when test execution has completely finished for one or many test classes.
10449
*/
105-
protected abstract void onBetweenTestClasses(IClass testClass);
50+
protected abstract void onBetweenTestClasses(List<ITestClass> testClasses);
10651
}

‎buildtools/src/main/java/org/apache/pulsar/tests/FastThreadLocalCleanupListener.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,10 @@
1818
*/
1919
package org.apache.pulsar.tests;
2020

21+
import java.util.List;
2122
import org.slf4j.Logger;
2223
import org.slf4j.LoggerFactory;
23-
import org.testng.IClass;
24+
import org.testng.ITestClass;
2425

2526
/**
2627
* Cleanup Thread Local state attach to Netty's FastThreadLocal.
@@ -49,7 +50,7 @@ public class FastThreadLocalCleanupListener extends BetweenTestClassesListenerAd
4950
});
5051

5152
@Override
52-
protected void onBetweenTestClasses(IClass testClass) {
53+
protected void onBetweenTestClasses(List<ITestClass> testClasses) {
5354
if (FAST_THREAD_LOCAL_CLEANUP_ENABLED && FastThreadLocalStateCleaner.isEnabled()) {
5455
LOG.info("Cleaning up FastThreadLocal thread local state.");
5556
CLEANER.cleanupAllFastThreadLocals((thread, value) -> {

‎buildtools/src/main/java/org/apache/pulsar/tests/MockitoCleanupListener.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,11 @@
1818
*/
1919
package org.apache.pulsar.tests;
2020

21+
import java.util.List;
2122
import org.mockito.Mockito;
2223
import org.slf4j.Logger;
2324
import org.slf4j.LoggerFactory;
24-
import org.testng.IClass;
25+
import org.testng.ITestClass;
2526

2627
/**
2728
* Cleanup Mockito's Thread Local state that leaks memory
@@ -40,7 +41,7 @@ public class MockitoCleanupListener extends BetweenTestClassesListenerAdapter {
4041
"Cleaning up Mockito's ThreadSafeMockingProgress.MOCKING_PROGRESS_PROVIDER thread local state.";
4142

4243
@Override
43-
protected void onBetweenTestClasses(IClass testClass) {
44+
protected void onBetweenTestClasses(List<ITestClass> testClasses) {
4445
if (MOCKITO_CLEANUP_ENABLED) {
4546
try {
4647
if (MockitoThreadLocalStateCleaner.INSTANCE.isEnabled()) {

‎buildtools/src/main/java/org/apache/pulsar/tests/SingletonCleanerListener.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,11 @@
2121

2222
import java.lang.reflect.InvocationTargetException;
2323
import java.lang.reflect.Method;
24+
import java.util.List;
2425
import org.apache.commons.lang3.ClassUtils;
2526
import org.slf4j.Logger;
2627
import org.slf4j.LoggerFactory;
27-
import org.testng.IClass;
28+
import org.testng.ITestClass;
2829

2930
/**
3031
* This TestNG listener contains cleanup for some singletons or caches.
@@ -77,7 +78,7 @@ public class SingletonCleanerListener extends BetweenTestClassesListenerAdapter
7778
}
7879

7980
@Override
80-
protected void onBetweenTestClasses(IClass testClass) {
81+
protected void onBetweenTestClasses(List<ITestClass> testClasses) {
8182
objectMapperFactoryClearCaches();
8283
jsonSchemaClearCaches();
8384
}

‎buildtools/src/main/java/org/apache/pulsar/tests/ThreadLeakDetectorListener.java‎

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.time.format.DateTimeFormatter;
3030
import java.util.Collections;
3131
import java.util.LinkedHashSet;
32+
import java.util.List;
3233
import java.util.Objects;
3334
import java.util.Set;
3435
import java.util.concurrent.ForkJoinWorkerThread;
@@ -37,9 +38,9 @@
3738
import org.apache.commons.lang3.mutable.MutableBoolean;
3839
import org.slf4j.Logger;
3940
import org.slf4j.LoggerFactory;
40-
import org.testng.IClass;
4141
import org.testng.ISuite;
4242
import org.testng.ISuiteListener;
43+
import org.testng.ITestClass;
4344

4445
/**
4546
* Detects new threads that have been created during the test execution. This is useful to detect thread leaks.
@@ -76,21 +77,42 @@ public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapte
7677
@Override
7778
public void onStart(ISuite suite) {
7879
// capture the initial set of threads
79-
detectLeakedThreads(null);
80+
detectLeakedThreads(Collections.emptyList());
8081
}
8182

8283
@Override
83-
protected void onBetweenTestClasses(IClass testClass) {
84-
detectLeakedThreads(testClass.getRealClass());
84+
protected void onBetweenTestClasses(List<ITestClass> testClasses) {
85+
detectLeakedThreads(testClasses);
8586
}
8687

87-
private void detectLeakedThreads(Class<?> endedTestClass) {
88+
private static String joinTestClassNames(List<ITestClass> testClasses) {
89+
return testClasses.stream()
90+
.map(ITestClass::getRealClass)
91+
.map(Class::getName)
92+
.collect(Collectors.joining(", "));
93+
}
94+
95+
private static String joinSimpleTestClassNames(List<ITestClass> testClasses) {
96+
return testClasses.stream()
97+
.map(ITestClass::getRealClass)
98+
.map(Class::getSimpleName)
99+
.collect(Collectors.joining(", "));
100+
}
101+
102+
private static String firstTestClassName(List<ITestClass> testClasses) {
103+
return testClasses.stream()
104+
.findFirst()
105+
.orElseThrow()
106+
.getRealClass().getName();
107+
}
108+
109+
private void detectLeakedThreads(List<ITestClass> testClasses) {
88110
LOG.info("Capturing identifiers of running threads.");
89111
MutableBoolean differenceDetected = new MutableBoolean();
90112
Set<ThreadKey> currentThreadKeys =
91-
compareThreads(capturedThreadKeys, endedTestClass, WAIT_FOR_THREAD_TERMINATION_MILLIS <= 0,
113+
compareThreads(capturedThreadKeys, testClasses, WAIT_FOR_THREAD_TERMINATION_MILLIS <= 0,
92114
differenceDetected, null);
93-
if (WAIT_FOR_THREAD_TERMINATION_MILLIS > 0 && endedTestClass != null && differenceDetected.booleanValue()) {
115+
if (WAIT_FOR_THREAD_TERMINATION_MILLIS > 0 && !testClasses.isEmpty() && differenceDetected.booleanValue()) {
94116
LOG.info("Difference detected in active threads. Waiting up to {} ms for threads to terminate.",
95117
WAIT_FOR_THREAD_TERMINATION_MILLIS);
96118
long endTime = System.currentTimeMillis() + WAIT_FOR_THREAD_TERMINATION_MILLIS;
@@ -101,7 +123,7 @@ private void detectLeakedThreads(Class<?> endedTestClass) {
101123
Thread.currentThread().interrupt();
102124
}
103125
differenceDetected.setFalse();
104-
currentThreadKeys = compareThreads(capturedThreadKeys, endedTestClass, false, differenceDetected, null);
126+
currentThreadKeys = compareThreads(capturedThreadKeys, testClasses, false, differenceDetected, null);
105127
if (!differenceDetected.booleanValue()) {
106128
break;
107129
}
@@ -110,23 +132,24 @@ private void detectLeakedThreads(Class<?> endedTestClass) {
110132
String datetimePart =
111133
DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss.SSS").format(ZonedDateTime.now());
112134
PrintWriter out = null;
135+
String firstTestClassName = firstTestClassName(testClasses);
113136
try {
114137
if (!DUMP_DIR.exists()) {
115138
DUMP_DIR.mkdirs();
116139
}
117140
File threadleakdumpFile =
118-
new File(DUMP_DIR, "threadleak" + datetimePart + endedTestClass.getName() + ".txt");
141+
new File(DUMP_DIR, "threadleak" + datetimePart + firstTestClassName + ".txt");
119142
out = new PrintWriter(threadleakdumpFile);
120143
} catch (IOException e) {
121144
LOG.error("Cannot write thread leak dump", e);
122145
}
123-
currentThreadKeys = compareThreads(capturedThreadKeys, endedTestClass, true, null, out);
146+
currentThreadKeys = compareThreads(capturedThreadKeys, testClasses, true, null, out);
124147
if (out != null) {
125148
out.close();
126149
}
127150
if (COLLECT_THREADDUMP) {
128151
File threaddumpFile =
129-
new File(DUMP_DIR, "threaddump" + datetimePart + endedTestClass.getName() + ".txt");
152+
new File(DUMP_DIR, "threaddump" + datetimePart + firstTestClassName + ".txt");
130153
try {
131154
Files.asCharSink(threaddumpFile, Charsets.UTF_8)
132155
.write(ThreadDumpUtil.buildThreadDiagnosticString());
@@ -139,15 +162,15 @@ private void detectLeakedThreads(Class<?> endedTestClass) {
139162
capturedThreadKeys = currentThreadKeys;
140163
}
141164

142-
private static Set<ThreadKey> compareThreads(Set<ThreadKey> previousThreadKeys, Class<?> endedTestClass,
165+
private static Set<ThreadKey> compareThreads(Set<ThreadKey> previousThreadKeys, List<ITestClass> testClasses,
143166
boolean logDifference, MutableBoolean differenceDetected,
144167
PrintWriter out) {
145168
Set<ThreadKey> threadKeys = Collections.unmodifiableSet(ThreadUtils.getAllThreads().stream()
146169
.filter(thread -> !shouldSkipThread(thread))
147170
.map(ThreadKey::of)
148171
.collect(Collectors.<ThreadKey, Set<ThreadKey>>toCollection(LinkedHashSet::new)));
149172

150-
if (endedTestClass != null && previousThreadKeys != null) {
173+
if (!testClasses.isEmpty() && previousThreadKeys != null) {
151174
int newThreadsCounter = 0;
152175
for (ThreadKey threadKey : threadKeys) {
153176
if (!previousThreadKeys.contains(threadKey)) {
@@ -157,7 +180,7 @@ private static Set<ThreadKey> compareThreads(Set<ThreadKey> previousThreadKeys,
157180
}
158181
if (logDifference || out != null) {
159182
String message = String.format("Tests in class %s created thread id %d with name '%s'",
160-
endedTestClass.getSimpleName(),
183+
joinSimpleTestClassNames(testClasses),
161184
threadKey.getThreadId(), threadKey.getThreadName());
162185
if (logDifference) {
163186
LOG.warn(message);
@@ -171,7 +194,7 @@ private static Set<ThreadKey> compareThreads(Set<ThreadKey> previousThreadKeys,
171194
if (newThreadsCounter > 0 && (logDifference || out != null)) {
172195
String message = String.format(
173196
"Summary: Tests in class %s created %d new threads. There are now %d threads in total.",
174-
endedTestClass.getName(), newThreadsCounter, threadKeys.size());
197+
joinTestClassNames(testClasses), newThreadsCounter, threadKeys.size());
175198
if (logDifference) {
176199
LOG.warn(message);
177200
}

‎buildtools/src/test/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapterTest.java‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import java.util.stream.Collectors;
2828
import org.testng.Assert;
2929
import org.testng.IClass;
30+
import org.testng.ITestClass;
3031
import org.testng.ITestListener;
3132
import org.testng.ITestResult;
3233
import org.testng.TestNG;
@@ -143,14 +144,14 @@ private void runTestNGWithClasses(int expectedFailureCount, Class<?>... testClas
143144
XmlSuite suite = new XmlSuite();
144145
suite.setName("Programmatic Suite");
145146

146-
XmlTest test = new XmlTest(suite);
147-
test.setName("Programmatic Test");
148-
149-
List<XmlClass> xmlClasses = new ArrayList<>();
150147
for (Class<?> cls : testClasses) {
148+
// create a new XmlTest for each class so that this simulates the behavior of maven-surefire-plugin
149+
XmlTest test = new XmlTest(suite);
150+
test.setName("Programmatic Test for " + cls.getName());
151+
List<XmlClass> xmlClasses = new ArrayList<>();
151152
xmlClasses.add(new XmlClass(cls));
153+
test.setXmlClasses(xmlClasses);
152154
}
153-
test.setXmlClasses(xmlClasses);
154155

155156
List<XmlSuite> suites = new ArrayList<>();
156157
suites.add(suite);
@@ -174,16 +175,18 @@ public void onTestFailure(ITestResult result) {
174175

175176
// Test implementation of the abstract listener
176177
private class TestBetweenTestClassesListener extends BetweenTestClassesListenerAdapter {
177-
private final List<IClass> classesCalled = new ArrayList<>();
178+
private final List<ITestClass> classesCalled = new ArrayList<>();
178179

179180
@Override
180-
protected void onBetweenTestClasses(IClass testClass) {
181-
System.out.println("onBetweenTestClasses " + testClass.getName());
181+
protected void onBetweenTestClasses(List<ITestClass> testClasses) {
182+
assertEquals(testClasses.size(), 1);
183+
ITestClass testClass = testClasses.get(0);
184+
System.out.println("onBetweenTestClasses " + testClass);
182185
classesCalled.add(testClass);
183186
closeTestInstance(testClass);
184187
}
185188

186-
private void closeTestInstance(IClass testClass) {
189+
private void closeTestInstance(ITestClass testClass) {
187190
Arrays.stream(testClass.getInstances(false))
188191
.map(instance -> instance instanceof IParameterInfo
189192
? ((IParameterInfo) instance).getInstance() : instance)
@@ -198,7 +201,7 @@ private void closeTestInstance(IClass testClass) {
198201
});
199202
}
200203

201-
public List<IClass> getClassesCalled() {
204+
public List<ITestClass> getClassesCalled() {
202205
return classesCalled;
203206
}
204207

0 commit comments

Comments
 (0)