From 1288058fac43aa200ee2a89f3a779e5604ca7011 Mon Sep 17 00:00:00 2001
From: Krishnan Mahadevan <krishnan.mahadevan1978@gmail.com>
Date: Wed, 7 Jun 2023 15:57:33 +0530
Subject: [PATCH] Dont honour params specified in suite-file tag

Closes #581

TestNG does not have any logic that would
honour parameter tags specified within suite-files
tag. But due to a parsing bug, we end up reading
parameters that were specified within
`<suite-file>` tag.
This tag by definition is NOT meant to have any
child tags inside of it.

Fixed this discrepancy by adding a warning when
this anomaly is detected and skipping of reading
the `<parameter>` tag inside it as if it were
specified within `<suite>` tag.
---
 CHANGES.txt                                   |   1 +
 .../org/testng/xml/TestNGContentHandler.java  |  10 +
 .../java/org/testng/xml/internal/Parser.java  |   2 +-
 testng-core/src/main/resources/testng-1.1.dtd | 213 ++++++++++++++++++
 .../java/test/parameters/ParameterTest.java   |  20 ++
 .../parameters/issue581/TestClassSample.java  |  12 +
 .../parametertest/issue_581/parent_suite.xml  |  11 +
 .../parametertest/issue_581/suite_one.xml     |   8 +
 .../parametertest/issue_581/suite_two.xml     |   8 +
 9 files changed, 284 insertions(+), 1 deletion(-)
 create mode 100755 testng-core/src/main/resources/testng-1.1.dtd
 create mode 100644 testng-core/src/test/java/test/parameters/issue581/TestClassSample.java
 create mode 100644 testng-core/src/test/resources/parametertest/issue_581/parent_suite.xml
 create mode 100644 testng-core/src/test/resources/parametertest/issue_581/suite_one.xml
 create mode 100644 testng-core/src/test/resources/parametertest/issue_581/suite_two.xml

diff --git a/CHANGES.txt b/CHANGES.txt
index 45b2265678..e12be724ec 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,5 +1,6 @@
 Current
 
+Fixed: GITHUB-581: Parameters of nested test suites are overridden(Krishnan Mahadevan)
 Fixed: GITHUB-727 : Fixing data races (Krishnan Mahadevan)
 Fixed: GITHUB-2913: Maps containing nulls can be incorrectly considered equal (Alex Heneveld)
 
diff --git a/testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java b/testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
index 89a9308e69..249fbf9e09 100644
--- a/testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
+++ b/testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
@@ -101,6 +101,7 @@ enum Location {
   }
 
   private final Stack<Location> m_locations = new Stack<>();
+  private boolean isSuiteFileTag = false;
 
   private XmlClass m_currentClass = null;
   private ArrayList<XmlInclude> m_currentIncludedMethods = null;
@@ -196,9 +197,11 @@ private void xmlSuiteFile(boolean start, Attributes attributes) {
       String path = attributes.getValue("path");
       pushLocation(Location.SUITE);
       m_suiteFiles.add(path);
+      isSuiteFileTag = true;
     } else {
       m_currentSuite.setSuiteFiles(m_suiteFiles);
       popLocation();
+      isSuiteFileTag = false;
     }
   }
 
@@ -611,6 +614,13 @@ public void startElement(String uri, String localName, String qName, Attributes
     } else if ("exclude".equals(qName)) {
       xmlExclude(true, attributes);
     } else if ("parameter".equals(qName)) {
+      if (isSuiteFileTag) {
+        // do-not process a <parameter> tag when it is specified inside <suite-files>
+        Logger.getLogger(getClass())
+            .warn(
+                "Ignoring the <parameter> tag because it is specified inside a <suite-file> tag.");
+        return;
+      }
       String value = expandValue(attributes.getValue("value"));
       Location location = m_locations.peek();
       switch (location) {
diff --git a/testng-core/src/main/java/org/testng/xml/internal/Parser.java b/testng-core/src/main/java/org/testng/xml/internal/Parser.java
index 7c42cfb8d3..5713b66eff 100644
--- a/testng-core/src/main/java/org/testng/xml/internal/Parser.java
+++ b/testng-core/src/main/java/org/testng/xml/internal/Parser.java
@@ -24,7 +24,7 @@
 public class Parser {
 
   /** The name of the TestNG DTD. */
-  public static final String TESTNG_DTD = "testng-1.0.dtd";
+  public static final String TESTNG_DTD = "testng-1.1.dtd";
 
   /** The URL to the deprecated TestNG DTD. */
   // It has to be public because its being used by TestNG eclipse plugin
diff --git a/testng-core/src/main/resources/testng-1.1.dtd b/testng-core/src/main/resources/testng-1.1.dtd
new file mode 100755
index 0000000000..62c6fd0cb5
--- /dev/null
+++ b/testng-core/src/main/resources/testng-1.1.dtd
@@ -0,0 +1,213 @@
+<!--
+
+Here is a quick overview of the main parts of this DTD.  For more information,
+refer to the <a href="https://testng.org">main web site</a>.
+                                                      
+A <b>suite</b> is made of <b>tests</b> and <b>parameters</b>.
+                                                      
+A <b>test</b> is made of three parts:                        
+
+<ul>
+<li> <b>parameters</b>, which override the suite parameters     
+<li> <b>groups</b>, made of two parts                           
+<li> <b>classes</b>, defining which classes are going to be part
+  of this test run                                    
+</ul>
+                                                      
+In turn, <b>groups</b> are made of two parts:                
+<ul>
+<li> Definitions, which allow you to group groups into   
+  bigger groups                                       
+<li> Runs, which defines the groups that the methods     
+  must belong to in order to be run during this test  
+</ul>
+                                                      
+Cedric Beust & Alexandru Popescu                      
+@title DTD for TestNG                                    
+@root suite
+
+-->
+
+
+<!-- A suite is the top-level element of a testng.xml file                  -->
+<!ELEMENT suite (groups?,(listeners|packages|test|parameter|method-selectors|suite-files)*) >
+
+<!-- Attributes: -->
+<!--
+@attr  name        The name of this suite (as it will appear in the reports)
+@attr  junit       Whether to run in JUnit mode.
+@attr  verbose     How verbose the output on the console will be.  
+                This setting has no impact on the HTML reports.
+@attr  parallel   Whether TestNG should use different threads
+                to run your tests (might speed up the process)
+                Do not use "true" and "false" values, they are now deprecated.
+@attr  parent-module A module used to create the parent injector of all guice injectors used
+       in tests of the suite
+@attr  guice-stage The stage with which the parent injector is created
+@attr  configfailurepolicy  Whether to continue attempting Before/After
+                Class/Methods after they've failed once or just skip remaining.
+@attr  thread-count An integer giving the size of the thread pool to use
+                if you set parallel.
+@attr  annotations  If "javadoc", TestNG will look for
+                JavaDoc annotations in your sources, otherwise it will
+                use JDK5 annotations.
+@attr  time-out     The time to wait in milliseconds before aborting the
+                method (if parallel="methods") or the test (parallel="tests")
+@attr  skipfailedinvocationcounts Whether to skip failed invocations.
+@attr  data-provider-thread-count An integer giving the size of the thread pool to use
+       for parallel data providers.
+@attr  object-factory A class that implements IObjectFactory that will be used to
+       instantiate the test objects.
+@attr allow-return-values If true, tests that return a value will be run as well
+-->
+<!ATTLIST suite 
+    name CDATA #REQUIRED
+    junit (true | false) "false"
+    verbose CDATA #IMPLIED
+    parallel (false | true | none | methods | tests | classes | instances) "none"
+    parent-module CDATA #IMPLIED
+    guice-stage (DEVELOPMENT | PRODUCTION | TOOL) "DEVELOPMENT"
+    configfailurepolicy (skip | continue) "skip"
+    thread-count CDATA "5"
+    annotations CDATA #IMPLIED
+    time-out CDATA #IMPLIED
+    skipfailedinvocationcounts (true | false) "false"
+    data-provider-thread-count CDATA "10"
+    object-factory CDATA #IMPLIED
+    group-by-instances (true | false) "false"
+    preserve-order (true | false) "true"
+    allow-return-values (true | false) "false"
+>
+
+<!-- A list of XML files that contain more suite descriptions -->
+<!ELEMENT suite-files (suite-file)* >
+
+<!ELEMENT suite-file EMPTY >
+<!ATTLIST suite-file
+    path CDATA #REQUIRED
+>
+
+<!--
+Parameters can be defined at the <suite> or at the <test> level.
+Parameters defined at the <test> level override parameters of the same name in <suite>
+Parameters are used to link Java method parameters to their actual value, defined here.
+-->
+<!ELEMENT parameter EMPTY>
+<!ATTLIST parameter
+    name CDATA #REQUIRED
+    value CDATA #REQUIRED >
+
+<!--
+Method selectors define user classes used to select which methods to run.
+They need to implement <tt>org.testng.IMethodSelector</tt> 
+-->
+<!ELEMENT method-selectors (method-selector*) >
+<!ELEMENT method-selector ((selector-class)*|script) >
+<!ELEMENT selector-class EMPTY>
+<!ATTLIST selector-class
+    name CDATA #REQUIRED
+  priority CDATA #IMPLIED
+>
+<!ELEMENT script EMPTY>
+<!ATTLIST script
+    language CDATA #REQUIRED
+>
+
+<!--
+A test contains parameters and classes.  Additionally, you can define additional groups ("groups of groups")
+-->
+
+<!ELEMENT test (method-selectors?,parameter*,groups?,packages?,classes?) >
+
+<!--
+@attr  name         The name of this test (as it will appear in the reports)
+@attr  junit        Whether to run in JUnit mode.
+@attr  verbose      How verbose the output on the console will be.
+                This setting has no impact on the HTML reports.
+                Default value: suite level verbose.
+@attr  parallel     Whether TestNG should use different threads
+                to run your tests (might speed up the process)
+                Do not use "true" and "false" values, they are now deprecated.
+@attr  thread-count An integer giving the size of the thread pool to be used if
+                parallel mode is used. Overrides the suite level value.
+@attr  annotations  If "javadoc", TestNG will look for
+                JavaDoc annotations in your sources, otherwise it will
+                use JDK5 annotations.
+@attr  time-out     the time to wait in milliseconds before aborting
+                the method (if parallel="methods") or the test (if parallel="tests")
+@attr  enabled      flag to enable/disable current test. Default value: true 
+@attr  skipfailedinvocationcounts Whether to skip failed invocations.
+@attr preserve-order If true, the classes in this tag will be run in the same order as
+found in the XML file.
+@attr allow-return-values If true, tests that return a value will be run as well
+-->
+<!ATTLIST test
+    name CDATA #REQUIRED 
+    junit (true | false) "false"
+    verbose  CDATA #IMPLIED
+    parallel  (false | true | none | methods | tests | classes | instances) #IMPLIED
+    thread-count CDATA #IMPLIED
+    annotations  CDATA #IMPLIED
+    time-out CDATA #IMPLIED
+    enabled (true | false) #IMPLIED
+    skipfailedinvocationcounts (true | false) "false"
+    preserve-order (true | false) "true"
+    group-by-instances (true | false) "false"
+    allow-return-values (true | false) "false"
+>
+
+<!--
+Defines additional groups ("groups of groups") and also which groups to include in this test run
+-->
+<!ELEMENT groups (define*,run?,dependencies?) >
+
+<!ELEMENT define (include*)>
+<!ATTLIST define
+    name CDATA #REQUIRED>
+
+<!-- Defines which groups to include in the current group of groups         -->
+<!ELEMENT include EMPTY>
+<!ATTLIST include
+    name CDATA #REQUIRED
+    description CDATA #IMPLIED
+    invocation-numbers CDATA #IMPLIED>
+
+<!-- Defines which groups to exclude from the current group of groups       -->
+<!ELEMENT exclude EMPTY>
+<!ATTLIST exclude
+    name CDATA #REQUIRED>
+
+<!-- The subtag of groups used to define which groups should be run         -->
+<!ELEMENT run (include?,exclude?)* >
+
+<!ELEMENT dependencies (group*)>
+
+<!ELEMENT group EMPTY>
+<!ATTLIST group
+    name CDATA #REQUIRED
+    depends-on CDATA #REQUIRED>
+
+<!-- The list of classes to include in this test                            -->
+<!ELEMENT classes (class*,parameter*) >
+<!ELEMENT class (methods|parameter)* >
+<!ATTLIST class
+    name CDATA #REQUIRED >
+
+<!-- The list of packages to include in this test                           -->
+<!ELEMENT packages (package*) >
+<!-- The package description. 
+     If the package name ends with .* then subpackages are included too.
+-->
+<!ELEMENT package (include?,exclude?)*>
+<!ATTLIST package
+    name CDATA #REQUIRED >
+
+<!-- The list of methods to include/exclude from this test                 -->
+<!ELEMENT methods (include?,exclude?,parameter?)* >
+
+<!-- The list of listeners that will be passed to TestNG -->
+<!ELEMENT listeners (listener*) >
+
+<!ELEMENT listener EMPTY>
+<!ATTLIST listener
+    class-name CDATA #REQUIRED >
diff --git a/testng-core/src/test/java/test/parameters/ParameterTest.java b/testng-core/src/test/java/test/parameters/ParameterTest.java
index 44c2ec93a8..8f43d40036 100644
--- a/testng-core/src/test/java/test/parameters/ParameterTest.java
+++ b/testng-core/src/test/java/test/parameters/ParameterTest.java
@@ -2,9 +2,12 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import org.testng.ISuite;
+import org.testng.ISuiteListener;
 import org.testng.ITestResult;
 import org.testng.TestListenerAdapter;
 import org.testng.TestNG;
@@ -131,4 +134,21 @@ public void testNativeInjectionAndParamsInjection() {
     testng.run();
     assertThat(listener.getPassedTests().isEmpty()).isFalse();
   }
+
+  @Test(description = "GITHUB-581")
+  public void ensureParametersSpecifiedInsideSuiteFilesTagAreIgnored() {
+    TestNG testng = create();
+    testng.setTestSuites(
+        Collections.singletonList("src/test/resources/parametertest/issue_581/parent_suite.xml"));
+    Map<String, Map<String, String>> actualParameters = new HashMap<>();
+    testng.addListener(
+        new ISuiteListener() {
+          @Override
+          public void onStart(ISuite suite) {
+            actualParameters.put(suite.getName(), suite.getXmlSuite().getAllParameters());
+          }
+        });
+    testng.run();
+    actualParameters.values().forEach(each -> assertThat(each).isEmpty());
+  }
 }
diff --git a/testng-core/src/test/java/test/parameters/issue581/TestClassSample.java b/testng-core/src/test/java/test/parameters/issue581/TestClassSample.java
new file mode 100644
index 0000000000..fb21ec245f
--- /dev/null
+++ b/testng-core/src/test/java/test/parameters/issue581/TestClassSample.java
@@ -0,0 +1,12 @@
+package test.parameters.issue581;
+
+import org.testng.annotations.Optional;
+import org.testng.annotations.Parameters;
+import org.testng.annotations.Test;
+
+public class TestClassSample {
+
+  @Parameters(value = "url")
+  @Test
+  public void test(@Optional String ignored) {}
+}
diff --git a/testng-core/src/test/resources/parametertest/issue_581/parent_suite.xml b/testng-core/src/test/resources/parametertest/issue_581/parent_suite.xml
new file mode 100644
index 0000000000..268285ba28
--- /dev/null
+++ b/testng-core/src/test/resources/parametertest/issue_581/parent_suite.xml
@@ -0,0 +1,11 @@
+<!DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd" >
+<suite name="parent_suite">
+  <suite-files>
+    <suite-file path="suite_one.xml">
+      <parameter name="url" value="http://www.google.com"/>
+    </suite-file>
+    <suite-file path="suite_two.xml">
+      <parameter name="url" value="http://www.bing.com"/>
+    </suite-file>
+  </suite-files>
+</suite>
diff --git a/testng-core/src/test/resources/parametertest/issue_581/suite_one.xml b/testng-core/src/test/resources/parametertest/issue_581/suite_one.xml
new file mode 100644
index 0000000000..e8a1c79820
--- /dev/null
+++ b/testng-core/src/test/resources/parametertest/issue_581/suite_one.xml
@@ -0,0 +1,8 @@
+<!DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd" >
+<suite name="suite_one">
+  <test name="test_one">
+    <classes>
+      <class name="test.parameters.issue581.TestClassSample"/>
+    </classes>
+  </test>
+</suite>
diff --git a/testng-core/src/test/resources/parametertest/issue_581/suite_two.xml b/testng-core/src/test/resources/parametertest/issue_581/suite_two.xml
new file mode 100644
index 0000000000..d584d6cbdb
--- /dev/null
+++ b/testng-core/src/test/resources/parametertest/issue_581/suite_two.xml
@@ -0,0 +1,8 @@
+<!DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd" >
+<suite name="suite_two">
+  <test name="test_two">
+    <classes>
+      <class name="test.parameters.issue581.TestClassSample"/>
+    </classes>
+  </test>
+</suite>