Merge pull request #1235 from krmahadevan/krmahadevan-fix-1232

Ensure unique listener injection in TestNG

.travis.yml

@@ -1,4 +1,9 @@
 language: java
+
+env:
+  global:
+    - GRADLE_OPTS=-Xmx512m
+
 jdk:
   - oraclejdk8
   - oraclejdk7

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-1232: Prevent TestNG from adding duplicate instances of the same listener (Krishnan Mahadevan)
 Fixed: GITHUB-1170: Fixing the test DataProviderTest.shouldNotThrowConcurrentModification (Krishnan Mahadevan)
 Fixed: GITHUB-1231: Make IExecutionListener implementation be the last reporter call before JVM exit(Krishnan Mahadevan)
 Fixed: GITHUB-1227: Prevent multiple instances of same Reporter from being injected into TestNG (Krishnan Mahadevan)

build.gradle

@@ -113,6 +113,7 @@ test {
 //    testLogging.showStandardStreams = true
     systemProperties = System.getProperties()
     systemProperties['test.resources.dir'] = 'build/resources/test/'
+	maxHeapSize = '1500m'
 }
 
 if (JavaVersion.current().isJava8Compatible()) {

src/main/java/org/testng/SuiteRunner.java

@@ -104,6 +104,12 @@ public SuiteRunner(IConfiguration configuration,
         null /* class listeners */);
   }
 
+  /**
+   * @deprecated - This constructor stands deprecated.
+   */
+  @Deprecated
+  //There are no external callers for this constructor but for TestNG. But since this method is a protected method
+  //we are following a proper deprecation strategy.
   protected SuiteRunner(IConfiguration configuration,
       XmlSuite suite,
       String outputDir,
@@ -117,15 +123,28 @@ protected SuiteRunner(IConfiguration configuration,
     init(configuration, suite, outputDir, runnerFactory, useDefaultListeners, methodInterceptors, invokedMethodListeners, testListeners, classListeners);
   }
 
+  protected SuiteRunner(IConfiguration configuration,
+      XmlSuite suite,
+      String outputDir,
+      ITestRunnerFactory runnerFactory,
+      boolean useDefaultListeners,
+      List<IMethodInterceptor> methodInterceptors,
+      Collection<IInvokedMethodListener> invokedMethodListeners,
+      Collection<ITestListener> testListeners,
+      Collection<IClassListener> classListeners)
+  {
+    init(configuration, suite, outputDir, runnerFactory, useDefaultListeners, methodInterceptors, invokedMethodListeners, testListeners, classListeners);
+  }
+
   private void init(IConfiguration configuration,
     XmlSuite suite,
     String outputDir,
     ITestRunnerFactory runnerFactory,
     boolean useDefaultListeners,
     List<IMethodInterceptor> methodInterceptors,
-    List<IInvokedMethodListener> invokedMethodListener,
-    List<ITestListener> testListeners,
-    List<IClassListener> classListeners)
+    Collection<IInvokedMethodListener> invokedMethodListener,
+    Collection<ITestListener> testListeners,
+    Collection<IClassListener> classListeners)
   {
     m_configuration = configuration;
     m_suite = suite;

src/main/java/org/testng/TestNG.java

@@ -134,10 +134,10 @@ public class TestNG {
   private ITestRunnerFactory m_testRunnerFactory;
 
   // These listeners can be overridden from the command line
-  private List<IClassListener> m_classListeners = Lists.newArrayList();
-  private List<ITestListener> m_testListeners = Lists.newArrayList();
-  private List<ISuiteListener> m_suiteListeners = Lists.newArrayList();
-  private Map<Class<? extends IReporter>, IReporter> m_reporters = Maps.newHashMap();
+  private final Map<Class<? extends IClassListener>, IClassListener> m_classListeners = Maps.newHashMap();
+  private final Map<Class<? extends ITestListener>, ITestListener> m_testListeners = Maps.newHashMap();
+  private final Map<Class<? extends ISuiteListener>, ISuiteListener> m_suiteListeners = Maps.newHashMap();
+  private final Map<Class<? extends IReporter>, IReporter> m_reporters = Maps.newHashMap();
 
   protected static final int HAS_FAILURE = 1;
   protected static final int HAS_SKIPPED = 2;
@@ -162,7 +162,8 @@ public class TestNG {
 
   private ITestObjectFactory m_objectFactory;
 
-  private List<IInvokedMethodListener> m_invokedMethodListeners = Lists.newArrayList();
+  private final Map<Class<? extends IInvokedMethodListener>, IInvokedMethodListener> m_invokedMethodListeners = Maps
+      .newHashMap();
 
   private Integer m_dataProviderThreadCount = null;
 
@@ -178,9 +179,8 @@ public class TestNG {
   protected long m_end;
   protected long m_start;
 
-  private List<IExecutionListener> m_executionListeners = Lists.newArrayList();
-
-  private List<IAlterSuiteListener> m_alterSuiteListeners= Lists.newArrayList();
+  private final Map<Class<? extends IExecutionListener>, IExecutionListener> m_executionListeners = Maps.newHashMap();
+  private final Map<Class<? extends IAlterSuiteListener>, IAlterSuiteListener> m_alterSuiteListeners= Maps.newHashMap();
 
   private boolean m_isInitialized = false;
 
@@ -712,22 +712,33 @@ public void addListener(Object listener) {
     addListener((ITestNGListener) listener);
   }
 
+  private static <E> void maybeAddListener(Map<Class<? extends E>, E> map, Class<? extends E> type, E value) {
+    if (map.containsKey(value.getClass())) {
+      LOGGER.warn("Ignoring duplicate listener : " + value.getClass().getName());
+    } else {
+      map.put(type, value);
+    }
+  }
+
   public void addListener(ITestNGListener listener) {
     if (listener == null) {
       return;
     }
     if (listener instanceof ISuiteListener) {
-      m_suiteListeners.add((ISuiteListener) listener);
+      ISuiteListener suite = (ISuiteListener) listener;
+      maybeAddListener(m_suiteListeners, suite.getClass(),  suite);
     }
     if (listener instanceof ITestListener) {
-      m_testListeners.add((ITestListener) listener);
+      ITestListener test = (ITestListener) listener;
+      maybeAddListener(m_testListeners, test.getClass(), test);
     }
     if (listener instanceof IClassListener) {
-      m_classListeners.add((IClassListener) listener);
+      IClassListener clazz = (IClassListener) listener;
+      maybeAddListener(m_classListeners, clazz.getClass(), clazz);
     }
     if (listener instanceof IReporter) {
       IReporter reporter = (IReporter) listener;
-      m_reporters.put(reporter.getClass(), reporter);
+      maybeAddListener(m_reporters, reporter.getClass(), reporter);
     }
     if (listener instanceof IAnnotationTransformer) {
       setAnnotationTransformer((IAnnotationTransformer) listener);
@@ -736,7 +747,8 @@ public void addListener(ITestNGListener listener) {
       m_methodInterceptors.add((IMethodInterceptor) listener);
     }
     if (listener instanceof IInvokedMethodListener) {
-      m_invokedMethodListeners.add((IInvokedMethodListener) listener);
+      IInvokedMethodListener method = (IInvokedMethodListener) listener;
+      maybeAddListener(m_invokedMethodListeners, method.getClass(), method);
     }
     if (listener instanceof IHookable) {
       setHookable((IHookable) listener);
@@ -745,13 +757,15 @@ public void addListener(ITestNGListener listener) {
       setConfigurable((IConfigurable) listener);
     }
     if (listener instanceof IExecutionListener) {
-      m_executionListeners.add((IExecutionListener) listener);
+      IExecutionListener execution = (IExecutionListener) listener;
+      maybeAddListener(m_executionListeners, execution.getClass(), execution);
     }
     if (listener instanceof IConfigurationListener) {
       getConfiguration().addConfigurationListener((IConfigurationListener) listener);
     }
     if (listener instanceof IAlterSuiteListener) {
-      m_alterSuiteListeners.add((IAlterSuiteListener) listener);
+      IAlterSuiteListener alter = (IAlterSuiteListener) listener;
+      maybeAddListener(m_alterSuiteListeners, alter.getClass(), alter);
     }
   }
 
@@ -761,9 +775,7 @@ public void addListener(ITestNGListener listener) {
   // TODO remove later
   @Deprecated
   public void addListener(IInvokedMethodListener listener) {
-    if (!m_invokedMethodListeners.contains(listener)) {
-      addListener((ITestNGListener) listener);
-    }
+    addListener((ITestNGListener) listener);
   }
 
   /**
@@ -772,9 +784,7 @@ public void addListener(IInvokedMethodListener listener) {
   // TODO remove later
   @Deprecated
   public void addListener(ISuiteListener listener) {
-    if (!m_suiteListeners.contains(listener)) {
-      addListener((ITestNGListener) listener);
-    }
+    addListener((ITestNGListener) listener);
   }
 
   /**
@@ -783,9 +793,7 @@ public void addListener(ISuiteListener listener) {
   // TODO remove later
   @Deprecated
   public void addListener(ITestListener listener) {
-    if (!m_testListeners.contains(listener)) {
-      addListener((ITestNGListener) listener);
-    }
+    addListener((ITestNGListener) listener);
   }
 
   /**
@@ -794,9 +802,7 @@ public void addListener(ITestListener listener) {
   // TODO remove later
   @Deprecated
   public void addListener(IClassListener listener) {
-    if (!m_classListeners.contains(listener)) {
-      addListener((ITestNGListener) listener);
-    }
+    addListener((ITestNGListener) listener);
   }
 
   /**
@@ -805,9 +811,7 @@ public void addListener(IClassListener listener) {
   // TODO remove later
   @Deprecated
   public void addListener(IReporter listener) {
-    if (!m_reporters.containsValue(listener)) {
-      addListener((ITestNGListener) listener);
-    }
+    addListener((ITestNGListener) listener);
   }
 
   /**
@@ -816,9 +820,7 @@ public void addListener(IReporter listener) {
   // TODO remove later
   @Deprecated
   public void addInvokedMethodListener(IInvokedMethodListener listener) {
-    if (!m_invokedMethodListeners.contains(listener)) {
-      addListener((ITestNGListener) listener);
-    }
+    addListener((ITestNGListener) listener);
   }
 
   public Set<IReporter> getReporters() {
@@ -828,11 +830,11 @@ public Set<IReporter> getReporters() {
   }
 
   public List<ITestListener> getTestListeners() {
-    return m_testListeners;
+    return Lists.newArrayList(m_testListeners.values());
   }
 
   public List<ISuiteListener> getSuiteListeners() {
-    return m_suiteListeners;
+    return Lists.newArrayList(m_suiteListeners.values());
   }
 
   /** If m_verbose gets set, it will override the verbose setting in testng.xml */
@@ -936,7 +938,7 @@ private void addReporter(Class<? extends IReporter> r) {
   }
 
   private void initializeDefaultListeners() {
-    m_testListeners.add(new ExitCodeListener(this));
+    m_testListeners.put(ExitCodeListener.class, new ExitCodeListener(this));
 
     if (m_useDefaultListeners) {
       addReporter(SuiteHTMLReporter.class);
@@ -1132,17 +1134,17 @@ protected List<ISuite> runSuites() {
   }
 
   private void runSuiteAlterationListeners() {
-    for (List<IAlterSuiteListener> listeners
-        : Arrays.asList(m_alterSuiteListeners, m_configuration.getAlterSuiteListeners())) {
+    for (Collection<IAlterSuiteListener> listeners
+        : Arrays.asList(m_alterSuiteListeners.values(), m_configuration.getAlterSuiteListeners())) {
       for (IAlterSuiteListener l : listeners) {
         l.alter(m_suites);
       }
     }
   }
 
   private void runExecutionListeners(boolean start) {
-    for (List<IExecutionListener> listeners
-        : Arrays.asList(m_executionListeners, m_configuration.getExecutionListeners())) {
+    for (Collection<IExecutionListener> listeners
+        : Arrays.asList(m_executionListeners.values(), m_configuration.getExecutionListeners())) {
       for (IExecutionListener l : listeners) {
         if (start) l.onExecutionStart();
         else l.onExecutionFinish();
@@ -1368,11 +1370,11 @@ private SuiteRunner createSuiteRunner(XmlSuite xmlSuite) {
         m_testRunnerFactory,
         m_useDefaultListeners,
         m_methodInterceptors,
-        m_invokedMethodListeners,
-        m_testListeners,
-        m_classListeners);
+        m_invokedMethodListeners.values(),
+        m_testListeners.values(),
+        m_classListeners.values());
 
-    for (ISuiteListener isl : m_suiteListeners) {
+    for (ISuiteListener isl : m_suiteListeners.values()) {
       result.addListener(isl);
     }
 
@@ -2057,7 +2059,7 @@ public void setGroupByInstances(boolean b) {
   //
 
   private URLClassLoader m_serviceLoaderClassLoader;
-  private List<ITestNGListener> m_serviceLoaderListeners = Lists.newArrayList();
+  private Map<Class<? extends ITestNGListener>, ITestNGListener> m_serviceLoaderListeners = Maps.newHashMap();
 
   /*
    * Used to test ServiceClassLoader
@@ -2070,14 +2072,16 @@ public void setServiceLoaderClassLoader(URLClassLoader ucl) {
    * Used to test ServiceClassLoader
    */
   private void addServiceLoaderListener(ITestNGListener l) {
-    m_serviceLoaderListeners.add(l);
+    if (! m_serviceLoaderListeners.containsKey(l.getClass())) {
+      m_serviceLoaderListeners.put(l.getClass(), l);
+    }
   }
 
   /*
    * Used to test ServiceClassLoader
    */
   public List<ITestNGListener> getServiceLoaderListeners() {
-    return m_serviceLoaderListeners;
+    return Lists.newArrayList(m_serviceLoaderListeners.values());
   }
 
   //

src/test/java/test/reports/UniqueReporterInjectionTest.java

@@ -19,7 +19,9 @@ public void testPruningOfDuplicateReporter() {
         tng.setUseDefaultListeners(false);
         tng.addListener((ITestNGListener) new ReporterListenerForIssue1227());
         tng.run();
-        Assert.assertEquals(tng.getReporters().size(),1);
+        //Since we have another reporting listener that is injected via the service loader file
+        //reporting listeners size will now have to be two.
+        Assert.assertEquals(tng.getReporters().size(),2);
         Assert.assertEquals(ReporterListenerForIssue1227.counter, 1);
     }
 

src/test/java/test/serviceloader/ServiceLoaderTest.java

@@ -43,7 +43,7 @@ public void serviceLoaderShouldWorkWithConfigurationListener() {
     TestNG tng = create(ServiceLoaderSampleTest.class);
     tng.run();
 
-    Assert.assertEquals(1, tng.getServiceLoaderListeners().size());
+    Assert.assertEquals(2, tng.getServiceLoaderListeners().size());
     ListenerAssert.assertListenerType(tng.getServiceLoaderListeners(), MyConfigurationListener.class);
   }
 }