This is an automated email from the ASF dual-hosted git repository. lhotari pushed a commit to branch branch-3.3 in repository https://gitbox.apache.org/repos/asf/pulsar.git
commit 161bc96997f9493d401b5ed6cd39044c4429d1f1 Author: Lari Hotari <[email protected]> AuthorDate: Thu May 15 07:16:27 2025 +0300 [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 965ef5c14c93ca896ef4c8f34520066285fcf047) --- .../tests/BetweenTestClassesListenerAdapter.java | 85 ++++------------------ .../tests/FastThreadLocalCleanupListener.java | 5 +- .../pulsar/tests/MockitoCleanupListener.java | 5 +- .../pulsar/tests/SingletonCleanerListener.java | 5 +- .../pulsar/tests/ThreadLeakDetectorListener.java | 53 ++++++++++---- .../BetweenTestClassesListenerAdapterTest.java | 23 +++--- 6 files changed, 75 insertions(+), 101 deletions(-) diff --git a/buildtools/src/main/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapter.java b/buildtools/src/main/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapter.java index 370ec7aa377..cd2639b7bc6 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapter.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapter.java @@ -19,88 +19,33 @@ package org.apache.pulsar.tests; import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.testng.IClass; -import org.testng.IClassListener; -import org.testng.IConfigurationListener; import org.testng.ITestClass; +import org.testng.ITestContext; +import org.testng.ITestListener; import org.testng.ITestNGMethod; -import org.testng.ITestResult; /** - * TestNG listener adapter that detects when execution finishes in a test class, including AfterClass methods. - * TestNG's IClassListener.onAfterClass method is called before AfterClass methods are executed, - * which is why this solution is needed. + * TestNG listener adapter that detects when execution finishes for a test class, + * assuming that a single test class is run in each context. + * This is the case when running tests with maven-surefire-plugin. */ -abstract class BetweenTestClassesListenerAdapter implements IClassListener, IConfigurationListener { +abstract class BetweenTestClassesListenerAdapter implements ITestListener { private static final Logger log = LoggerFactory.getLogger(BetweenTestClassesListenerAdapter.class); - volatile Class<?> currentTestClass; - volatile int remainingAfterClassMethodCount; @Override - public final void onBeforeClass(ITestClass testClass) { - // for parameterized tests for the same class, the onBeforeClass method is called for each instance - // so we need to check if the test class is the same as for the previous call before resetting the counter - if (testClass.getRealClass() != currentTestClass) { - // find out how many parameterized instances of the test class are expected - Object[] instances = testClass.getInstances(false); - int instanceCount = instances != null && instances.length != 0 ? instances.length : 1; - // expect invocations of all annotated and enabled after class methods - int annotatedAfterClassMethodCount = (int) Arrays.stream(testClass.getAfterClassMethods()) - .filter(ITestNGMethod::getEnabled) - .count(); - // additionally expect invocations of the "onAfterClass" listener method in this class - int expectedMethodCountForEachInstance = 1 + annotatedAfterClassMethodCount; - // multiple by the number of instances - remainingAfterClassMethodCount = instanceCount * expectedMethodCountForEachInstance; - currentTestClass = testClass.getRealClass(); - } - } - - @Override - public final void onAfterClass(ITestClass testClass) { - handleAfterClassMethodCalled(testClass); - } - - @Override - public final void onConfigurationSuccess(ITestResult tr) { - handleAfterClassConfigurationMethodCompletion(tr); - } - - @Override - public final void onConfigurationSkip(ITestResult tr) { - handleAfterClassConfigurationMethodCompletion(tr); - } - - @Override - public final void onConfigurationFailure(ITestResult tr) { - handleAfterClassConfigurationMethodCompletion(tr); - } - - private void handleAfterClassConfigurationMethodCompletion(ITestResult tr) { - if (tr.getMethod().isAfterClassConfiguration() && !tr.wasRetried()) { - handleAfterClassMethodCalled(tr.getTestClass()); - } - } - - private void handleAfterClassMethodCalled(IClass testClass) { - if (currentTestClass != testClass.getRealClass()) { - log.error("Unexpected test class: {}. Expected: {}", testClass.getRealClass(), currentTestClass); - return; - } - remainingAfterClassMethodCount--; - if (remainingAfterClassMethodCount == 0) { - onBetweenTestClasses(testClass); - } else if (remainingAfterClassMethodCount < 0) { - // unexpected case, log it for easier debugging if this causes test failures - log.error("Remaining after class method count is negative: {} for test class: {}", - remainingAfterClassMethodCount, testClass.getRealClass()); - } + public final void onFinish(ITestContext context) { + List<ITestClass> testClasses = + Arrays.stream(context.getAllTestMethods()).map(ITestNGMethod::getTestClass).distinct() + .collect(Collectors.toList()); + onBetweenTestClasses(testClasses); } /** - * Call back hook for adding logic when test execution has completely finished for a test class. + * Call back hook for adding logic when test execution has completely finished for one or many test classes. */ - protected abstract void onBetweenTestClasses(IClass testClass); + protected abstract void onBetweenTestClasses(List<ITestClass> testClasses); } diff --git a/buildtools/src/main/java/org/apache/pulsar/tests/FastThreadLocalCleanupListener.java b/buildtools/src/main/java/org/apache/pulsar/tests/FastThreadLocalCleanupListener.java index d1d205982d8..bdabf327dbb 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/FastThreadLocalCleanupListener.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/FastThreadLocalCleanupListener.java @@ -18,9 +18,10 @@ */ package org.apache.pulsar.tests; +import java.util.List; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.testng.IClass; +import org.testng.ITestClass; /** * Cleanup Thread Local state attach to Netty's FastThreadLocal. @@ -49,7 +50,7 @@ public class FastThreadLocalCleanupListener extends BetweenTestClassesListenerAd }); @Override - protected void onBetweenTestClasses(IClass testClass) { + protected void onBetweenTestClasses(List<ITestClass> testClasses) { if (FAST_THREAD_LOCAL_CLEANUP_ENABLED && FastThreadLocalStateCleaner.isEnabled()) { LOG.info("Cleaning up FastThreadLocal thread local state."); CLEANER.cleanupAllFastThreadLocals((thread, value) -> { diff --git a/buildtools/src/main/java/org/apache/pulsar/tests/MockitoCleanupListener.java b/buildtools/src/main/java/org/apache/pulsar/tests/MockitoCleanupListener.java index b5a3dbe8e4c..ad8870a13e5 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/MockitoCleanupListener.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/MockitoCleanupListener.java @@ -18,10 +18,11 @@ */ package org.apache.pulsar.tests; +import java.util.List; import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.testng.IClass; +import org.testng.ITestClass; /** * Cleanup Mockito's Thread Local state that leaks memory @@ -40,7 +41,7 @@ public class MockitoCleanupListener extends BetweenTestClassesListenerAdapter { "Cleaning up Mockito's ThreadSafeMockingProgress.MOCKING_PROGRESS_PROVIDER thread local state."; @Override - protected void onBetweenTestClasses(IClass testClass) { + protected void onBetweenTestClasses(List<ITestClass> testClasses) { if (MOCKITO_CLEANUP_ENABLED) { try { if (MockitoThreadLocalStateCleaner.INSTANCE.isEnabled()) { diff --git a/buildtools/src/main/java/org/apache/pulsar/tests/SingletonCleanerListener.java b/buildtools/src/main/java/org/apache/pulsar/tests/SingletonCleanerListener.java index 8db6c095566..2260ad090d4 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/SingletonCleanerListener.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/SingletonCleanerListener.java @@ -21,10 +21,11 @@ package org.apache.pulsar.tests; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.util.List; import org.apache.commons.lang3.ClassUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.testng.IClass; +import org.testng.ITestClass; /** * This TestNG listener contains cleanup for some singletons or caches. @@ -77,7 +78,7 @@ public class SingletonCleanerListener extends BetweenTestClassesListenerAdapter } @Override - protected void onBetweenTestClasses(IClass testClass) { + protected void onBetweenTestClasses(List<ITestClass> testClasses) { objectMapperFactoryClearCaches(); jsonSchemaClearCaches(); } diff --git a/buildtools/src/main/java/org/apache/pulsar/tests/ThreadLeakDetectorListener.java b/buildtools/src/main/java/org/apache/pulsar/tests/ThreadLeakDetectorListener.java index 89a0e0b34f0..d28eabf238b 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/ThreadLeakDetectorListener.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/ThreadLeakDetectorListener.java @@ -29,6 +29,7 @@ import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import java.util.Collections; import java.util.LinkedHashSet; +import java.util.List; import java.util.Objects; import java.util.Set; import java.util.concurrent.ForkJoinWorkerThread; @@ -37,9 +38,9 @@ import org.apache.commons.lang3.ThreadUtils; import org.apache.commons.lang3.mutable.MutableBoolean; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.testng.IClass; import org.testng.ISuite; import org.testng.ISuiteListener; +import org.testng.ITestClass; /** * 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 @Override public void onStart(ISuite suite) { // capture the initial set of threads - detectLeakedThreads(null); + detectLeakedThreads(Collections.emptyList()); } @Override - protected void onBetweenTestClasses(IClass testClass) { - detectLeakedThreads(testClass.getRealClass()); + protected void onBetweenTestClasses(List<ITestClass> testClasses) { + detectLeakedThreads(testClasses); } - private void detectLeakedThreads(Class<?> endedTestClass) { + private static String joinTestClassNames(List<ITestClass> testClasses) { + return testClasses.stream() + .map(ITestClass::getRealClass) + .map(Class::getName) + .collect(Collectors.joining(", ")); + } + + private static String joinSimpleTestClassNames(List<ITestClass> testClasses) { + return testClasses.stream() + .map(ITestClass::getRealClass) + .map(Class::getSimpleName) + .collect(Collectors.joining(", ")); + } + + private static String firstTestClassName(List<ITestClass> testClasses) { + return testClasses.stream() + .findFirst() + .orElseThrow() + .getRealClass().getName(); + } + + private void detectLeakedThreads(List<ITestClass> testClasses) { LOG.info("Capturing identifiers of running threads."); MutableBoolean differenceDetected = new MutableBoolean(); Set<ThreadKey> currentThreadKeys = - compareThreads(capturedThreadKeys, endedTestClass, WAIT_FOR_THREAD_TERMINATION_MILLIS <= 0, + compareThreads(capturedThreadKeys, testClasses, WAIT_FOR_THREAD_TERMINATION_MILLIS <= 0, differenceDetected, null); - if (WAIT_FOR_THREAD_TERMINATION_MILLIS > 0 && endedTestClass != null && differenceDetected.booleanValue()) { + if (WAIT_FOR_THREAD_TERMINATION_MILLIS > 0 && !testClasses.isEmpty() && differenceDetected.booleanValue()) { LOG.info("Difference detected in active threads. Waiting up to {} ms for threads to terminate.", WAIT_FOR_THREAD_TERMINATION_MILLIS); long endTime = System.currentTimeMillis() + WAIT_FOR_THREAD_TERMINATION_MILLIS; @@ -101,7 +123,7 @@ public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapte Thread.currentThread().interrupt(); } differenceDetected.setFalse(); - currentThreadKeys = compareThreads(capturedThreadKeys, endedTestClass, false, differenceDetected, null); + currentThreadKeys = compareThreads(capturedThreadKeys, testClasses, false, differenceDetected, null); if (!differenceDetected.booleanValue()) { break; } @@ -110,23 +132,24 @@ public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapte String datetimePart = DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss.SSS").format(ZonedDateTime.now()); PrintWriter out = null; + String firstTestClassName = firstTestClassName(testClasses); try { if (!DUMP_DIR.exists()) { DUMP_DIR.mkdirs(); } File threadleakdumpFile = - new File(DUMP_DIR, "threadleak" + datetimePart + endedTestClass.getName() + ".txt"); + new File(DUMP_DIR, "threadleak" + datetimePart + firstTestClassName + ".txt"); out = new PrintWriter(threadleakdumpFile); } catch (IOException e) { LOG.error("Cannot write thread leak dump", e); } - currentThreadKeys = compareThreads(capturedThreadKeys, endedTestClass, true, null, out); + currentThreadKeys = compareThreads(capturedThreadKeys, testClasses, true, null, out); if (out != null) { out.close(); } if (COLLECT_THREADDUMP) { File threaddumpFile = - new File(DUMP_DIR, "threaddump" + datetimePart + endedTestClass.getName() + ".txt"); + new File(DUMP_DIR, "threaddump" + datetimePart + firstTestClassName + ".txt"); try { Files.asCharSink(threaddumpFile, Charsets.UTF_8) .write(ThreadDumpUtil.buildThreadDiagnosticString()); @@ -139,7 +162,7 @@ public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapte capturedThreadKeys = currentThreadKeys; } - private static Set<ThreadKey> compareThreads(Set<ThreadKey> previousThreadKeys, Class<?> endedTestClass, + private static Set<ThreadKey> compareThreads(Set<ThreadKey> previousThreadKeys, List<ITestClass> testClasses, boolean logDifference, MutableBoolean differenceDetected, PrintWriter out) { Set<ThreadKey> threadKeys = Collections.unmodifiableSet(ThreadUtils.getAllThreads().stream() @@ -147,7 +170,7 @@ public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapte .map(ThreadKey::of) .collect(Collectors.<ThreadKey, Set<ThreadKey>>toCollection(LinkedHashSet::new))); - if (endedTestClass != null && previousThreadKeys != null) { + if (!testClasses.isEmpty() && previousThreadKeys != null) { int newThreadsCounter = 0; for (ThreadKey threadKey : threadKeys) { if (!previousThreadKeys.contains(threadKey)) { @@ -157,7 +180,7 @@ public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapte } if (logDifference || out != null) { String message = String.format("Tests in class %s created thread id %d with name '%s'", - endedTestClass.getSimpleName(), + joinSimpleTestClassNames(testClasses), threadKey.getThreadId(), threadKey.getThreadName()); if (logDifference) { LOG.warn(message); @@ -171,7 +194,7 @@ public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapte if (newThreadsCounter > 0 && (logDifference || out != null)) { String message = String.format( "Summary: Tests in class %s created %d new threads. There are now %d threads in total.", - endedTestClass.getName(), newThreadsCounter, threadKeys.size()); + joinTestClassNames(testClasses), newThreadsCounter, threadKeys.size()); if (logDifference) { LOG.warn(message); } diff --git a/buildtools/src/test/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapterTest.java b/buildtools/src/test/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapterTest.java index c7467b206a5..c25f751d741 100644 --- a/buildtools/src/test/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapterTest.java +++ b/buildtools/src/test/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapterTest.java @@ -27,6 +27,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import org.testng.Assert; import org.testng.IClass; +import org.testng.ITestClass; import org.testng.ITestListener; import org.testng.ITestResult; import org.testng.TestNG; @@ -143,14 +144,14 @@ public class BetweenTestClassesListenerAdapterTest { XmlSuite suite = new XmlSuite(); suite.setName("Programmatic Suite"); - XmlTest test = new XmlTest(suite); - test.setName("Programmatic Test"); - - List<XmlClass> xmlClasses = new ArrayList<>(); for (Class<?> cls : testClasses) { + // create a new XmlTest for each class so that this simulates the behavior of maven-surefire-plugin + XmlTest test = new XmlTest(suite); + test.setName("Programmatic Test for " + cls.getName()); + List<XmlClass> xmlClasses = new ArrayList<>(); xmlClasses.add(new XmlClass(cls)); + test.setXmlClasses(xmlClasses); } - test.setXmlClasses(xmlClasses); List<XmlSuite> suites = new ArrayList<>(); suites.add(suite); @@ -174,16 +175,18 @@ public class BetweenTestClassesListenerAdapterTest { // Test implementation of the abstract listener private class TestBetweenTestClassesListener extends BetweenTestClassesListenerAdapter { - private final List<IClass> classesCalled = new ArrayList<>(); + private final List<ITestClass> classesCalled = new ArrayList<>(); @Override - protected void onBetweenTestClasses(IClass testClass) { - System.out.println("onBetweenTestClasses " + testClass.getName()); + protected void onBetweenTestClasses(List<ITestClass> testClasses) { + assertEquals(testClasses.size(), 1); + ITestClass testClass = testClasses.get(0); + System.out.println("onBetweenTestClasses " + testClass); classesCalled.add(testClass); closeTestInstance(testClass); } - private void closeTestInstance(IClass testClass) { + private void closeTestInstance(ITestClass testClass) { Arrays.stream(testClass.getInstances(false)) .map(instance -> instance instanceof IParameterInfo ? ((IParameterInfo) instance).getInstance() : instance) @@ -198,7 +201,7 @@ public class BetweenTestClassesListenerAdapterTest { }); } - public List<IClass> getClassesCalled() { + public List<ITestClass> getClassesCalled() { return classesCalled; }
