fix(xml): re-enable DTD validation and stop losing data in toXml()

CHANGES.txt

@@ -1,4 +1,9 @@
 Current (7.13.0)
+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)
+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,12 @@
  */
 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 actually resolves from the classpath; the two disagreed until 7.12, so the
+   * emitted doctype advertised a schema that was never the one used to read the file back.
+   */
+  private static final String TESTNG_DTD = "testng-1.1.dtd";
 
   private static final String HTTPS_TESTNG_DTD_URL = "https://testng.org/" + TESTNG_DTD;
 

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);

testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java

@@ -495,10 +495,8 @@ public void xmlSelectorClass(boolean start, Attributes attributes) {
     if (start) {
       m_currentSelector.setName(attributes.getValue("name"));
       String priority = attributes.getValue("priority");
-      if (priority == null) {
-        priority = "0";
-      }
-      m_currentSelector.setPriority(Integer.parseInt(priority));
+      m_currentSelector.setPriority(
+          priority == null ? XmlMethodSelector.DEFAULT_PRIORITY : Integer.parseInt(priority));
     }
   }
 
@@ -781,8 +779,29 @@ public void endElement(String uri, String localName, String qName) {
 
   @Override
   public void error(SAXParseException e) throws SAXException {
-    if (m_validate) {
-      throw e;
+    if (!m_validate) {
+      // No DTD was resolved, so there is nothing to validate against. The missing <!DOCTYPE> is
+      // already reported by startElement().
+      return;
+    }
+    switch (XmlValidationMode.current()) {
+      case STRICT:
+        throw e;
+      case WARN:
+        Logger.getLogger(TestNGContentHandler.class)
+            .warn(
+                "The suite file ["
+                    + m_fileName
+                    + "] does not conform to "
+                    + Parser.TESTNG_DTD
+                    + ": "
+                    + e.getMessage()
+                    + ". Run with [-D"
+                    + RuntimeBehavior.XML_VALIDATION_MODE
+                    + "=strict] to turn this into a failure.");
+        break;
+      case OFF:
+        break;
     }
   }
 

testng-core/src/main/java/org/testng/xml/XMLParser.java

@@ -19,7 +19,7 @@ public abstract class XMLParser<T> implements IFileParser<T> {
   static {
     SAXParserFactory spf = loadSAXParserFactory();
 
-    if (supportsValidation(spf)) {
+    if (XmlValidationMode.current().isValidating() && supportsValidation(spf)) {
       spf.setNamespaceAware(true);
       spf.setValidating(true);
     }
@@ -57,12 +57,20 @@ private static SAXParserFactory loadSAXParserFactory() {
     }
   }
 
-  /** Tests if the current <code>SAXParserFactory</code> supports DTD validation. */
+  /**
+   * Tests if the current <code>SAXParserFactory</code> supports DTD validation.
+   *
+   * <p>The feature name is a plain identifier, not a URL to dereference, so it keeps its historical
+   * <code>http</code> scheme. Probing it under <code>https</code> makes every conforming parser
+   * raise <code>SAXNotRecognizedException</code>, which silently disabled validation altogether.
+   */
   private static boolean supportsValidation(SAXParserFactory spf) {
     try {
-      spf.getFeature("https://xml.org/sax/features/validation");
+      spf.getFeature("http://xml.org/sax/features/validation");
       return true;
     } catch (Exception ex) {
+      Logger.getLogger(XMLParser.class)
+          .warn("The XML parser in use does not support DTD validation: " + ex);
       return false;
     }
   }

testng-core/src/main/java/org/testng/xml/XmlValidationMode.java

@@ -0,0 +1,64 @@
+package org.testng.xml;
+
+import java.util.Arrays;
+import org.testng.internal.RuntimeBehavior;
+import org.testng.log4testng.Logger;
+
+/**
+ * How strictly a suite file is checked against the TestNG DTD, selected with the {@code
+ * testng.xml.validation} system property.
+ *
+ * <p>Validation used to be silently disabled: {@code XMLParser} probed the SAX validation feature
+ * under an {@code https} identifier, which no parser recognizes, so {@code setValidating(true)} was
+ * never reached and DTD violations went unreported. Turning it back on means suite files that have
+ * been accepted for years can suddenly be rejected -- the DTD constrains the order of the children
+ * of {@code <suite>}, for instance -- so {@link #WARN} is the default for now and reports
+ * violations without failing the run.
+ */
+public enum XmlValidationMode {
+
+  /** Do not validate at all. */
+  OFF,
+
+  /** Validate and report violations as warnings. The default. */
+  WARN,
+
+  /** Validate and fail on the first violation. */
+  STRICT;
+
+  private static final XmlValidationMode DEFAULT = WARN;
+
+  public boolean isValidating() {
+    return this != OFF;
+  }
+
+  /**
+   * The mode requested by the {@code testng.xml.validation} system property, falling back to {@link
+   * #WARN} when the property is absent or holds an unknown value.
+   */
+  public static XmlValidationMode current() {
+    String requested = RuntimeBehavior.getXmlValidationMode();
+    if (requested == null || requested.trim().isEmpty()) {
+      return DEFAULT;
+    }
+    String normalized = requested.trim().toUpperCase();
+    return Arrays.stream(values())
+        .filter(mode -> mode.name().equals(normalized))
+        .findFirst()
+        .orElseGet(
+            () -> {
+              Logger.getLogger(XmlValidationMode.class)
+                  .warn(
+                      "Unknown value ["
+                          + requested
+                          + "] for the system property ["
+                          + RuntimeBehavior.XML_VALIDATION_MODE
+                          + "]. Expected one of "
+                          + Arrays.toString(values())
+                          + ". Falling back to ["
+                          + DEFAULT
+                          + "].");
+              return DEFAULT;
+            });
+  }
+}

testng-core/src/test/java/org/testng/xml/SuiteDigest.java

@@ -0,0 +1,134 @@
+package org.testng.xml;
+
+import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
+
+/**
+ * A canonical, human-readable dump of everything a suite file can express, used by the round trip
+ * characterization tests.
+ *
+ * <p>Comparing digests rather than calling {@link XmlSuite#equals(Object)} is deliberate: {@code
+ * equals} ignores 11 of the 26 fields of {@link XmlSuite}, among them the parameters, the groups,
+ * the method selectors, {@code preserve-order}, {@code group-by-instances}, {@code guice-stage},
+ * {@code parent-module} and {@code allow-return-values}. A round trip that dropped any of those
+ * would still compare equal.
+ *
+ * <p>Any value lost or altered by a round trip shows up as a diff on a single line.
+ */
+public final class SuiteDigest {
+
+  private SuiteDigest() {}
+
+  public static String of(XmlSuite suite) {
+    StringBuilder sb = new StringBuilder();
+    append(sb, "suite.name", suite.getName());
+    append(sb, "suite.verbose", suite.getVerbose());
+    append(sb, "suite.parallel", suite.getParallel());
+    append(sb, "suite.threadCount", suite.getThreadCount());
+    append(sb, "suite.dataProviderThreadCount", suite.getDataProviderThreadCount());
+    append(sb, "suite.useGlobalThreadPool", suite.useGlobalThreadPool());
+    append(sb, "suite.shareThreadPoolForDataProviders", suite.isShareThreadPoolForDataProviders());
+    append(sb, "suite.timeOut", suite.getTimeOut());
+    append(sb, "suite.configFailurePolicy", suite.getConfigFailurePolicy());
+    append(sb, "suite.skipFailedInvocationCounts", suite.skipFailedInvocationCounts());
+    append(sb, "suite.preserveOrder", suite.getPreserveOrder());
+    append(sb, "suite.groupByInstances", suite.getGroupByInstances());
+    append(sb, "suite.allowReturnValues", suite.getAllowReturnValues());
+    append(sb, "suite.parentModule", suite.getParentModule());
+    append(sb, "suite.guiceStage", suite.getGuiceStage());
+    append(sb, "suite.objectFactory", suite.getObjectFactoryClass());
+    append(sb, "suite.listeners", suite.getListeners());
+    append(sb, "suite.suiteFiles", suite.getSuiteFiles());
+    append(sb, "suite.parameters", sorted(suite.getParameters()));
+    append(sb, "suite.includedGroups", suite.getIncludedGroups());
+    append(sb, "suite.excludedGroups", suite.getExcludedGroups());
+    appendPackages(sb, "suite", suite.getPackages());
+    appendMethodSelectors(sb, "suite", suite.getMethodSelectors());
+
+    List<XmlTest> tests = suite.getTests();
+    append(sb, "suite.tests.count", tests.size());
+    for (XmlTest test : tests) {
+      appendTest(sb, test);
+    }
+    return sb.toString();
+  }
+
+  private static void appendTest(StringBuilder sb, XmlTest test) {
+    String prefix = "test[" + test.getIndex() + ']';
+    append(sb, prefix + ".name", test.getName());
+    append(sb, prefix + ".verbose", test.getVerbose());
+    append(sb, prefix + ".parallel", test.getParallel());
+    append(sb, prefix + ".threadCount", test.getThreadCount());
+    append(sb, prefix + ".timeOut", test.getTimeOut());
+    append(sb, prefix + ".preserveOrder", test.getPreserveOrder());
+    append(sb, prefix + ".groupByInstances", test.getGroupByInstances());
+    append(sb, prefix + ".allowReturnValues", test.getAllowReturnValues());
+    append(sb, prefix + ".skipFailedInvocationCounts", test.skipFailedInvocationCounts());
+    append(sb, prefix + ".parameters", sorted(test.getLocalParameters()));
+    append(sb, prefix + ".includedGroups", test.getIncludedGroups());
+    append(sb, prefix + ".excludedGroups", test.getExcludedGroups());
+    append(sb, prefix + ".metaGroups", sorted(test.getMetaGroups()));
+    append(sb, prefix + ".dependencyGroups", sorted(test.getXmlDependencyGroups()));
+    appendScript(sb, prefix, test.getScript());
+    appendPackages(sb, prefix, test.getXmlPackages());
+    appendMethodSelectors(sb, prefix, test.getMethodSelectors());
+
+    for (XmlClass xmlClass : test.getXmlClasses()) {
+      String classPrefix = prefix + ".class[" + xmlClass.getIndex() + ']';
+      append(sb, classPrefix + ".name", xmlClass.getName());
+      append(sb, classPrefix + ".parameters", sorted(xmlClass.getLocalParameters()));
+      append(sb, classPrefix + ".excludedMethods", xmlClass.getExcludedMethods());
+      for (XmlInclude include : xmlClass.getIncludedMethods()) {
+        String includePrefix = classPrefix + ".include[" + include.getIndex() + ']';
+        append(sb, includePrefix + ".name", include.getName());
+        append(sb, includePrefix + ".description", include.getDescription());
+        append(sb, includePrefix + ".invocationNumbers", include.getInvocationNumbers());
+        append(sb, includePrefix + ".parameters", sorted(include.getLocalParameters()));
+      }
+    }
+  }
+
+  /**
+   * Packages are described by name and filters only. {@code XmlPackage.getXmlClasses()} is
+   * deliberately not called: it scans the classpath, which would make the digest depend on the
+   * runtime environment rather than on the suite file.
+   */
+  private static void appendPackages(StringBuilder sb, String prefix, List<XmlPackage> packages) {
+    append(sb, prefix + ".packages.count", packages.size());
+    for (int i = 0; i < packages.size(); i++) {
+      XmlPackage xmlPackage = packages.get(i);
+      append(sb, prefix + ".package[" + i + "].name", xmlPackage.getName());
+      append(sb, prefix + ".package[" + i + "].include", xmlPackage.getInclude());
+      append(sb, prefix + ".package[" + i + "].exclude", xmlPackage.getExclude());
+    }
+  }
+
+  private static void appendMethodSelectors(
+      StringBuilder sb, String prefix, List<XmlMethodSelector> selectors) {
+    append(sb, prefix + ".methodSelectors.count", selectors.size());
+    for (int i = 0; i < selectors.size(); i++) {
+      XmlMethodSelector selector = selectors.get(i);
+      String selectorPrefix = prefix + ".methodSelector[" + i + ']';
+      append(sb, selectorPrefix + ".className", selector.getClassName());
+      append(sb, selectorPrefix + ".priority", selector.getPriority());
+      appendScript(sb, selectorPrefix, selector.getScript());
+    }
+  }
+
+  private static void appendScript(StringBuilder sb, String prefix, XmlScript script) {
+    if (script == null) {
+      return;
+    }
+    append(sb, prefix + ".script.language", script.getLanguage());
+    append(sb, prefix + ".script.expression", script.getExpression());
+  }
+
+  private static <V> Map<String, V> sorted(Map<String, V> map) {
+    return new TreeMap<>(map);
+  }
+
+  private static void append(StringBuilder sb, String key, Object value) {
+    sb.append(key).append('=').append(value).append('\n');
+  }
+}