Wire-In listeners consistently

Closes #2752

CHANGES.txt

@@ -1,6 +1,7 @@
 Current
 7.6.0
-New: GITHUB-2724: DataProvider: possibility to unload dataprovider class, when done with it (Dzmitry Sankouski)
+Fixed: GITHUB-2752: TestListener is being lost when implenting both IClassListener and ITestListener (Krishnan Mahadevan)
+New:   GITHUB-2724: DataProvider: possibility to unload dataprovider class, when done with it (Dzmitry Sankouski)
 Fixed: GITHUB-217: Configure TestNG to fail when there's a failure in data provider (Krishnan Mahadevan)
 Fixed: GITHUB-2743: SuiteRunner could not be initial by default Configuration (Nan Liang)
 Fixed: GITHUB-2729: beforeConfiguration() listener method should be invoked for skipped configurations as well(Nan Liang)

testng-core/src/main/java/org/testng/TestRunner.java

@@ -17,12 +17,14 @@
 import java.util.concurrent.PriorityBlockingQueue;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.Nonnull;
 import org.testng.collections.Lists;
 import org.testng.collections.Maps;
 import org.testng.collections.Sets;
 import org.testng.internal.Attributes;
+import org.testng.internal.ClassBasedWrapper;
 import org.testng.internal.ClassInfoMap;
 import org.testng.internal.ConfigurationGroupMethods;
 import org.testng.internal.DefaultListenerFactory;
@@ -374,11 +376,7 @@ private void initListeners() {
 
     // Instantiate all the listeners
     for (Class<? extends ITestNGListener> c : listenerClasses) {
-      if (IClassListener.class.isAssignableFrom(c) && m_classListeners.containsKey(c)) {
-        continue;
-      }
       ITestNGListener listener = factory.createListener(c);
-
       addListener(listener);
     }
   }
@@ -1119,20 +1117,11 @@ public List<ITestListener> getTestListeners() {
 
   @Override
   public List<IConfigurationListener> getConfigurationListeners() {
-    List<IConfigurationListener> listeners = Lists.newArrayList(m_configurationListeners);
-    for (IConfigurationListener each : this.m_configuration.getConfigurationListeners()) {
-      boolean duplicate = false;
-      for (IConfigurationListener listener : listeners) {
-        if (each.getClass().equals(listener.getClass())) {
-          duplicate = true;
-          break;
-        }
-      }
-      if (!duplicate) {
-        listeners.add(each);
-      }
-    }
-    return Lists.newArrayList(listeners);
+    return m_configurationListeners.stream()
+        .map(ClassBasedWrapper::wrap)
+        .distinct()
+        .map(ClassBasedWrapper::unWrap)
+        .collect(Collectors.toUnmodifiableList());
   }
 
   private void logFailedTest(ITestResult tr, boolean withinSuccessPercentage) {
@@ -1170,7 +1159,6 @@ void addTestListener(ITestListener listener) {
   }
 
   public void addListener(ITestNGListener listener) {
-    // TODO a listener may be added many times if it implements many interfaces
     if (listener instanceof IMethodInterceptor) {
       m_methodInterceptors.add((IMethodInterceptor) listener);
     }
@@ -1180,9 +1168,7 @@ public void addListener(ITestNGListener listener) {
     }
     if (listener instanceof IClassListener) {
       IClassListener classListener = (IClassListener) listener;
-      if (!m_classListeners.containsKey(classListener.getClass())) {
-        m_classListeners.put(classListener.getClass(), classListener);
-      }
+      m_classListeners.putIfAbsent(classListener.getClass(), classListener);
     }
     if (listener instanceof IConfigurationListener) {
       addConfigurationListener((IConfigurationListener) listener);

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

@@ -0,0 +1,33 @@
+package org.testng.internal;
+
+import java.util.Objects;
+
+public final class ClassBasedWrapper<T> {
+
+  private final T object;
+
+  private ClassBasedWrapper(T object) {
+    this.object = object;
+  }
+
+  public static <T> ClassBasedWrapper<T> wrap(T object) {
+    return new ClassBasedWrapper<>(object);
+  }
+
+  public T unWrap() {
+    return object;
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) return true;
+    if (o == null || getClass() != o.getClass()) return false;
+    ClassBasedWrapper<?> wrapper = (ClassBasedWrapper<?>) o;
+    return object.getClass().equals(wrapper.getClass());
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(object);
+  }
+}

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

@@ -1,6 +1,6 @@
 package test.listeners;
 
-import java.util.Arrays;
+import java.util.List;
 import org.testng.Assert;
 import org.testng.TestNG;
 import org.testng.annotations.Test;
@@ -11,7 +11,7 @@ public class ListenerInXmlTest extends SimpleBaseTest {
   @Test(description = "Make sure that listeners defined in testng.xml are invoked")
   public void listenerInXmlShouldBeInvoked() {
     TestNG tng = create();
-    tng.setTestSuites(Arrays.asList(getPathToResource("listener-in-xml.xml")));
+    tng.setTestSuites(List.of(getPathToResource("listener-in-xml.xml")));
     LListener.invoked = false;
     tng.run();
     Assert.assertTrue(LListener.invoked);

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

@@ -8,6 +8,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import org.assertj.core.api.Assertions;
 import org.testng.TestNG;
@@ -20,6 +21,8 @@
 import test.listeners.issue2638.TestClassBSample;
 import test.listeners.issue2685.InterruptedTestSample;
 import test.listeners.issue2685.SampleTestFailureListener;
+import test.listeners.issue2752.ListenerSample;
+import test.listeners.issue2752.TestClassSample;
 
 public class ListenersTest extends SimpleBaseTest {
 
@@ -67,6 +70,42 @@ public Object[][] getSuites() throws IOException {
     };
   }
 
+  @Test(description = "GITHUB-2752")
+  public void testWiringInOfListenersInMultipleTestTagsWithNoListenerInSuite() {
+    setupTest(false);
+    List<String> expected =
+        Arrays.asList(
+            "onStart", "onBeforeClass", "onTestStart", "onTestSuccess", "onAfterClass", "onFinish");
+    Map<String, List<String>> logs = ListenerSample.getLogs();
+    assertThat(logs.get("Xml_Test_1")).containsAll(expected);
+    assertThat(logs.get("Xml_Test_2")).containsAll(expected);
+  }
+
+  @Test(description = "GITHUB-2752")
+  public void testWiringInOfListenersInMultipleTestTagsWithListenerInSuite() {
+    setupTest(true);
+    List<String> expected =
+        Arrays.asList(
+            "onStart", "onBeforeClass", "onTestStart", "onTestSuccess", "onAfterClass", "onFinish");
+    Map<String, List<String>> logs = ListenerSample.getLogs();
+    assertThat(logs.get("Xml_Test_1")).containsAll(expected);
+    assertThat(logs.get("Xml_Test_2")).containsAll(expected);
+  }
+
+  private void setupTest(boolean addExplicitListener) {
+    TestNG testng = new TestNG();
+    XmlSuite xmlSuite = createXmlSuite("Xml_Suite");
+    createXmlTest(xmlSuite, "Xml_Test_1", TestClassSample.class);
+    createXmlTest(xmlSuite, "Xml_Test_2", TestClassSample.class);
+    testng.setXmlSuites(Collections.singletonList(xmlSuite));
+    testng.setVerbose(2);
+    if (addExplicitListener) {
+      ListenerSample listener = new ListenerSample();
+      testng.addListener(listener);
+    }
+    testng.run();
+  }
+
   private static Map<String, String[]> getExpectations() {
     Map<String, String[]> expected = new HashMap<>();
     expected.put("Container_Suite", new String[] {});

testng-core/src/test/java/test/listeners/issue2752/ListenerSample.java

@@ -0,0 +1,60 @@
+package test.listeners.issue2752;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import org.testng.IClassListener;
+import org.testng.ITestClass;
+import org.testng.ITestContext;
+import org.testng.ITestListener;
+import org.testng.ITestResult;
+
+public class ListenerSample implements IClassListener, ITestListener {
+
+  private static final Map<String, List<String>> logs = new ConcurrentHashMap<>();
+
+  public ListenerSample() {
+    logs.clear();
+  }
+
+  public static Map<String, List<String>> getLogs() {
+    return logs;
+  }
+
+  @Override
+  public void onBeforeClass(ITestClass testClass) {
+    String key = testClass.getXmlTest().getName();
+    logs.computeIfAbsent(key, k -> new ArrayList<>()).add("onBeforeClass");
+  }
+
+  @Override
+  public void onAfterClass(ITestClass testClass) {
+    String key = testClass.getXmlTest().getName();
+    logs.computeIfAbsent(key, k -> new ArrayList<>()).add("onAfterClass");
+  }
+
+  @Override
+  public void onTestStart(ITestResult result) {
+    String key = result.getTestContext().getName();
+    logs.computeIfAbsent(key, k -> new ArrayList<>()).add("onTestStart");
+  }
+
+  @Override
+  public void onTestSuccess(ITestResult result) {
+    String key = result.getTestContext().getName();
+    logs.computeIfAbsent(key, k -> new ArrayList<>()).add("onTestSuccess");
+  }
+
+  @Override
+  public void onFinish(ITestContext context) {
+    String key = context.getName();
+    logs.computeIfAbsent(key, k -> new ArrayList<>()).add("onFinish");
+  }
+
+  @Override
+  public void onStart(ITestContext context) {
+    String key = context.getName();
+    logs.computeIfAbsent(key, k -> new ArrayList<>()).add("onStart");
+  }
+}

testng-core/src/test/java/test/listeners/issue2752/TestClassSample.java

@@ -0,0 +1,15 @@
+package test.listeners.issue2752;
+
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Listeners;
+import org.testng.annotations.Test;
+
+@Listeners(ListenerSample.class)
+public class TestClassSample {
+
+  @Test
+  public void testMethod() {}
+
+  @BeforeClass
+  public void beforeClass() {}
+}