This is an automated email from the ASF dual-hosted git repository. lhotari pushed a commit to branch branch-3.0 in repository https://gitbox.apache.org/repos/asf/pulsar.git
commit 335da2a8bd3c9e6efc1d2bfc0b47ce43414cccdb Author: Lari Hotari <[email protected]> AuthorDate: Tue May 6 21:16:41 2025 +0300 [fix][test] Fix TestNG BetweenTestClassesListenerAdapter listener (#24258) (cherry picked from commit 4d54e39503b8d26237e5c208f9fe4ad4244ec65a) --- buildtools/pom.xml | 2 + .../tests/BetweenTestClassesListenerAdapter.java | 90 +++-- .../tests/FastThreadLocalCleanupListener.java | 3 +- .../pulsar/tests/MockitoCleanupListener.java | 3 +- .../pulsar/tests/SingletonCleanerListener.java | 3 +- .../pulsar/tests/ThreadLeakDetectorListener.java | 17 +- .../BetweenTestClassesListenerAdapterTest.java | 392 +++++++++++++++++++++ 7 files changed, 483 insertions(+), 27 deletions(-) diff --git a/buildtools/pom.xml b/buildtools/pom.xml index 49443fcea60..116840b4795 100644 --- a/buildtools/pom.xml +++ b/buildtools/pom.xml @@ -66,6 +66,7 @@ --add-opens java.base/java.lang=ALL-UNNAMED <!--Mockito--> --add-opens java.base/jdk.internal.platform=ALL-UNNAMED <!--LinuxInfoUtils--> </test.additional.args> + <redirectTestOutputToFile>true</redirectTestOutputToFile> </properties> <dependencyManagement> @@ -189,6 +190,7 @@ <argLine> ${test.additional.args} </argLine> + <redirectTestOutputToFile>${redirectTestOutputToFile}</redirectTestOutputToFile> </configuration> <dependencies> <dependency> 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 8463e8519a0..370ec7aa377 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapter.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapter.java @@ -18,43 +18,89 @@ */ package org.apache.pulsar.tests; +import java.util.Arrays; +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 for detecting when execution finishes in previous - * test class and starts in a new class. + * 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. */ -abstract class BetweenTestClassesListenerAdapter implements IClassListener, ITestListener { - Class<?> lastTestClass; +abstract class BetweenTestClassesListenerAdapter implements IClassListener, IConfigurationListener { + private static final Logger log = LoggerFactory.getLogger(BetweenTestClassesListenerAdapter.class); + volatile Class<?> currentTestClass; + volatile int remainingAfterClassMethodCount; @Override - public void onBeforeClass(ITestClass testClass) { - checkIfTestClassChanged(testClass.getRealClass()); + 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(); + } } - private void checkIfTestClassChanged(Class<?> testClazz) { - if (lastTestClass != testClazz) { - onBetweenTestClasses(lastTestClass, testClazz); - lastTestClass = testClazz; - } + @Override + public final void onAfterClass(ITestClass testClass) { + handleAfterClassMethodCalled(testClass); + } + + @Override + public final void onConfigurationSuccess(ITestResult tr) { + handleAfterClassConfigurationMethodCompletion(tr); } @Override - public void onFinish(ITestContext context) { - if (lastTestClass != null) { - onBetweenTestClasses(lastTestClass, null); - lastTestClass = null; + 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()); } } /** - * Call back hook for adding logic when test execution moves from test class to another. - * - * @param endedTestClass the test class which has finished execution. null if the started test class is the first - * @param startedTestClass the test class which has started execution. null if the ended test class is the last + * Call back hook for adding logic when test execution has completely finished for a test class. */ - protected abstract void onBetweenTestClasses(Class<?> endedTestClass, Class<?> startedTestClass); + protected abstract void onBetweenTestClasses(IClass testClass); } 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 2cd2f579279..d1d205982d8 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/FastThreadLocalCleanupListener.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/FastThreadLocalCleanupListener.java @@ -20,6 +20,7 @@ package org.apache.pulsar.tests; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.testng.IClass; /** * Cleanup Thread Local state attach to Netty's FastThreadLocal. @@ -48,7 +49,7 @@ public class FastThreadLocalCleanupListener extends BetweenTestClassesListenerAd }); @Override - protected void onBetweenTestClasses(Class<?> endedTestClass, Class<?> startedTestClass) { + protected void onBetweenTestClasses(IClass testClass) { 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 1796d457d09..b5a3dbe8e4c 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/MockitoCleanupListener.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/MockitoCleanupListener.java @@ -21,6 +21,7 @@ package org.apache.pulsar.tests; import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.testng.IClass; /** * Cleanup Mockito's Thread Local state that leaks memory @@ -39,7 +40,7 @@ public class MockitoCleanupListener extends BetweenTestClassesListenerAdapter { "Cleaning up Mockito's ThreadSafeMockingProgress.MOCKING_PROGRESS_PROVIDER thread local state."; @Override - protected void onBetweenTestClasses(Class<?> endedTestClass, Class<?> startedTestClass) { + protected void onBetweenTestClasses(IClass testClass) { 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 532361d2bdb..8db6c095566 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/SingletonCleanerListener.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/SingletonCleanerListener.java @@ -24,6 +24,7 @@ import java.lang.reflect.Method; import org.apache.commons.lang3.ClassUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.testng.IClass; /** * This TestNG listener contains cleanup for some singletons or caches. @@ -76,7 +77,7 @@ public class SingletonCleanerListener extends BetweenTestClassesListenerAdapter } @Override - protected void onBetweenTestClasses(Class<?> endedTestClass, Class<?> startedTestClass) { + protected void onBetweenTestClasses(IClass testClass) { 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 803d1c4980b..6ab2e3440cb 100644 --- a/buildtools/src/main/java/org/apache/pulsar/tests/ThreadLeakDetectorListener.java +++ b/buildtools/src/main/java/org/apache/pulsar/tests/ThreadLeakDetectorListener.java @@ -26,17 +26,30 @@ import java.util.stream.Collectors; import org.apache.commons.lang3.ThreadUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.testng.IClass; +import org.testng.ISuite; +import org.testng.ISuiteListener; /** * Detects new threads that have been created during the test execution. */ -public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapter { +public class ThreadLeakDetectorListener extends BetweenTestClassesListenerAdapter implements ISuiteListener { private static final Logger LOG = LoggerFactory.getLogger(ThreadLeakDetectorListener.class); private Set<ThreadKey> capturedThreadKeys; @Override - protected void onBetweenTestClasses(Class<?> endedTestClass, Class<?> startedTestClass) { + public void onStart(ISuite suite) { + // capture the initial set of threads + detectLeakedThreads(null); + } + + @Override + protected void onBetweenTestClasses(IClass testClass) { + detectLeakedThreads(testClass.getRealClass()); + } + + private void detectLeakedThreads(Class<?> endedTestClass) { LOG.info("Capturing identifiers of running threads."); capturedThreadKeys = compareThreads(capturedThreadKeys, endedTestClass); } diff --git a/buildtools/src/test/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapterTest.java b/buildtools/src/test/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapterTest.java new file mode 100644 index 00000000000..c7467b206a5 --- /dev/null +++ b/buildtools/src/test/java/org/apache/pulsar/tests/BetweenTestClassesListenerAdapterTest.java @@ -0,0 +1,392 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.tests; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; +import org.testng.Assert; +import org.testng.IClass; +import org.testng.ITestListener; +import org.testng.ITestResult; +import org.testng.TestNG; +import org.testng.annotations.AfterClass; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Factory; +import org.testng.annotations.Test; +import org.testng.internal.IParameterInfo; +import org.testng.xml.XmlClass; +import org.testng.xml.XmlSuite; +import org.testng.xml.XmlTest; + +public class BetweenTestClassesListenerAdapterTest { + private TestBetweenTestClassesListener listener; + + @BeforeMethod + public void setUp() { + listener = new TestBetweenTestClassesListener(); + listener.reset(); + } + + @Test + public void testListenerWithNoAfterClassMethods() { + runTestNGWithClass(NoAfterClassMethods.class); + verifyListenerCalledForClass(NoAfterClassMethods.class); + } + + @Test + public void testListenerWithOneAfterClassMethod() { + runTestNGWithClass(OneAfterClassMethod.class); + verifyListenerCalledForClass(OneAfterClassMethod.class); + } + + @Test + public void testListenerWithMultipleAfterClassMethods() { + runTestNGWithClass(MultipleAfterClassMethods.class); + verifyListenerCalledForClass(MultipleAfterClassMethods.class); + } + + @Test + public void testListenerWithDisabledAfterClassMethod() { + runTestNGWithClass(DisabledAfterClassMethod.class); + verifyListenerCalledForClass(DisabledAfterClassMethod.class); + } + + @Test + public void testListenerWithFailingAfterClassMethods() { + runTestNGWithClass(FailingAfterClassMethod.class); + verifyListenerCalledForClass(FailingAfterClassMethod.class); + } + + @Test + public void testListenerWithTimeoutAndAfterClassMethod() { + runTestNGWithClasses(1, TimeoutAndAfterClassMethod.class); + verifyListenerCalledForClass(TimeoutAndAfterClassMethod.class); + } + + @Test + public void testListenerWithFactoryMethod() { + runTestNGWithClass(FactoryMethodCase.class); + String className = FactoryMethodCase.class.getName(); + assertEquals(1, listener.getClassesCalled().size(), + "Listener should be called exactly once"); + assertEquals(className, listener.getClassesCalled().get(0).getName(), + "Listener should be called for the correct class"); + } + + @Test + public void testListenerWithFactoryMethodWithoutAfterClassMethods() { + runTestNGWithClass(FactoryMethodCaseWithoutAfterClass.class); + String className = FactoryMethodCaseWithoutAfterClass.class.getName(); + assertEquals(1, listener.getClassesCalled().size(), + "Listener should be called exactly once"); + assertEquals(className, listener.getClassesCalled().get(0).getName(), + "Listener should be called for the correct class"); + } + + @Test + public void testListenerWithMultipleTestClasses() { + runTestNGWithClasses(0, + NoAfterClassMethods.class, + OneAfterClassMethod.class, + FailingAfterClassMethod.class, + MultipleAfterClassMethods.class); + + assertEquals(4, listener.getClassesCalled().size()); + + List<String> actualClassNames = listener.getClassesCalled().stream() + .map(IClass::getName) + .collect(Collectors.toList()); + + assertTrue(actualClassNames.contains(NoAfterClassMethods.class.getName())); + assertTrue(actualClassNames.contains(OneAfterClassMethod.class.getName())); + assertTrue(actualClassNames.contains(MultipleAfterClassMethods.class.getName())); + assertTrue(actualClassNames.contains(FailingAfterClassMethod.class.getName())); + } + + private void verifyListenerCalledForClass(Class<?> clazz) { + String className = clazz.getName(); + assertEquals(1, listener.getClassesCalled().size(), + "Listener should be called exactly once"); + assertEquals(className, listener.getClassesCalled().get(0).getName(), + "Listener should be called for the correct class"); + } + + private void runTestNGWithClass(Class<?> testClass) { + runTestNGWithClasses(0, testClass); + } + + // programmatically test TestNG listener with some classes + private void runTestNGWithClasses(int expectedFailureCount, Class<?>... testClasses) { + 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) { + xmlClasses.add(new XmlClass(cls)); + } + test.setXmlClasses(xmlClasses); + + List<XmlSuite> suites = new ArrayList<>(); + suites.add(suite); + + TestNG tng = new TestNG(); + tng.setXmlSuites(suites); + tng.addListener(listener); + tng.addListener(new TestLoggingListener()); + // set verbose output for debugging + tng.setVerbose(2); + AtomicInteger failureCounter = new AtomicInteger(); + tng.addListener(new ITestListener() { + @Override + public void onTestFailure(ITestResult result) { + failureCounter.incrementAndGet(); + } + }); + tng.run(); + assertEquals(failureCounter.get(), expectedFailureCount, "TestNG run should complete successfully"); + } + + // Test implementation of the abstract listener + private class TestBetweenTestClassesListener extends BetweenTestClassesListenerAdapter { + private final List<IClass> classesCalled = new ArrayList<>(); + + @Override + protected void onBetweenTestClasses(IClass testClass) { + System.out.println("onBetweenTestClasses " + testClass.getName()); + classesCalled.add(testClass); + closeTestInstance(testClass); + } + + private void closeTestInstance(IClass testClass) { + Arrays.stream(testClass.getInstances(false)) + .map(instance -> instance instanceof IParameterInfo + ? ((IParameterInfo) instance).getInstance() : instance) + .filter(AutoCloseable.class::isInstance) + .map(AutoCloseable.class::cast) + .forEach(autoCloseable -> { + try { + autoCloseable.close(); + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + } + + public List<IClass> getClassesCalled() { + return classesCalled; + } + + public void reset() { + classesCalled.clear(); + } + } + + private static class TestLoggingListener implements ITestListener { + @Override + public void onTestStart(ITestResult result) { + System.out.println("Test started: " + result.getName() + " in instance " + result.getInstance()); + } + } + + public static class TestRetryAnalyzer extends RetryAnalyzer { + public TestRetryAnalyzer() { + // retry once + setCount(1); + } + } + + private static class CloseableBase implements AutoCloseable { + protected boolean closed; + + protected void checkNotClosed() { + Assert.assertFalse(closed); + } + + public void close() { + closed = true; + } + } + + private static class Base extends CloseableBase { + int counter = 0; + + protected void failOnFirstExecution() { + if (counter++ == 0) { + throw new IllegalStateException("Simulated failure"); + } + } + + @Test(retryAnalyzer = TestRetryAnalyzer.class) + public void testMethod() { + checkNotClosed(); + failOnFirstExecution(); + } + + @AfterMethod(alwaysRun = true) + public void afterMethodInBase() { + checkNotClosed(); + } + + @AfterClass(alwaysRun = true) + public void afterClassInBase() { + checkNotClosed(); + } + } + + private static class NoAfterClassMethods extends Base { + + } + + private static class OneAfterClassMethod extends Base { + @AfterClass + public void afterClass() { + checkNotClosed(); + } + } + + private static class TimeoutAndAfterClassMethod extends Base { + @Override + @Test(timeOut = 100) + public void testMethod() { + checkNotClosed(); + try { + Thread.sleep(300); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + + @AfterClass(alwaysRun = true) + public void afterClass() { + checkNotClosed(); + } + } + + private static class MultipleAfterClassMethods extends Base { + private static final AtomicInteger afterClassCounter = new AtomicInteger(0); + + @AfterClass + public void afterClass1() { + checkNotClosed(); + afterClassCounter.incrementAndGet(); + } + + @AfterClass + public void afterClass2() { + checkNotClosed(); + afterClassCounter.incrementAndGet(); + } + } + + private static class DisabledAfterClassMethod extends Base { + @AfterClass(enabled = false) + public void disabledAfterClass() {} + } + + private static class FailingAfterClassMethod extends Base { + @AfterClass + public void failingAfterClass() { + checkNotClosed(); + throw new RuntimeException("Simulated failure"); + } + } + + protected static class FactoryMethodCase extends Base { + private final int id; + + private FactoryMethodCase(int id) { + this.id = id; + } + + @Factory + public static Object[] createTestInstances() { + return new Object[]{ + new FactoryMethodCase(1), + new FactoryMethodCase(2) + }; + } + + @DataProvider + public Object[] idDataProvider() { + return new Object[]{ id }; + } + + @Test(dataProvider = "idDataProvider") + public void testWithDataProvider(int id) { + checkNotClosed(); + Assert.assertEquals(this.id, id); + } + + @AfterClass + public void afterClass() { + checkNotClosed(); + } + + @Override + public String toString() { + return "FactoryMethodCase{" + + "id=" + id + + '}'; + } + } + + protected static class FactoryMethodCaseWithoutAfterClass extends CloseableBase { + private final int id; + + private FactoryMethodCaseWithoutAfterClass(int id) { + this.id = id; + } + + @Factory + public static Object[] createTestInstances() { + return new Object[]{ + new FactoryMethodCaseWithoutAfterClass(1), + new FactoryMethodCaseWithoutAfterClass(2) + }; + } + + @DataProvider + public Object[] idDataProvider() { + return new Object[]{ id }; + } + + @Test(dataProvider = "idDataProvider") + public void testWithDataProvider(int id) { + checkNotClosed(); + Assert.assertEquals(this.id, id); + } + + @Override + public String toString() { + return "FactoryMethodCaseWithoutAfterClass{" + + "id=" + id + + '}'; + } + } +} \ No newline at end of file
