From 96ea07bc6fb0e023feddd622fd41732ec4e0dc83 Mon Sep 17 00:00:00 2001 From: Krishnan Mahadevan Date: Tue, 1 Jan 2019 18:56:44 +0530 Subject: [PATCH] Streamline native injection w.r.t cloning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1994 Currently as part of supporting native injection TestNG resorts to invoking the “clone()” method on the parameter being natively injected. This becomes a problem because XmlTest is the only internal TestNG object that implements the Cloneable interface wherein it resorts to something like a deep copy and adds the cloned instance to the XmlSuite. This causes a lot of phony XmlTest objects to be added up to the suite and causes issues as detailed in the bug. Fixed this by introducing a marker interface to indicate to TestNG to skip cloning and use the object as is. Annotated “XmlTest” with this annotation so that we dont clone xmltest but instead use it as is. Yes this will expose the XmlTest object, but its already exposed to the end user via various different means. So I guess we are ok here. --- CHANGES.txt | 1 + .../org/testng/annotations/SkipCloning.java | 16 ++++++++++ .../java/org/testng/internal/TestResult.java | 14 +++++++- src/main/java/org/testng/xml/XmlTest.java | 30 +++++++++++++++++ .../java/test/parameters/ParameterTest.java | 10 +++++- .../parameters/issue1994/TestclassSample.java | 32 +++++++++++++++++++ 6 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 src/main/java/org/testng/annotations/SkipCloning.java create mode 100644 src/test/java/test/parameters/issue1994/TestclassSample.java diff --git a/CHANGES.txt b/CHANGES.txt index 2acbb35d73..a5ca2780df 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ Current +Fixed: GITHUB-1994: Prevent duplication of XmlTest objects when its used as a parameter for Native Injection (Krishnan Mahadevan) Fixed: GITHUB-165: @AfterGroups is not executed when group member fails or is skipped (Krishnan Mahadevan) Fixed: GITHUB-118: @BeforeGroups only called if group is specified explicitly (Krishnan Mahadevan) Fixed: GITHUB-182: Inherited test methods do not get expected group behavior (Krishnan Mahadevan) diff --git a/src/main/java/org/testng/annotations/SkipCloning.java b/src/main/java/org/testng/annotations/SkipCloning.java new file mode 100644 index 0000000000..b7edd5a5cf --- /dev/null +++ b/src/main/java/org/testng/annotations/SkipCloning.java @@ -0,0 +1,16 @@ +package org.testng.annotations; + + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +/** + * Marker interface which when used on a type will ensure that TestNG does not clone the object but + * instead uses it as is when TestNG resorts to dependency injection. + */ +@Retention(java.lang.annotation.RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE}) +public @interface SkipCloning { + +} diff --git a/src/main/java/org/testng/internal/TestResult.java b/src/main/java/org/testng/internal/TestResult.java index a78f9789e6..a71da35703 100644 --- a/src/main/java/org/testng/internal/TestResult.java +++ b/src/main/java/org/testng/internal/TestResult.java @@ -12,6 +12,7 @@ import org.testng.Reporter; import org.testng.TestNGException; import org.testng.TestRunner; +import org.testng.annotations.SkipCloning; import org.testng.collections.Lists; import org.testng.collections.Objects; @@ -276,7 +277,7 @@ public void setParameters(Object[] parameters) { m_parameters = new Object[parameters.length]; for (int i = 0; i < parameters.length; i++) { // Copy parameter if possible because user may change it later - if (parameters[i] instanceof Cloneable) { + if (canAttemptCloning(parameters[i]) ) { try { Method clone = parameters[i].getClass().getDeclaredMethod("clone"); m_parameters[i] = clone.invoke(parameters[i]); @@ -292,6 +293,17 @@ public void setParameters(Object[] parameters) { } } + private static boolean canAttemptCloning(Object parameter) { + if (parameter == null) { + return false; + } + SkipCloning skipCloning = parameter.getClass().getAnnotation(SkipCloning.class); + if (skipCloning != null) { + return false; + } + return parameter instanceof Cloneable; + } + @Override public Object getInstance() { return IParameterInfo.embeddedInstance(this.m_method.getInstance()); diff --git a/src/main/java/org/testng/xml/XmlTest.java b/src/main/java/org/testng/xml/XmlTest.java index e0416d363c..62ec7ed892 100644 --- a/src/main/java/org/testng/xml/XmlTest.java +++ b/src/main/java/org/testng/xml/XmlTest.java @@ -9,6 +9,7 @@ import org.testng.TestNG; import org.testng.TestNGException; +import org.testng.annotations.SkipCloning; import org.testng.collections.Lists; import org.testng.collections.Maps; import org.testng.xml.dom.ParentSetter; @@ -16,6 +17,7 @@ import static org.testng.xml.XmlSuite.ParallelMode.skipDeprecatedValues; /** This class describes the tag <test> in testng.xml. */ +@SkipCloning public class XmlTest implements Cloneable { public static final int DEFAULT_TIMEOUT_MS = Integer.MAX_VALUE; @@ -75,6 +77,31 @@ private void init(XmlSuite suite, int index) { // For YAML public XmlTest() {} + /** + * This constructor acts as a copy constructor. Please note that it does not automatically + * associate the copied {@link XmlTest} object with the current {@link XmlSuite} object and + * requires it to be done explicitly. + * + * @param xmlTest - The {@link XmlTest} object to copy from. + */ + public XmlTest(XmlTest xmlTest) { + XmlTest result = new XmlTest(); + result.setName(getName()); + result.setIncludedGroups(getIncludedGroups()); + result.setExcludedGroups(getExcludedGroups()); + result.setJUnit(isJUnit()); + result.setParallel(getParallel()); + result.setVerbose(getVerbose()); + result.setParameters(getLocalParameters()); + result.setXmlPackages(getXmlPackages()); + result.setTimeOut(getTimeOut()); + + Map> metagroups = getMetaGroups(); + for (Map.Entry> group : metagroups.entrySet()) { + result.addMetaGroup(group.getKey(), group.getValue()); + } + } + public void setXmlPackages(List packages) { m_xmlPackages = Lists.newArrayList(packages); } @@ -482,8 +509,11 @@ public String toXml(String indent) { *

The <classes> sub element is ignored for the moment. * * @return a clone of the current XmlTest + * @Deprecated - This method stands deprecated as of TestNG 7.0.0. Please make use of + * {@link XmlTest#XmlTest(XmlTest)} instead. */ @Override + @Deprecated public Object clone() { XmlTest result = new XmlTest(getSuite()); diff --git a/src/test/java/test/parameters/ParameterTest.java b/src/test/java/test/parameters/ParameterTest.java index 2abc64b96e..3ff237aeb1 100644 --- a/src/test/java/test/parameters/ParameterTest.java +++ b/src/test/java/test/parameters/ParameterTest.java @@ -1,6 +1,5 @@ package test.parameters; -import org.testng.ITestNGListener; import org.testng.ITestResult; import org.testng.TestListenerAdapter; import org.testng.TestNG; @@ -15,6 +14,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import test.parameters.issue1994.TestclassSample; import static org.assertj.core.api.Assertions.assertThat; @@ -130,4 +130,12 @@ public void testNativeInjectionAndParamsInjection() { testng.run(); assertThat(listener.getPassedTests().isEmpty()).isFalse(); } + + @Test(description = "GITHUB-1994") + public void testToEnsureNativeInjectionDoesnotResortToCloning() { + XmlSuite xmlsuite = createXmlSuite("suite", "test", TestclassSample.class); + TestNG testng = create(xmlsuite); + testng.run(); + assertThat(TestclassSample.count).isEqualTo(1); + } } diff --git a/src/test/java/test/parameters/issue1994/TestclassSample.java b/src/test/java/test/parameters/issue1994/TestclassSample.java new file mode 100644 index 0000000000..2abd6d26bd --- /dev/null +++ b/src/test/java/test/parameters/issue1994/TestclassSample.java @@ -0,0 +1,32 @@ +package test.parameters.issue1994; + +import org.testng.ISuite; +import org.testng.ISuiteListener; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Listeners; +import org.testng.annotations.Test; +import org.testng.xml.XmlTest; + +@Listeners(TestclassSample.class) +public class TestclassSample implements ISuiteListener { + + public static int count = 0; + + @BeforeClass + public void beforeClass(XmlTest xmlTest) { + } + + @Test + public void testMethod() { + } + + @Override + public void onFinish(ISuite suite) { + setCount(suite.getXmlSuite().getTests().size()); + } + + private static void setCount(int count) { + TestclassSample.count = count; + } + +}