Allow ITestObjectFactory injection via listeners (#2677)

Closes #2676

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-2676: NPE is triggered when working with ITestObjectFactory (Krishnan Mahadevan)
 Fixed: GITHUB-2674: Run onTestSkipped for each value from data provider (Krishnan Mahadevan)
 Fixed: GITHUB-2672: Log real stacktrace when test times out. (cdalexndr)
 Fixed: GITHUB-2669: A failed retry with ITestContext will lose the ITestContext. (Nan Liang)

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

@@ -155,6 +155,9 @@ private void init(
           !configuration.getObjectFactory().getClass().equals(suite.getObjectFactoryClass());
       final ITestObjectFactory suiteObjectFactory;
       if (create) {
+        if (objectFactory == null) {
+          objectFactory = configuration.getObjectFactory();
+        }
         // Dont keep creating the object factory repeatedly since our current object factory
         // Was already created based off of a suite level object factory.
         suiteObjectFactory = objectFactory.newInstance(suite.getObjectFactoryClass());
@@ -166,27 +169,27 @@ private void init(
             @Override
             public <T> T newInstance(Class<T> cls, Object... parameters) {
               try {
-                return configuration.getObjectFactory().newInstance(cls, parameters);
-              } catch (Exception e) {
                 return suiteObjectFactory.newInstance(cls, parameters);
+              } catch (Exception e) {
+                return configuration.getObjectFactory().newInstance(cls, parameters);
               }
             }
 
             @Override
             public <T> T newInstance(String clsName, Object... parameters) {
               try {
-                return configuration.getObjectFactory().newInstance(clsName, parameters);
-              } catch (Exception e) {
                 return suiteObjectFactory.newInstance(clsName, parameters);
+              } catch (Exception e) {
+                return configuration.getObjectFactory().newInstance(clsName, parameters);
               }
             }
 
             @Override
             public <T> T newInstance(Constructor<T> constructor, Object... parameters) {
               try {
-                return configuration.getObjectFactory().newInstance(constructor, parameters);
-              } catch (Exception e) {
                 return suiteObjectFactory.newInstance(constructor, parameters);
+              } catch (Exception e) {
+                return configuration.getObjectFactory().newInstance(constructor, parameters);
               }
             }
           };

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

@@ -39,6 +39,7 @@
 import org.testng.internal.annotations.JDK15AnnotationFinder;
 import org.testng.internal.invokers.SuiteRunnerMap;
 import org.testng.internal.invokers.objects.GuiceContext;
+import org.testng.internal.objects.DefaultTestObjectFactory;
 import org.testng.internal.objects.Dispenser;
 import org.testng.internal.objects.IObjectDispenser;
 import org.testng.internal.objects.pojo.BasicAttributes;
@@ -169,7 +170,7 @@ public class TestNG {
 
   private final Set<XmlMethodSelector> m_selectors = Sets.newLinkedHashSet();
 
-  private static final ITestObjectFactory DEFAULT_OBJECT_FACTORY = new ITestObjectFactory() {};
+  private static final ITestObjectFactory DEFAULT_OBJECT_FACTORY = new DefaultTestObjectFactory();
   private ITestObjectFactory m_objectFactory = DEFAULT_OBJECT_FACTORY;
 
   private final Map<Class<? extends IInvokedMethodListener>, IInvokedMethodListener>

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

@@ -240,7 +240,7 @@ private void init(
     m_host = suite.getHost();
     m_testClassesFromXml = test.getXmlClasses();
     m_injectorFactory = m_configuration.getInjectorFactory();
-    m_objectFactory = m_configuration.getObjectFactory();
+    m_objectFactory = suite.getObjectFactory();
     setVerbose(test.getVerbose());
 
     boolean preserveOrder = test.getPreserveOrder();

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

@@ -10,6 +10,7 @@
 import org.testng.collections.Lists;
 import org.testng.collections.Objects;
 import org.testng.internal.annotations.IAnnotationFinder;
+import org.testng.internal.objects.DefaultTestObjectFactory;
 import org.testng.internal.objects.Dispenser;
 import org.testng.internal.objects.IObjectDispenser;
 import org.testng.internal.objects.pojo.BasicAttributes;
@@ -94,7 +95,11 @@ private Object getDefaultInstance(boolean create, String errMsgPrefix) {
       if (m_instance != null) {
         m_defaultInstance = m_instance;
       } else {
-        IObjectDispenser dispenser = Dispenser.newInstance(m_objectFactory);
+        ITestObjectFactory factory = m_objectFactory;
+        if (factory instanceof DefaultTestObjectFactory) {
+          factory = m_testContext.getSuite().getObjectFactory();
+        }
+        IObjectDispenser dispenser = Dispenser.newInstance(factory);
         BasicAttributes basic = new BasicAttributes(this, null);
         DetailedAttributes detailed = newDetailedAttributes(create, errMsgPrefix);
         CreationAttributes attributes = new CreationAttributes(m_testContext, basic, detailed);
@@ -118,7 +123,11 @@ public Object[] getInstances(boolean create, String errorMsgPrefix) {
       if (create) {
         DetailedAttributes ea = newDetailedAttributes(create, errorMsgPrefix);
         CreationAttributes attributes = new CreationAttributes(m_testContext, null, ea);
-        result = new Object[] {Dispenser.newInstance(m_objectFactory).dispense(attributes)};
+        result =
+            new Object[] {
+              Dispenser.newInstance(m_testContext.getSuite().getObjectFactory())
+                  .dispense(attributes)
+            };
       }
     }
     if (m_instances.size() > 0) {

testng-core/src/main/java/org/testng/internal/objects/DefaultTestObjectFactory.java

@@ -0,0 +1,12 @@
+package org.testng.internal.objects;
+
+import org.testng.ITestObjectFactory;
+
+/**
+ * Intended to be the default way of instantiating objects within TestNG. Intentionally does not
+ * provide any specific implementation because the interface already defines the default behavior.
+ * This class still exists to ensure that we dont use an anonymous object instantiation so that its
+ * easy to find out what type of object factory is being injected into our object dispensing
+ * mechanisms.
+ */
+public class DefaultTestObjectFactory implements ITestObjectFactory {}

testng-core/src/test/java/test/objectfactory/ObjectFactoryTest.java

@@ -0,0 +1,78 @@
+package test.objectfactory;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.testng.TestNG;
+import org.testng.TestNGException;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import org.testng.xml.XmlSuite;
+import test.SimpleBaseTest;
+import test.objectfactory.github1131.EmptyConstructorSample;
+import test.objectfactory.github1131.IntConstructorSample;
+import test.objectfactory.github1131.MyObjectFactory;
+import test.objectfactory.github1131.StringConstructorSample;
+import test.objectfactory.github1827.GitHub1827Sample;
+import test.objectfactory.issue2676.LocalSuiteAlteringListener;
+import test.objectfactory.issue2676.LoggingObjectFactorySample;
+import test.objectfactory.issue2676.TestClassSample;
+
+public class ObjectFactoryTest extends SimpleBaseTest {
+
+  @Test(description = "GITHUB-2676")
+  public void ensureObjectFactoryIsInvokedWhenAddedViaListeners() {
+    TestNG testng = create(TestClassSample.class);
+    testng.addListener(new LocalSuiteAlteringListener());
+    testng.run();
+    assertThat(LoggingObjectFactorySample.wasInvoked).isTrue();
+  }
+
+  @Test(
+      expectedExceptions = TestNGException.class,
+      expectedExceptionsMessageRegExp = ".*Check to make sure it can be instantiated",
+      description = "GITHUB-1827")
+  public void ensureExceptionThrownWhenNoSuitableConstructorFound() {
+
+    TestNG testng = create(GitHub1827Sample.class);
+    testng.run();
+  }
+
+  @Test(dataProvider = "dp", description = "GITHUB-1131")
+  public void factoryWithEmptyConstructorShouldWork(boolean bool) {
+    testFactory(bool, EmptyConstructorSample.class);
+    assertThat(MyObjectFactory.allParams).containsExactly(new Object[] {}, new Object[] {});
+  }
+
+  @Test(dataProvider = "dp", description = "GITHUB-1131")
+  public void factoryWithIntConstructorShouldWork(boolean bool) {
+    testFactory(bool, IntConstructorSample.class);
+    assertThat(MyObjectFactory.allParams).containsExactly(new Object[] {1}, new Object[] {2});
+  }
+
+  @Test(dataProvider = "dp", description = "GITHUB-1131")
+  public void factoryWithStringConstructorShouldWork(boolean bool) {
+    testFactory(bool, StringConstructorSample.class);
+    assertThat(MyObjectFactory.allParams)
+        .containsExactly(new Object[] {"foo"}, new Object[] {"bar"});
+  }
+
+  private void testFactory(boolean onSuite, Class<?> sample) {
+    MyObjectFactory.allParams.clear();
+
+    XmlSuite suite = createXmlSuite("Test IObjectFactory2", "TmpTest", sample);
+    TestNG tng = create(suite);
+
+    if (onSuite) {
+      suite.setObjectFactoryClass(MyObjectFactory.class);
+    } else {
+      tng.setObjectFactory(MyObjectFactory.class);
+    }
+
+    tng.run();
+  }
+
+  @DataProvider
+  public static Object[][] dp() {
+    return new Object[][] {new Object[] {true}, new Object[] {false}};
+  }
+}

testng-core/src/test/java/test/objectfactory/issue2676/LocalSuiteAlteringListener.java

@@ -0,0 +1,13 @@
+package test.objectfactory.issue2676;
+
+import java.util.List;
+import org.testng.IAlterSuiteListener;
+import org.testng.xml.XmlSuite;
+
+public class LocalSuiteAlteringListener implements IAlterSuiteListener {
+
+  @Override
+  public void alter(List<XmlSuite> suites) {
+    suites.forEach(each -> each.setObjectFactoryClass(LoggingObjectFactorySample.class));
+  }
+}

testng-core/src/test/java/test/objectfactory/issue2676/LoggingObjectFactorySample.java

@@ -0,0 +1,16 @@
+package test.objectfactory.issue2676;
+
+import java.lang.reflect.Constructor;
+import org.testng.ITestObjectFactory;
+import org.testng.internal.objects.InstanceCreator;
+
+public class LoggingObjectFactorySample implements ITestObjectFactory {
+
+  public static boolean wasInvoked = false;
+
+  @Override
+  public <T> T newInstance(Constructor<T> constructor, Object... parameters) {
+    wasInvoked = true;
+    return InstanceCreator.newInstance(constructor, parameters);
+  }
+}

testng-core/src/test/java/test/objectfactory/issue2676/TestClassSample.java

@@ -0,0 +1,9 @@
+package test.objectfactory.issue2676;
+
+import org.testng.annotations.Test;
+
+public class TestClassSample {
+
+  @Test
+  public void sampleTestMethod() {}
+}

testng-core/src/test/resources/testng.xml

@@ -207,7 +207,7 @@
       <class name="test.factory.issue1924.IssueTest"/>
       <class name="test.factory.github328.GitHub328Test" />
       <class name="test.factory.issue1745.Github1745Test"/>
-      <class name="test.objectfactory.github1827.GitHub1827Test"/>
+      <class name="test.objectfactory.ObjectFactoryTest"/>
       <class name="test.github1490.VerifyDataProviderListener"/>
       <class name="test.listeners.issue2456.IssueTest"/>
       <class name="test.methodselection.MethodSelectionTest"/>
@@ -432,7 +432,6 @@
       <class name="test.factory.FactoryIntegrationTest" />
       <class name="test.factory.EmptyFactoryDataProviderTest" />
 
-      <class name="test.factory.github1131.GitHub1131Test" />
       <class name="test.factory.nested.GitHub1307Test"/>
 
     </classes>