testng-team · juherr · Jul 30, 2026 · Jul 30, 2026 · Jul 30, 2026 · Jul 30, 2026
@@ -1,4 +1,11 @@
 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)
+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

@@ -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"));
   }

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

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

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

@@ -117,8 +117,15 @@ enum Location {
   private final String m_fileName;
   private final boolean m_loadClasses;
   private boolean m_validate = false;
+  private boolean m_doctypeDeclared = false;
   private boolean m_hasWarn = false;
 
+  /**
+   * Resolved once per parse rather than per violation, so a malformed suite cannot re-read the
+   * system property -- and re-log the "unknown value" warning -- for every error it produces.
+   */
+  private final XmlValidationMode m_validationMode = XmlValidationMode.current();
+
   public TestNGContentHandler(String fileName, boolean loadClasses) {
     m_fileName = fileName;
     m_loadClasses = loadClasses;
@@ -128,6 +135,12 @@ public TestNGContentHandler(String fileName, boolean loadClasses) {
   public InputSource resolveEntity(String publicId, String systemId)
       throws SAXException, IOException {
 
+    // The document declares a doctype, whoever ends up providing it. Tracked separately from
+    // m_validate, which means "TestNG substituted its own copy of the DTD": gating error reporting
+    // on m_validate silently discarded every violation for suites pointing at their own DTD copy
+    // or at a corporate mirror.
+    m_doctypeDeclared = true;
+
     if (skipConsideringSystemId(systemId)) {
       m_validate = true;
       InputStream is = loadDtdUsingClassLoader();
@@ -495,10 +508,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 +792,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_doctypeDeclared) {
+      // Without a doctype a validating parser only ever complains that no grammar was found, which
+      // would turn the existing "you should add a <!DOCTYPE>" hint into a hard failure.
+      return;
+    }
+    switch (m_validationMode) {
+      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;
     }
   }
 

@@ -7,38 +7,62 @@
 import javax.xml.parsers.SAXParser;
 import javax.xml.parsers.SAXParserFactory;
 import org.testng.TestNGException;
-import org.testng.internal.AutoCloseableLock;
 import org.testng.log4testng.Logger;
 import org.xml.sax.SAXException;
 import org.xml.sax.helpers.DefaultHandler;
 
 public abstract class XMLParser<T> implements IFileParser<T> {
 
-  private static final SAXParser m_saxParser;
+  /**
+   * Whether the next parse will validate against the TestNG DTD. Exposed so that tests can tell
+   * "validation is off in this JVM" apart from "this file is valid", instead of inferring it from a
+   * parse that does not fail -- an inference that cannot be made.
+   */
+  static boolean isValidating() {
+    return configureValidation(loadSAXParserFactory());
+  }
 
-  static {
+  /**
+   * Each parse gets its own parser.
+   *
+   * <p>A single shared {@link SAXParser} was kept behind a class wide lock, which made every parse
+   * in the JVM wait for every other one. That is not a matter of contention only: entity resolution
+   * happens inside {@code parse}, and {@link TestNGContentHandler} resolves an unknown system id
+   * over HTTP with no connect or read timeout, so one suite pointing at an unreachable DTD mirror
+   * would block suite parsing everywhere until the socket gave up.
+   *
+   * <p>The parser was shared because building one was assumed to be expensive. Measured, {@code
+   * SAXParserFactory.newInstance()} and {@code newSAXParser()} cost about 20 microseconds each --
+   * nothing against reading a suite file, let alone against fetching a DTD. Building per parse also
+   * removes the mutable static state that had to be invalidated whenever the validation mode
+   * changed, so the mode in effect is now simply read at each parse.
+   */
+  public void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException {
     SAXParserFactory spf = loadSAXParserFactory();
-
-    if (supportsValidation(spf)) {
-      spf.setNamespaceAware(true);
-      spf.setValidating(true);
-    }
-
-    SAXParser parser = null;
+    configureValidation(spf);
+    SAXParser parser;
     try {
       parser = spf.newSAXParser();
     } catch (ParserConfigurationException | SAXException e) {
       Logger.getLogger(XMLParser.class).error(e.getMessage(), e);
+      throw new TestNGException("No SAXParser could be configured to read suite files.", e);
     }
-    m_saxParser = parser;
+    parser.parse(is, dh);
   }
 
-  private static final AutoCloseableLock lock = new AutoCloseableLock();
-
-  public void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException {
-    try (AutoCloseableLock ignore = lock.lock()) {
-      m_saxParser.parse(is, dh);
-    }
+  /**
+   * Configures the factory for the validation mode currently in effect, and reports whether the
+   * parsers it builds will validate. Pinning the mode to whatever was set when this class was first
+   * loaded made {@code testng.xml.validation} silently ineffective for anything that sets it later
+   * -- the very failure mode this setting exists to fix.
+   */
+  private static boolean configureValidation(SAXParserFactory spf) {
+    // Namespace awareness is deliberately left off: DTD validation does not need it, suite files
+    // are not namespaced, and turning it on would make an unbound prefix fatal and an xmlns
+    // attribute a validity error -- neither of which has anything to do with validating a suite.
+    boolean validating = XmlValidationMode.current().isValidating() && supportsValidation(spf);
+    spf.setValidating(validating);
+    return validating;
   }
 
   /**
@@ -57,12 +81,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;
     }
   }

@@ -0,0 +1,69 @@
+package org.testng.xml;
+
+import java.util.Arrays;
+import java.util.Locale;
+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.
+ *
+ * <p>The property is read once per parse, never in the middle of one. {@code XMLParser} rebuilds
+ * its shared parser when the mode has changed since the last parse, which decides <em>whether</em>
+ * violations are raised at all, and {@code TestNGContentHandler} captures the mode when it is
+ * constructed, which decides <em>how</em> they are reported. Changing the property therefore takes
+ * effect from the next parse onwards, including when moving away from {@link #OFF}; changing it
+ * while a parse is in flight has no effect on that parse.
+ */
+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;
+    }
+    try {
+      return valueOf(requested.trim().toUpperCase(Locale.ROOT));
+    } catch (IllegalArgumentException e) {
+      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;
+    }
+  }
+}