fix(yaml): rewrite the YAML writer so its output can be read back

CHANGES.txt

@@ -1,4 +1,13 @@
 Current (7.13.0)
+Fixed: GITHUB-3318: Yaml.toYaml() produced YAML that could not be read back -- a duplicated "packages" key, sequence items written without "- ", <test> keys indented at the column of the item they belong to, package filters written without a colon and under the plural keys "includes"/"excludes" the reader does not bind, and "suite-files" written under an unknown key and only for a suite that has child suites. The writer now builds a document and lets snakeyaml emit it, so quoting, escaping and indentation are correct by construction: a parameter valued "a,b" no longer reads back as two entries, and one valued "44.0" no longer reads back as a Double (Julien Herr)
+New: GITHUB-3318: The YAML writer now also emits the suite-level groups, preserve-order, parent-module, guice-stage, allow-return-values, share-thread-pool-for-data-providers, the method selectors at both levels, class parameters and include descriptions, all of which were silently dropped (Julien Herr)
+Fixed: DTD validation of suite files was silently disabled: the SAX validation feature was probed under an "https" identifier that no parser recognizes, so setValidating(true) was never reached and violations went unreported. Validation is enabled again, with a new testng.xml.validation=off|warn|strict system property; the default "warn" reports violations without failing the run (Julien Herr)
+Fixed: XmlSuite.toXml() dropped the "description" attribute of <include>, so regenerating a suite (testng-failed.xml, for instance) lost method descriptions (Julien Herr)
+Fixed: XmlSuite.toXml() dropped a <selector-class> priority of -1 while the parser reads a missing priority as 0. Since a negative method-selector priority changes selector evaluation, serializing a suite and reading it back altered its behaviour (Julien Herr)
+Fixed: The doctype written by XmlSuite.toXml() advertised testng-1.0.dtd although the parser always resolves testng-1.1.dtd (Julien Herr)
+Fixed: XmlSuite.toXml() emitted two sibling <groups> elements for a suite that has suite-level groups, which the DTD allows only once, so TestNG's own output did not validate (Julien Herr)
+Fixed: DTD violations were discarded for suite files pointing at their own copy or a mirror of the DTD rather than at testng.org, so those suites were never validated (Julien Herr)
+New: Added round trip characterization tests covering every suite file of the test corpus, so that XML serialization can be refactored safely (Julien Herr)
 New: Added OpenRewrite to the build with a hand-picked recipe list (see rewrite.yml), and applied it to the main sources (Julien Herr)
 Fixed: Remove leftover dead JUnit code: the deprecated unused ConversionUtils and orphaned JUnit test samples, following the removal of JUnit execution support in 7.10.0 (Julien Herr)
 Update: Dependency refresh: Guice 6.0.0, JCommander 2.0, snakeyaml 2.6, slf4j-api 2.0.18. Guice 7 and JCommander 3 were skipped: they require jakarta.inject and Java 17 respectively

testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java

@@ -13,6 +13,7 @@ public final class RuntimeBehavior {
   private static final String TEST_CLASSPATH = "testng.test.classpath";
   private static final String SKIP_CALLER_CLS_LOADER = "skip.caller.clsLoader";
   public static final String TESTNG_USE_UNSECURED_URL = "testng.dtd.http";
+  public static final String XML_VALIDATION_MODE = "testng.xml.validation";
   public static final String SHOW_TESTNG_STACK_FRAMES = "testng.show.stack.frames";
   private static final String MEMORY_FRIENDLY_MODE = "testng.memory.friendly";
   public static final String STRICTLY_HONOUR_PARALLEL_MODE = "testng.strict.parallel";
@@ -52,6 +53,14 @@ public static boolean useSecuredUrlForDtd() {
     return !Boolean.getBoolean(TESTNG_USE_UNSECURED_URL);
   }
 
+  /**
+   * @return the raw value of {@value #XML_VALIDATION_MODE}, or {@code null} when unset. Interpreted
+   *     by {@code org.testng.xml.XmlValidationMode}.
+   */
+  public static String getXmlValidationMode() {
+    return System.getProperty(XML_VALIDATION_MODE);
+  }
+
   public static boolean isMemoryFriendlyMode() {
     return Boolean.parseBoolean(System.getProperty(MEMORY_FRIENDLY_MODE, "false"));
   }

testng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.java

@@ -15,8 +15,13 @@
  */
 class DefaultXmlWeaver implements IWeaveXml {
   // TODO: move constants to XmlSuite?
-  /** The name of the TestNG DTD. */
-  private static final String TESTNG_DTD = "testng-1.0.dtd";
+  /**
+   * The name of the TestNG DTD. Must stay in sync with {@code Parser.TESTNG_DTD}, which is the
+   * version the reader resolves from the classpath. The two had drifted apart, so the emitted
+   * doctype advertised a schema that was never the one used to read the file back. They cannot
+   * share a constant: {@code Parser} lives in testng-core, which depends on this module.
+   */
+  private static final String TESTNG_DTD = "testng-1.1.dtd";
 
   private static final String HTTPS_TESTNG_DTD_URL = "https://testng.org/" + TESTNG_DTD;
 
@@ -99,23 +104,28 @@ public String asXml(XmlSuite xmlSuite) {
         DEFAULT_ALLOW_RETURN_VALUES.toString());
     xsb.push("suite", p);
 
-    List<String> included = xmlSuite.getIncludedGroups();
-    List<String> excluded = xmlSuite.getExcludedGroups();
-    if (hasElements(included) || hasElements(excluded)) {
-      xsb.push("groups");
-      xsb.push("run");
-      for (String g : included) {
-        xsb.addEmptyElement("include", "name", g);
-      }
-      for (String g : excluded) {
-        xsb.addEmptyElement("exclude", "name", g);
-      }
-      xsb.pop("run");
-      xsb.pop("groups");
-    }
-
     if (xmlSuite.getGroups() != null) {
       xsb.getStringBuffer().append(xmlSuite.getGroups().toXml("  "));
+    } else {
+      // Only synthesize a <groups> block when the suite has no XmlGroups of its own to write.
+      // getIncludedGroups()/getExcludedGroups() read through to that same XmlGroups, so emitting
+      // both produced two sibling <groups> elements -- which the DTD allows only once, making
+      // TestNG's own output invalid. When the groups come from a parent suite there is nothing
+      // else to write, and flattening them here is what keeps a generated suite self-contained.
+      List<String> included = xmlSuite.getIncludedGroups();
+      List<String> excluded = xmlSuite.getExcludedGroups();
+      if (hasElements(included) || hasElements(excluded)) {
+        xsb.push("groups");
+        xsb.push("run");
+        for (String g : included) {
+          xsb.addEmptyElement("include", "name", g);
+        }
+        for (String g : excluded) {
+          xsb.addEmptyElement("exclude", "name", g);
+        }
+        xsb.pop("run");
+        xsb.pop("groups");
+      }
     }
 
     XmlUtils.dumpParameters(xsb, xmlSuite.getParameters());

testng-core-api/src/main/java/org/testng/xml/XmlInclude.java

@@ -75,6 +75,9 @@ public String toXml(String indent) {
     XMLStringBuffer xsb = new XMLStringBuffer(indent);
     Properties p = new Properties();
     p.setProperty("name", getName());
+    if (m_description != null) {
+      p.setProperty("description", m_description);
+    }
     List<Integer> invocationNumbers = getInvocationNumbers();
     if (invocationNumbers != null && !invocationNumbers.isEmpty()) {
       p.setProperty("invocation-numbers", XmlClass.listToString(invocationNumbers));

testng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.java

@@ -6,9 +6,13 @@
 
 /** This class describes the tag <code>&lt;method-selector&gt;</code> in testng.xml. */
 public class XmlMethodSelector {
+
+  /** The priority assumed when the {@code priority} attribute is absent from the suite file. */
+  public static final int DEFAULT_PRIORITY = 0;
+
   // Either this:
   private String m_className;
-  private int m_priority;
+  private int m_priority = DEFAULT_PRIORITY;
 
   // Or that:
   private XmlScript m_script;
@@ -56,7 +60,10 @@ public String toXml(String indent) {
     if (null != m_className) {
       Properties clsProp = new Properties();
       clsProp.setProperty("name", getClassName());
-      if (getPriority() != -1) {
+      // Omit the value the parser falls back to when the attribute is absent, so that a
+      // round trip is lossless. A negative priority is meaningful (see RunInfo#includeMethod)
+      // and must therefore be written out.
+      if (getPriority() != DEFAULT_PRIORITY) {
         clsProp.setProperty("priority", String.valueOf(getPriority()));
       }
       xsb.addEmptyElement("selector-class", clsProp);