Honour inheritance when parsing listener factories

CHANGES.txt

@@ -1,5 +1,6 @@
 Current (7.10.0)
 
+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)
 Fixed: GITHUB-3082: IInvokedMethodListener Iinvoked method does not return correct instance during @BeforeMethod, @AfterMethod and @AfterClass (Krishnan Mahadevan)

testng-core/src/main/java/org/testng/internal/TestListenerHelper.java

@@ -1,6 +1,9 @@
 package org.testng.internal;
 
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import org.testng.IClass;
 import org.testng.IConfigurationListener;
 import org.testng.ITestContext;
@@ -13,7 +16,6 @@
 import org.testng.ListenerComparator;
 import org.testng.TestNGException;
 import org.testng.annotations.IListenersAnnotation;
-import org.testng.collections.Lists;
 import org.testng.internal.annotations.IAnnotationFinder;
 
 /** A helper class that internally houses some of the listener related actions support. */
@@ -129,38 +131,16 @@ public static void runTestListeners(ITestResult tr, List<ITestListener> listener
    * @return all the @Listeners annotations found in the current class and its superclasses and
    *     inherited interfaces.
    */
-  @SuppressWarnings("unchecked")
   public static ListenerHolder findAllListeners(Class<?> cls, IAnnotationFinder finder) {
     ListenerHolder result = new ListenerHolder();
-    result.listenerClasses = Lists.newArrayList();
 
     while (cls != Object.class) {
       List<IListenersAnnotation> annotations =
           finder.findInheritedAnnotations(cls, IListenersAnnotation.class);
-      IListenersAnnotation l = finder.findAnnotation(cls, IListenersAnnotation.class);
-      if (l != null) {
-        annotations.add(l);
-      }
-      annotations.forEach(
-          anno -> {
-            Class<? extends ITestNGListener>[] classes = anno.getValue();
-            for (Class<? extends ITestNGListener> c : classes) {
-              result.listenerClasses.add(c);
-
-              if (ITestNGListenerFactory.class.isAssignableFrom(c)) {
-                if (result.listenerFactoryClass == null) {
-                  result.listenerFactoryClass = (Class<? extends ITestNGListenerFactory>) c;
-                } else {
-                  throw new TestNGException(
-                      "Found more than one class implementing "
-                          + "ITestNGListenerFactory:"
-                          + c
-                          + " and "
-                          + result.listenerFactoryClass);
-                }
-              }
-            }
-          });
+      Optional.ofNullable(finder.findAnnotation(cls, IListenersAnnotation.class))
+          .ifPresent(annotations::add);
+
+      annotations.stream().flatMap(it -> Arrays.stream(it.getValue())).forEach(result::addListener);
       cls = cls.getSuperclass();
     }
     return result;
@@ -196,9 +176,38 @@ public static ITestNGListenerFactory createListenerFactory(
   }
 
   public static class ListenerHolder {
-    private List<Class<? extends ITestNGListener>> listenerClasses;
+    private final List<Class<? extends ITestNGListener>> listenerClasses = new ArrayList<>();
     private Class<? extends ITestNGListenerFactory> listenerFactoryClass;
 
+    @SuppressWarnings("unchecked")
+    public void addListener(Class<? extends ITestNGListener> c) {
+      // @Listener annotation is now inheritable. So let's add it ONLY
+      // if it wasn't added already
+      if (!listenerClasses.contains(c)) {
+        listenerClasses.add(c);
+      }
+      if (ITestNGListenerFactory.class.isAssignableFrom(c)) {
+        setListenerFactoryClass((Class<? extends ITestNGListenerFactory>) c);
+      }
+    }
+
+    private void setListenerFactoryClass(Class<? extends ITestNGListenerFactory> c) {
+      if (c.equals(listenerFactoryClass)) {
+        return;
+      }
+      if (listenerFactoryClass != null) {
+        // Let's say we already know of a ListenerFactoryClass called `MyFactory`.
+        // Now we are stumbling into another ListenerFactoryClass called `YourFactory`
+        // We need to throw an exception, because we can ONLY deal with 1 listener factory.
+        throw new TestNGException(
+            "Found more than one class implementing ITestNGListenerFactory:"
+                + c
+                + " and "
+                + listenerFactoryClass);
+      }
+      listenerFactoryClass = c;
+    }
+
     public List<Class<? extends ITestNGListener>> getListenerClasses() {
       return listenerClasses;
     }

testng-core/src/test/java/org/testng/internal/TestListenerHelperTest.java

@@ -14,11 +14,11 @@
 import org.testng.internal.annotations.DefaultAnnotationTransformer;
 import org.testng.internal.annotations.IAnnotationFinder;
 import org.testng.internal.annotations.JDK15AnnotationFinder;
-import org.testng.internal.listeners.DummyListenerFactory;
+import org.testng.internal.listeners.ListenerFactoryContainer;
+import org.testng.internal.listeners.TestClassContainer;
 import org.testng.internal.listeners.TestClassDoublingupAsListenerFactory;
 import org.testng.internal.listeners.TestClassWithCompositeListener;
 import org.testng.internal.listeners.TestClassWithListener;
-import org.testng.internal.listeners.TestClassWithMultipleListenerFactories;
 import org.testng.internal.paramhandler.FakeTestContext;
 import org.testng.xml.XmlClass;
 import test.SimpleBaseTest;
@@ -45,13 +45,25 @@ public Object[][] getTestData() {
     };
   }
 
+  @Test(description = "GITHUB-3095")
+  public void testFindAllListenersDuplicateListenerFactories() {
+    TestListenerHelper.ListenerHolder result =
+        TestListenerHelper.findAllListeners(
+            TestClassContainer.TestClassWithDuplicateListenerFactories.class, finder);
+    assertThat(result.getListenerFactoryClass()).isNotNull();
+  }
+
   @Test(
+      description = "GITHUB-3095",
       expectedExceptions = TestNGException.class,
       expectedExceptionsMessageRegExp =
           "\nFound more than one class implementing ITestNGListenerFactory:class "
-              + "org.testng.internal.listeners.DummyListenerFactory and class org.testng.internal.listeners.DummyListenerFactory")
+              + "org.testng.internal.listeners.ListenerFactoryContainer\\$DummyListenerFactory2 "
+              + "and class org.testng.internal.listeners"
+              + ".ListenerFactoryContainer\\$DummyListenerFactory")
   public void testFindAllListenersErrorCondition() {
-    TestListenerHelper.findAllListeners(TestClassWithMultipleListenerFactories.class, finder);
+    TestListenerHelper.findAllListeners(
+        TestClassContainer.TestClassWithMultipleUniqueListenerFactories.class, finder);
   }
 
   @Test(dataProvider = "getFactoryTestData")
@@ -73,7 +85,7 @@ public void testCreateListenerFactory(
   @DataProvider(name = "getFactoryTestData")
   public Object[][] getFactoryTestData() {
     return new Object[][] {
-      {TestClassWithListener.class, DummyListenerFactory.class},
+      {TestClassWithListener.class, ListenerFactoryContainer.DummyListenerFactory.class},
       {TestClassDoublingupAsListenerFactory.class, TestClassDoublingupAsListenerFactory.class}
     };
   }

testng-core/src/test/java/org/testng/internal/listeners/ListenerFactoryContainer.java

@@ -0,0 +1,21 @@
+package org.testng.internal.listeners;
+
+import org.testng.IExecutionListener;
+import org.testng.ITestNGListener;
+import org.testng.ITestNGListenerFactory;
+
+public class ListenerFactoryContainer {
+  public static class DummyListenerFactory implements ITestNGListenerFactory, IExecutionListener {
+    @Override
+    public ITestNGListener createListener(Class<? extends ITestNGListener> listenerClass) {
+      return this;
+    }
+  }
+
+  public static class DummyListenerFactory2 implements ITestNGListenerFactory, IExecutionListener {
+    @Override
+    public ITestNGListener createListener(Class<? extends ITestNGListener> listenerClass) {
+      return this;
+    }
+  }
+}

testng-core/src/test/java/org/testng/internal/listeners/TestClassContainer.java

@@ -0,0 +1,17 @@
+package org.testng.internal.listeners;
+
+import org.testng.annotations.Listeners;
+
+public class TestClassContainer {
+  @Listeners({
+    ListenerFactoryContainer.DummyListenerFactory.class,
+    ListenerFactoryContainer.DummyListenerFactory.class
+  })
+  public static class TestClassWithDuplicateListenerFactories {}
+
+  @Listeners({
+    ListenerFactoryContainer.DummyListenerFactory.class,
+    ListenerFactoryContainer.DummyListenerFactory2.class
+  })
+  public static class TestClassWithMultipleUniqueListenerFactories {}
+}

testng-core/src/test/java/org/testng/internal/listeners/TestClassWithCompositeListener.java

@@ -3,7 +3,7 @@
 import org.testng.annotations.Listeners;
 import org.testng.annotations.Test;
 
-@Listeners(DummyListenerFactory.class)
+@Listeners(ListenerFactoryContainer.DummyListenerFactory.class)
 public class TestClassWithCompositeListener {
   @Test
   public void testMethod() {}

testng-core/src/test/java/test/listeners/ListenersTest.java

@@ -54,6 +54,7 @@
 import test.listeners.issue3064.SampleTestCase;
 import test.listeners.issue3082.ObjectRepository;
 import test.listeners.issue3082.ObjectTrackingMethodListener;
+import test.listeners.issue3095.ChildClassSample;
 
 public class ListenersTest extends SimpleBaseTest {
   public static final String[] github2638ExpectedList =
@@ -588,6 +589,13 @@ public void ensureListenerWorksWithCorrectTestClassInstance() {
     softly.assertAll();
   }
 
+  @Test(description = "GITHUB-3095")
+  public void ensureInheritanceIsHandledWhenDealingWithListeners() {
+    TestNG testng = create(ChildClassSample.class);
+    testng.run();
+    assertThat(testng.getStatus()).isZero();
+  }
+
   private void setupTest(boolean addExplicitListener) {
     TestNG testng = new TestNG();
     XmlSuite xmlSuite = createXmlSuite("Xml_Suite");

testng-core/src/test/java/test/listeners/issue3095/ChildClassSample.java

@@ -0,0 +1,8 @@
+package test.listeners.issue3095;
+
+import org.testng.annotations.Test;
+
+public class ChildClassSample extends SuperClassSample {
+  @Test
+  public void childTestMethod() {}
+}

testng-core/src/test/java/org/testng/internal/listeners/DummyListenerFactory.java → testng-core/src/test/java/test/listeners/issue3095/MyTestNgFactory.java

@@ -1,12 +1,11 @@
-package org.testng.internal.listeners;
+package test.listeners.issue3095;
 
-import org.testng.IExecutionListener;
 import org.testng.ITestNGListener;
 import org.testng.ITestNGListenerFactory;
 
-public class DummyListenerFactory implements ITestNGListenerFactory, IExecutionListener {
+public class MyTestNgFactory implements ITestNGListener, ITestNGListenerFactory {
   @Override
   public ITestNGListener createListener(Class<? extends ITestNGListener> listenerClass) {
-    return this;
+    return null;
   }
 }

testng-core/src/test/java/test/listeners/issue3095/SuperClassSample.java

@@ -0,0 +1,6 @@
+package test.listeners.issue3095;
+
+import org.testng.annotations.Listeners;
+
+@Listeners(MyTestNgFactory.class)
+public class SuperClassSample {}