Streamline Listener factory invocation (#3256)

Closes #3120

Author: krmahadevan

CHANGES.txt

@@ -1,4 +1,5 @@
 Current (7.13.0)
+Fixed: GITHUB-3120: ITestNGListenerFactory is broken and never invoked (Krishnan Mahadevan)
 
 7.12.0
 Fixed: GITHUB-3231: TestNG retry is going into infinite loop when the data provider returned object is modified before failure (Bartek Florczak)

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

@@ -12,7 +12,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -372,19 +371,20 @@ private void initListeners() {
       listenerClasses.addAll(listenerHolder.getListenerClasses());
     }
 
-    if (listenerFactoryClass == null) {
-      listenerFactoryClass = DefaultListenerFactory.class;
-    }
-
     //
     // Now we have all the listeners collected from @Listeners and at most one
     // listener factory collected from a class implementing ITestNGListenerFactory.
     // Instantiate all the requested listeners.
     //
 
-    ITestNGListenerFactory factory =
-        Optional.ofNullable(m_configuration.getListenerFactory())
-            .orElse(new DefaultListenerFactory(m_objectFactory, this));
+    ITestNGListenerFactory factory;
+    if (listenerFactoryClass != null) {
+      factory = m_objectFactory.newInstance(listenerFactoryClass);
+    } else if (m_configuration.getListenerFactory() != null) {
+      factory = m_configuration.getListenerFactory();
+    } else {
+      factory = new DefaultListenerFactory(m_objectFactory, this);
+    }
 
     // Instantiate all the listeners
     for (Class<? extends ITestNGListener> c : listenerClasses) {

testng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.java

@@ -3,10 +3,16 @@
 import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
 
 import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.testng.CommandLineArgs;
+import org.testng.ITestListener;
+import org.testng.ITestResult;
 import org.testng.TestNG;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import test.SimpleBaseTest;
+import test.listeners.factory.issue3120.FactoryListenerTestClassCombinedSample;
+import test.listeners.factory.issue3120.TestClassSample;
 
 public class TestNGFactoryTest extends SimpleBaseTest {
 
@@ -38,4 +44,27 @@ public void testListenerFactoryViaTestNGApi() {
     assertThat(testng.getStatus()).isZero();
     assertThat(factory.isInvoked()).isTrue();
   }
+
+  @Test(description = "GITHUB-3120", dataProvider = "dp-3120")
+  public void testListenerFactoryInvocationWhenCoupledAsListener(Class<?> testClass) {
+    TestNG testng = create(testClass);
+    AtomicInteger counter = new AtomicInteger(0);
+    testng.addListener(
+        new ITestListener() {
+          @Override
+          public void onTestFailure(ITestResult result) {
+            counter.incrementAndGet();
+          }
+        });
+    testng.run();
+    assertThat(testng.getStatus()).isZero();
+    assertThat(counter.get()).withFailMessage("No test should have failed").isEqualTo(0);
+  }
+
+  @DataProvider(name = "dp-3120")
+  public Object[][] dp3120() {
+    return new Object[][] {
+      {TestClassSample.class}, {FactoryListenerTestClassCombinedSample.class},
+    };
+  }
 }

testng-core/src/test/java/test/listeners/factory/issue3120/CustomFactory.java

@@ -0,0 +1,22 @@
+package test.listeners.factory.issue3120;
+
+import org.testng.IExecutionListener;
+import org.testng.ITestNGListener;
+import org.testng.ITestNGListenerFactory;
+
+public class CustomFactory implements IExecutionListener, ITestNGListenerFactory {
+
+  public static boolean factoryInvoked = false;
+  public static boolean listenerInvoked = false;
+
+  @Override
+  public ITestNGListener createListener(Class<? extends ITestNGListener> aClass) {
+    factoryInvoked = true;
+    return this;
+  }
+
+  @Override
+  public void onExecutionStart() {
+    listenerInvoked = true;
+  }
+}

testng-core/src/test/java/test/listeners/factory/issue3120/FactoryListenerTestClassCombinedSample.java

@@ -0,0 +1,33 @@
+package test.listeners.factory.issue3120;
+
+import org.testng.Assert;
+import org.testng.IExecutionListener;
+import org.testng.ITestNGListener;
+import org.testng.ITestNGListenerFactory;
+import org.testng.annotations.Listeners;
+import org.testng.annotations.Test;
+
+@Listeners(FactoryListenerTestClassCombinedSample.class)
+public class FactoryListenerTestClassCombinedSample
+    implements IExecutionListener, ITestNGListenerFactory {
+
+  private static boolean factoryInvoked = false;
+  private static boolean listenerInvoked = false;
+
+  @Override
+  public ITestNGListener createListener(Class<? extends ITestNGListener> listenerClass) {
+    factoryInvoked = true;
+    return this;
+  }
+
+  @Override
+  public void onExecutionStart() {
+    listenerInvoked = true;
+  }
+
+  @Test
+  public void sampleTestMethod() {
+    Assert.assertTrue(factoryInvoked, "Factory should have been invoked");
+    Assert.assertTrue(listenerInvoked, "Listener should have been invoked");
+  }
+}

testng-core/src/test/java/test/listeners/factory/issue3120/TestClassSample.java

@@ -0,0 +1,15 @@
+package test.listeners.factory.issue3120;
+
+import org.testng.Assert;
+import org.testng.annotations.Listeners;
+import org.testng.annotations.Test;
+
+@Listeners(CustomFactory.class)
+public class TestClassSample {
+
+  @Test
+  public void sampleTestMethod() {
+    Assert.assertTrue(CustomFactory.factoryInvoked, "Factory should have been invoked");
+    Assert.assertTrue(CustomFactory.listenerInvoked, "Listener should have been invoked");
+  }
+}