From 0af93d8f63d0b1ce57f95a16afb5af29371bf132 Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 14:16:15 +0200 Subject: [PATCH 1/7] fix(xml): re-enable DTD validation and stop losing data in toXml() DTD validation of suite files has been silently dead: XMLParser probed the SAX validation feature under "https://xml.org/sax/features/validation". The feature name is a plain identifier, not a URL to dereference, so every conforming parser raises SAXNotRecognizedException, supportsValidation() returned false, and neither setValidating(true) nor setNamespaceAware(true) was ever reached. TestNGContentHandler.error() could therefore never fire and only well-formedness errors surfaced. Turning validation back on can reject suite files that have been accepted for years -- the DTD constrains the order of the children of , for instance -- so introduce testng.xml.validation=off|warn|strict and default it to warn, which reports violations without failing the run. Add round trip characterization tests over every suite file of the corpus (112 files, two invariants each: the serialized form must be a fixed point, and the parsed model must survive unchanged). They compare a canonical digest rather than XmlSuite.equals(), which ignores 11 of its 26 fields, among them the parameters, the groups and the method selectors. Those tests immediately exposed two data losses in the writer: - was never written, so regenerating a suite (testng-failed.xml, for instance) dropped method descriptions. - omitted a priority of -1 while the parser reads a missing priority as 0. A negative method-selector priority is meaningful, since RunInfo#includeMethod short-circuits on it, so serializing a suite and reading it back changed its behaviour. Reader and writer now share XmlMethodSelector.DEFAULT_PRIORITY. Also align the emitted doctype with the DTD the parser actually resolves (1.1, not 1.0) and reorder testng-all.xml, the single real DTD violation in the corpus. --- CHANGES.txt | 5 + .../org/testng/internal/RuntimeBehavior.java | 9 ++ .../java/org/testng/xml/DefaultXmlWeaver.java | 8 +- .../main/java/org/testng/xml/XmlInclude.java | 3 + .../org/testng/xml/XmlMethodSelector.java | 11 +- .../org/testng/xml/TestNGContentHandler.java | 31 +++- .../main/java/org/testng/xml/XMLParser.java | 14 +- .../org/testng/xml/XmlValidationMode.java | 64 +++++++++ .../test/java/org/testng/xml/SuiteDigest.java | 134 ++++++++++++++++++ .../java/org/testng/xml/XmlRoundTripTest.java | 96 +++++++++++++ .../org/testng/xml/XmlValidationTest.java | 80 +++++++++++ testng-core/src/test/resources/testng-all.xml | 25 ++-- testng-core/src/test/resources/testng.xml | 2 + .../xml/validation/wrong-element-order.xml | 16 +++ 14 files changed, 473 insertions(+), 25 deletions(-) create mode 100644 testng-core/src/main/java/org/testng/xml/XmlValidationMode.java create mode 100644 testng-core/src/test/java/org/testng/xml/SuiteDigest.java create mode 100644 testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java create mode 100644 testng-core/src/test/java/org/testng/xml/XmlValidationTest.java create mode 100644 testng-core/src/test/resources/xml/validation/wrong-element-order.xml diff --git a/CHANGES.txt b/CHANGES.txt index f0895d9f3..7cfdddb95 100644 --- a/CHANGES.txt +++ b/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 , so regenerating a suite (testng-failed.xml, for instance) lost method descriptions (Julien Herr) +Fixed: XmlSuite.toXml() dropped a 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 diff --git a/testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java b/testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java index 33a26e5c2..f43c639ec 100644 --- a/testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java +++ b/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")); } diff --git a/testng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.java b/testng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.java index 3dd11cc04..a6a14af6f 100644 --- a/testng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.java +++ b/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; diff --git a/testng-core-api/src/main/java/org/testng/xml/XmlInclude.java b/testng-core-api/src/main/java/org/testng/xml/XmlInclude.java index e3a26b88b..b9a7c1189 100644 --- a/testng-core-api/src/main/java/org/testng/xml/XmlInclude.java +++ b/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 invocationNumbers = getInvocationNumbers(); if (invocationNumbers != null && !invocationNumbers.isEmpty()) { p.setProperty("invocation-numbers", XmlClass.listToString(invocationNumbers)); diff --git a/testng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.java b/testng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.java index e63228d61..b07d9b117 100644 --- a/testng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.java +++ b/testng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.java @@ -6,9 +6,13 @@ /** This class describes the tag <method-selector> 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); 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 93dacb28b..d5617aaeb 100644 --- a/testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java +++ b/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 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; } } diff --git a/testng-core/src/main/java/org/testng/xml/XMLParser.java b/testng-core/src/main/java/org/testng/xml/XMLParser.java index 2386d403c..5a96900d7 100644 --- a/testng-core/src/main/java/org/testng/xml/XMLParser.java +++ b/testng-core/src/main/java/org/testng/xml/XMLParser.java @@ -19,7 +19,7 @@ public abstract class XMLParser implements IFileParser { 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 SAXParserFactory supports DTD validation. */ + /** + * Tests if the current SAXParserFactory supports DTD validation. + * + *

The feature name is a plain identifier, not a URL to dereference, so it keeps its historical + * http scheme. Probing it under https makes every conforming parser + * raise SAXNotRecognizedException, 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; } } diff --git a/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java b/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java new file mode 100644 index 000000000..8f7b00412 --- /dev/null +++ b/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. + * + *

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 }, 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; + }); + } +} diff --git a/testng-core/src/test/java/org/testng/xml/SuiteDigest.java b/testng-core/src/test/java/org/testng/xml/SuiteDigest.java new file mode 100644 index 000000000..a908e4a65 --- /dev/null +++ b/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. + * + *

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. + * + *

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 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 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 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 Map sorted(Map map) { + return new TreeMap<>(map); + } + + private static void append(StringBuilder sb, String key, Object value) { + sb.append(key).append('=').append(value).append('\n'); + } +} diff --git a/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java b/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java new file mode 100644 index 000000000..e417fb284 --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java @@ -0,0 +1,96 @@ +package org.testng.xml; + +import static org.assertj.core.api.Assertions.assertThat; +import static test.SimpleBaseTest.getPathToResource; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.stream.Stream; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +/** + * Characterization tests over every suite file of the test corpus, pinning the behaviour of the XML + * reader ({@link SuiteXmlParser}) and of the XML writer ({@code toXml()}) as a pair. + * + *

These tests assert nothing about what the output should look like; they assert that + * it does not change. They exist so that moving the serialization code out of the domain model can + * be done safely, since the project has no binary-compatibility tooling in CI. + * + *

Two independent invariants are checked, because neither one alone is sufficient: the + * serialized form must be a fixed point, which pins attribute selection and layout, and the parsed + * model must survive unchanged, which pins the data (see {@link SuiteDigest}). + */ +public class XmlRoundTripTest { + + @Test(dataProvider = "suiteFiles") + public void serializedSuiteIsAFixedPoint(String suiteFile) throws IOException { + String firstPass = parseFile(suiteFile).toXml(); + String secondPass = parseString(suiteFile, firstPass).toXml(); + + assertThat(secondPass) + .as("re-serializing the suite parsed back from %s must be a fixed point", suiteFile) + .isEqualTo(firstPass); + } + + @Test(dataProvider = "suiteFiles") + public void suiteContentSurvivesTheRoundTrip(String suiteFile) throws IOException { + XmlSuite parsedFromFile = parseFile(suiteFile); + XmlSuite reparsed = parseString(suiteFile, parsedFromFile.toXml()); + + assertThat(SuiteDigest.of(reparsed)) + .as( + "the suite parsed back from the serialized form of %s must carry the same data", + suiteFile) + .isEqualTo(SuiteDigest.of(parsedFromFile)); + } + + /** + * Every {@code .xml} file of the test corpus whose content contains a {@code The filter is deliberately content based rather than name based, so that suite files added + * later are picked up without touching this class. It also excludes, without needing an explicit + * list, the fixtures whose root element is {@code } on purpose ({@code xml/badWith*.xml}). + */ + @DataProvider(name = "suiteFiles") + public static Object[][] suiteFiles() throws IOException { + Path root = Paths.get(getPathToResource("")); + try (Stream paths = Files.walk(root)) { + return paths + .filter(Files::isRegularFile) + .filter(path -> path.getFileName().toString().endsWith(".xml")) + .filter(XmlRoundTripTest::declaresASuite) + .sorted() + .map(path -> new Object[] {root.relativize(path).toString()}) + .toArray(Object[][]::new); + } + } + + private static boolean declaresASuite(Path path) { + try { + return new String(Files.readAllBytes(path), StandardCharsets.UTF_8).contains("Validation was silently dead: {@code XMLParser} probed the SAX validation feature under an + * {@code https} identifier, no parser recognized it, and {@code setValidating(true)} was never + * reached. A test asserting only that valid files parse would have passed throughout, so the check + * that matters is that an invalid file is rejected. + */ +public class XmlValidationTest { + + private static final String INVALID_SUITE = "xml/validation/wrong-element-order.xml"; + private static final String VALID_SUITE = "xml/goodWithDoctype.xml"; + + @AfterMethod(alwaysRun = true) + public void clearValidationMode() { + System.clearProperty(RuntimeBehavior.XML_VALIDATION_MODE); + } + + @Test + public void strictModeRejectsASuiteThatViolatesTheDtd() { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "strict"); + + assertThatThrownBy(() -> parse(INVALID_SUITE)) + .hasMessageContaining("must match") + .hasMessageContaining("suite"); + } + + @Test + public void warnModeAcceptsASuiteThatViolatesTheDtd() { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "warn"); + + assertThatCode(() -> assertThat(parse(INVALID_SUITE).getName()).isEqualTo("WrongElementOrder")) + .doesNotThrowAnyException(); + } + + @Test + public void strictModeAcceptsAValidSuite() { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "strict"); + + assertThatCode(() -> assertThat(parse(VALID_SUITE).getName()).isEqualTo("GitHub809")) + .doesNotThrowAnyException(); + } + + @Test + public void anUnknownModeFallsBackToWarn() { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "not-a-mode"); + + assertThat(XmlValidationMode.current()).isEqualTo(XmlValidationMode.WARN); + } + + @Test + public void theDefaultModeIsWarn() { + System.clearProperty(RuntimeBehavior.XML_VALIDATION_MODE); + + assertThat(XmlValidationMode.current()).isEqualTo(XmlValidationMode.WARN); + assertThat(XmlValidationMode.WARN.isValidating()).isTrue(); + assertThat(XmlValidationMode.OFF.isValidating()).isFalse(); + } + + private static XmlSuite parse(String suiteFile) throws IOException { + try (InputStream stream = Files.newInputStream(Paths.get(getPathToResource(suiteFile)))) { + return new SuiteXmlParser().parse(suiteFile, stream, false); + } + } +} diff --git a/testng-core/src/test/resources/testng-all.xml b/testng-core/src/test/resources/testng-all.xml index 9f14162f3..3aeb7a9df 100644 --- a/testng-core/src/test/resources/testng-all.xml +++ b/testng-core/src/test/resources/testng-all.xml @@ -3,18 +3,7 @@ - - - - - - - - - - - - + @@ -32,6 +21,18 @@ --> + + + + + + + + + + + + diff --git a/testng-core/src/test/resources/testng.xml b/testng-core/src/test/resources/testng.xml index 5ad70b2f7..06676d119 100644 --- a/testng-core/src/test/resources/testng.xml +++ b/testng-core/src/test/resources/testng.xml @@ -815,6 +815,8 @@ + + diff --git a/testng-core/src/test/resources/xml/validation/wrong-element-order.xml b/testng-core/src/test/resources/xml/validation/wrong-element-order.xml new file mode 100644 index 000000000..5a0ce88e4 --- /dev/null +++ b/testng-core/src/test/resources/xml/validation/wrong-element-order.xml @@ -0,0 +1,16 @@ + + + + + + + + + + + From e83e6a054eea2cc95f3aa844d917332183b662ae Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 15:31:16 +0200 Subject: [PATCH 2/7] fix(xml): make validation self-consistent and stop it depending on JVM state Follow-up to the previous commit, from reviewing it. Four CI jobs failed on a single test, and the axes ruled out any single culprit: fr_FR failed on one job and passed on another, and so did Windows and JDK 11. There were two causes. Turkish locale. XmlValidationMode parsed the property with toUpperCase() and no locale, so "strict" became "STRIC[I-with-dot]T", matched nothing, and silently degraded to warn. Only strict was affected, since "off" and "warn" have no 'i' -- which is why exactly one test failed. The matrix has a tr_TR axis, so this was live rather than theoretical. XmlSuite.getValidParallel already had the answer: Locale.ROOT. An inference that cannot be made. The test asked "does parsing an invalid file throw?" to decide whether validation was wired in. But the shared SAXParser is configured once per JVM, in a static initializer, so "this file is valid" and "validation is off in this JVM" are indistinguishable from the outside. XMLParser now records its decision and a test asserts it directly, with a message saying what a failure means; the reporting tests drive a parser built in the test, so they no longer depend on when the singleton was initialised. Three defects in the feature itself, found while reviewing: - Violations were discarded unless TestNG had substituted its own copy of the DTD. m_validate means "we provided the DTD", not "a doctype was declared", so a suite pointing at a local copy or a corporate mirror was never validated -- silently, even under strict. Tracked separately as m_doctypeDeclared. - toXml() emitted two sibling elements for any suite with suite-level groups, because the synthesized run block and XmlGroups both read the same XmlGroups. The DTD allows one, so TestNG's own output did not validate and strict would have rejected a regenerated testng-failed.xml. A new test now validates the serialized form of every suite in the corpus against the DTD. - setNamespaceAware(true) rode along with the validation switch. DTD validation does not need it, and it makes an unbound prefix fatal and an xmlns attribute a validity error. Removed. The mode is now resolved once per parse instead of once per violation, so a malformed suite no longer re-reads the property and re-logs for every error. Verified: ./gradlew build is green, and so is ./gradlew :testng-core:test -Dtestng.xml.validation=strict (14694 tests), which is the claim that matters -- strict has to accept everything TestNG itself produces. --- CHANGES.txt | 2 + .../java/org/testng/xml/DefaultXmlWeaver.java | 40 +++-- .../org/testng/xml/TestNGContentHandler.java | 21 ++- .../main/java/org/testng/xml/XMLParser.java | 21 ++- .../org/testng/xml/XmlValidationMode.java | 43 ++--- .../java/org/testng/xml/XmlRoundTripTest.java | 71 +++++++- .../org/testng/xml/XmlValidationTest.java | 155 ++++++++++++++++-- 7 files changed, 294 insertions(+), 59 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 7cfdddb95..e13faa865 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -3,6 +3,8 @@ Fixed: DTD validation of suite files was silently disabled: the SAX validation f Fixed: XmlSuite.toXml() dropped the "description" attribute of , so regenerating a suite (testng-failed.xml, for instance) lost method descriptions (Julien Herr) Fixed: XmlSuite.toXml() dropped a 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 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) diff --git a/testng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.java b/testng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.java index a6a14af6f..e0c0ce0ea 100644 --- a/testng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.java +++ b/testng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.java @@ -17,8 +17,9 @@ class DefaultXmlWeaver implements IWeaveXml { // TODO: move constants to XmlSuite? /** * 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. + * 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"; @@ -103,23 +104,28 @@ public String asXml(XmlSuite xmlSuite) { DEFAULT_ALLOW_RETURN_VALUES.toString()); xsb.push("suite", p); - List included = xmlSuite.getIncludedGroups(); - List 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 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 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 included = xmlSuite.getIncludedGroups(); + List 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()); 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 d5617aaeb..5564a2dd3 100644 --- a/testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java +++ b/testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java @@ -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(); @@ -779,12 +792,12 @@ public void endElement(String uri, String localName, String qName) { @Override public void error(SAXParseException e) throws SAXException { - if (!m_validate) { - // No DTD was resolved, so there is nothing to validate against. The missing is - // already reported by startElement(). + 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 " hint into a hard failure. return; } - switch (XmlValidationMode.current()) { + switch (m_validationMode) { case STRICT: throw e; case WARN: diff --git a/testng-core/src/main/java/org/testng/xml/XMLParser.java b/testng-core/src/main/java/org/testng/xml/XMLParser.java index 5a96900d7..f54545e6b 100644 --- a/testng-core/src/main/java/org/testng/xml/XMLParser.java +++ b/testng-core/src/main/java/org/testng/xml/XMLParser.java @@ -16,13 +16,21 @@ public abstract class XMLParser implements IFileParser { private static final SAXParser m_saxParser; + /** + * Whether the shared parser was built with DTD validation enabled. Decided once, because the + * parser itself is a singleton, and 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. + */ + private static final boolean validating; + static { SAXParserFactory spf = loadSAXParserFactory(); - if (XmlValidationMode.current().isValidating() && supportsValidation(spf)) { - spf.setNamespaceAware(true); - spf.setValidating(true); - } + // 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. + validating = XmlValidationMode.current().isValidating() && supportsValidation(spf); + spf.setValidating(validating); SAXParser parser = null; try { @@ -35,6 +43,11 @@ public abstract class XMLParser implements IFileParser { private static final AutoCloseableLock lock = new AutoCloseableLock(); + /** Whether the shared parser validates suite files against the TestNG DTD. */ + static boolean isValidating() { + return validating; + } + public void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException { try (AutoCloseableLock ignore = lock.lock()) { m_saxParser.parse(is, dh); diff --git a/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java b/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java index 8f7b00412..459dbc589 100644 --- a/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java +++ b/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java @@ -1,6 +1,7 @@ package org.testng.xml; import java.util.Arrays; +import java.util.Locale; import org.testng.internal.RuntimeBehavior; import org.testng.log4testng.Logger; @@ -14,6 +15,13 @@ * been accepted for years can suddenly be rejected -- the DTD constrains the order of the children * of {@code }, for instance -- so {@link #WARN} is the default for now and reports * violations without failing the run. + * + *

The property is read at two different moments, which constrains when it can be changed. {@code + * XMLParser} decides whether to validate once, when it builds its single static {@code + * SAXParser}; {@code TestNGContentHandler.error} decides how to report a violation on + * every occurrence. Moving between {@link #WARN} and {@link #STRICT} at run time therefore takes + * effect, but moving away from {@link #OFF} does not, because no violation is ever raised to + * report. Set the property on the command line to be safe. */ public enum XmlValidationMode { @@ -41,24 +49,21 @@ public static XmlValidationMode current() { 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; - }); + 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; + } } } diff --git a/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java b/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java index e417fb284..5b4bc2de6 100644 --- a/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java +++ b/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java @@ -4,6 +4,7 @@ import static test.SimpleBaseTest.getPathToResource; import java.io.ByteArrayInputStream; +import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.UncheckedIOException; @@ -11,9 +12,16 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.List; import java.util.stream.Stream; +import javax.xml.parsers.SAXParserFactory; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; +import org.testng.xml.internal.Parser; +import org.xml.sax.InputSource; +import org.xml.sax.SAXParseException; +import org.xml.sax.helpers.DefaultHandler; /** * Characterization tests over every suite file of the test corpus, pinning the behaviour of the XML @@ -51,6 +59,65 @@ public void suiteContentSurvivesTheRoundTrip(String suiteFile) throws IOExceptio .isEqualTo(SuiteDigest.of(parsedFromFile)); } + /** + * What we write must satisfy the DTD we advertise. Without this, {@code + * testng.xml.validation=strict} would reject TestNG's own output -- {@code testng-failed.xml} is + * produced by {@code toXml()} -- and the corpus round trip above would happily re-parse invalid + * XML because the default mode only warns. + */ + @Test(dataProvider = "suiteFiles") + public void serializedSuiteIsValidAgainstTheDtd(String suiteFile) throws Exception { + String xml = parseFile(suiteFile).toXml(); + + List violations = validateAgainstDtd(xml); + + assertThat(violations) + .as("the XML written for %s must satisfy %s:%n%s", suiteFile, Parser.TESTNG_DTD, xml) + .isEmpty(); + } + + /** + * Validates against the bundled DTD, resolving it locally so the test never touches the network. + */ + private static List validateAgainstDtd(String xml) throws Exception { + SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setValidating(true); + List violations = new ArrayList<>(); + factory + .newSAXParser() + .parse( + new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)), + new DefaultHandler() { + @Override + public InputSource resolveEntity(String publicId, String systemId) { + return new InputSource( + XmlRoundTripTest.class.getClassLoader().getResourceAsStream(Parser.TESTNG_DTD)); + } + + @Override + public void error(SAXParseException e) { + violations.add(e.getMessage()); + } + }); + return violations; + } + + /** + * The doctype we write must name the DTD the reader resolves. The writer lives in testng-core-api + * and the reader in testng-core, so they cannot share a constant and had silently drifted apart + * (1.0 written, 1.1 resolved). A comment would not have caught that; this does. + */ + @Test + public void theEmittedDoctypeNamesTheDtdTheParserResolves() { + assertThat(new XmlSuite().toXml()).contains(Parser.TESTNG_DTD); + } + + /** + * Fixtures that exist precisely because they are not valid, so they cannot be round tripped: with + * {@code testng.xml.validation=strict} the very first parse throws, which is what they are for. + */ + private static final String INVALID_ON_PURPOSE = "xml" + File.separator + "validation"; + /** * Every {@code .xml} file of the test corpus whose content contains a {@code path.getFileName().toString().endsWith(".xml")) .filter(XmlRoundTripTest::declaresASuite) .sorted() - .map(path -> new Object[] {root.relativize(path).toString()}) + .map(path -> root.relativize(path).toString()) + .filter(relativePath -> !relativePath.startsWith(INVALID_ON_PURPOSE)) + .map(relativePath -> new Object[] {relativePath}) .toArray(Object[][]::new); } } diff --git a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java index e4b6c918e..865a6a5c9 100644 --- a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java +++ b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java @@ -5,13 +5,21 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static test.SimpleBaseTest.getPathToResource; -import java.io.IOException; import java.io.InputStream; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Locale; +import java.util.Objects; +import javax.xml.parsers.SAXParserFactory; import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import org.testng.internal.RuntimeBehavior; +import org.testng.xml.internal.Parser; +import org.xml.sax.InputSource; +import org.xml.sax.SAXParseException; /** * Proves that DTD validation is actually wired into the parser. @@ -20,40 +28,112 @@ * {@code https} identifier, no parser recognized it, and {@code setValidating(true)} was never * reached. A test asserting only that valid files parse would have passed throughout, so the check * that matters is that an invalid file is rejected. + * + *

The two halves of the wiring are asserted separately on purpose. {@code XMLParser} decides + * whether to validate once, when it builds its singleton parser, so that decision is read + * back directly rather than inferred from a parse; inferring it made this test fail intermittently + * across the CI matrix, because "no error was reported" and "validation is not enabled" look + * identical from the outside. How a violation is reported is exercised through a parser + * built here, which no other test can have initialised first. */ public class XmlValidationTest { private static final String INVALID_SUITE = "xml/validation/wrong-element-order.xml"; private static final String VALID_SUITE = "xml/goodWithDoctype.xml"; + private String previousMode; + + @BeforeMethod + public void rememberValidationMode() { + previousMode = System.getProperty(RuntimeBehavior.XML_VALIDATION_MODE); + } + + /** + * Restores rather than clears: the property is global, so clearing it unconditionally would + * discard a value the JVM was started with -- the build forwards every {@code testng.*} property + * into the test JVM -- and leak into the rest of the suite. + */ @AfterMethod(alwaysRun = true) - public void clearValidationMode() { - System.clearProperty(RuntimeBehavior.XML_VALIDATION_MODE); + public void restoreValidationMode() { + if (previousMode == null) { + System.clearProperty(RuntimeBehavior.XML_VALIDATION_MODE); + } else { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, previousMode); + } + } + + /** + * The wiring that was broken. Asserted directly instead of through a parse, so that a JVM which + * cannot validate says so rather than looking like a suite file that happens to be valid. + */ + @Test + public void theSharedParserValidatesSuiteFilesByDefault() { + assertThat(XMLParser.isValidating()) + .as( + "the shared SAXParser must be built with DTD validation enabled; if this fails, either" + + " the JVM was started with -D%s=off or the JAXP implementation on the classpath" + + " does not support DTD validation", + RuntimeBehavior.XML_VALIDATION_MODE) + .isTrue(); } @Test public void strictModeRejectsASuiteThatViolatesTheDtd() { System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "strict"); - assertThatThrownBy(() -> parse(INVALID_SUITE)) - .hasMessageContaining("must match") - .hasMessageContaining("suite"); + assertThatThrownBy(() -> parseValidating(INVALID_SUITE)).isInstanceOf(SAXParseException.class); + } + + @Test + public void strictModeAcceptsAValidSuite() { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "strict"); + + assertThatCode(() -> assertThat(parseValidating(VALID_SUITE).getName()).isEqualTo("GitHub809")) + .doesNotThrowAnyException(); } @Test public void warnModeAcceptsASuiteThatViolatesTheDtd() { System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "warn"); - assertThatCode(() -> assertThat(parse(INVALID_SUITE).getName()).isEqualTo("WrongElementOrder")) + assertThatCode( + () -> + assertThat(parseValidating(INVALID_SUITE).getName()).isEqualTo("WrongElementOrder")) .doesNotThrowAnyException(); } + /** + * A violation must be reported even when the DTD is not the copy TestNG substitutes for its own + * doctype URLs -- an air-gapped or mirrored setup ships the DTD next to the suite. Reporting used + * to be gated on "TestNG provided the DTD", so those users got no validation at all, even under + * strict. + * + *

Written to a temporary directory rather than checked in, so that the DTD used here cannot + * drift from the one TestNG ships. + */ @Test - public void strictModeAcceptsAValidSuite() { + public void strictModeAlsoRejectsWhenTheSuitePointsAtItsOwnDtd() throws Exception { System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "strict"); - - assertThatCode(() -> assertThat(parse(VALID_SUITE).getName()).isEqualTo("GitHub809")) - .doesNotThrowAnyException(); + Path directory = Files.createTempDirectory("testng-local-dtd"); + Path dtd = directory.resolve(Parser.TESTNG_DTD); + try (InputStream shipped = getClass().getClassLoader().getResourceAsStream(Parser.TESTNG_DTD)) { + Files.copy(Objects.requireNonNull(shipped, "the DTD must be on the classpath"), dtd); + } + Path suite = directory.resolve("local-dtd-wrong-order.xml"); + Files.write( + suite, + ("\n" + + "\n" + // may only be the first child of . + + "\n" + + " \n" + + " \n" + + "\n") + .getBytes(StandardCharsets.UTF_8)); + + assertThatThrownBy(() -> parseValidating(suite)).isInstanceOf(SAXParseException.class); } @Test @@ -72,9 +152,56 @@ public void theDefaultModeIsWarn() { assertThat(XmlValidationMode.OFF.isValidating()).isFalse(); } - private static XmlSuite parse(String suiteFile) throws IOException { - try (InputStream stream = Files.newInputStream(Paths.get(getPathToResource(suiteFile)))) { - return new SuiteXmlParser().parse(suiteFile, stream, false); + /** + * {@code toUpperCase()} without a locale maps 'i' to 'İ' in Turkish, which turned {@code strict} + * into an unknown value and silently degraded it to the default. The CI matrix has a {@code + * tr_TR} axis, so this was a live failure rather than a theoretical one. + * + *

Mutating the default locale is process wide; it is restored in a {@code finally} and the + * enclosing {@code } of testng.xml runs single threaded. + */ + @Test + public void theModeIsParsedIndependentlyOfTheDefaultLocale() { + Locale previousLocale = Locale.getDefault(); + Locale.setDefault(new Locale("tr", "TR")); + try { + assertThat(modeOf("strict")).isEqualTo(XmlValidationMode.STRICT); + assertThat(modeOf("STRICT")).isEqualTo(XmlValidationMode.STRICT); + assertThat(modeOf(" Strict ")).isEqualTo(XmlValidationMode.STRICT); + assertThat(modeOf("off")).isEqualTo(XmlValidationMode.OFF); + assertThat(modeOf("warn")).isEqualTo(XmlValidationMode.WARN); + } finally { + Locale.setDefault(previousLocale); + } + } + + private static XmlValidationMode modeOf(String spelling) { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, spelling); + return XmlValidationMode.current(); + } + + /** + * Parses with a validating parser created here rather than with {@link SuiteXmlParser}, whose + * parser is a JVM-wide singleton configured once at class-initialisation time. Only the reporting + * path is under test; {@link #theSharedParserValidatesSuiteFilesByDefault()} covers the + * singleton. + */ + private static XmlSuite parseValidating(String suiteFile) throws Exception { + return parseValidating(Paths.get(getPathToResource(suiteFile))); + } + + private static XmlSuite parseValidating(Path path) throws Exception { + SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setValidating(true); + TestNGContentHandler handler = new TestNGContentHandler(path.toString(), false); + try (InputStream stream = Files.newInputStream(path)) { + InputSource source = new InputSource(stream); + // The system id matters: without it a relative doctype cannot be resolved, so TestNG falls + // back to substituting its own DTD and the "suite points at its own DTD" case would silently + // exercise the substituted path instead. + source.setSystemId(path.toUri().toString()); + factory.newSAXParser().parse(source, handler); } + return handler.getSuite(); } } From dd4dd49ab2706dd57452a378c98e4cc9c079acba Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 15:59:25 +0200 Subject: [PATCH 3/7] fix(xml): let the validation mode take effect after start-up Review follow-up. Three points were still valid against the current code; the fourth was already fixed and two more are noted below. XMLParser pinned the parser to whichever mode happened to be set when the class was first loaded. Anything setting testng.xml.validation later -- a second suite in a reused Surefire fork, an embedder, a test -- got no validation and no diagnostic, which is precisely the silent no-op this setting was introduced to remove. The parser is now rebuilt when the mode has changed since the last parse, under the lock it already holds, so moving away from OFF works. SuiteDigest compared only the included and excluded groups, which come from . A round trip dropping a suite-level or block was therefore invisible: verified by making the writer skip defines, which now fails two corpus files and previously failed none. The test asserting that the shared parser validates now skips under -Dtestng.xml.validation=off, a supported configuration it used to fail, and a new test pins that off actually reaches the parser rather than only the reporting. Not changed: the mode is already parsed with Locale.ROOT. Tracking the doctype via LexicalHandler.startDTD instead of resolveEntity would additionally cover a DOCTYPE with only an internal subset, but that means inlining the whole DTD in the suite file, which no TestNG suite does, and it would put a setProperty call on the parse path of every user for no practical gain. Verified: ./gradlew build, plus :testng-core:test under both -Dtestng.xml.validation=strict and =off, all green. --- .../main/java/org/testng/xml/XMLParser.java | 70 ++++++++++++------- .../org/testng/xml/XmlValidationMode.java | 12 ++-- .../test/java/org/testng/xml/SuiteDigest.java | 24 +++++++ .../org/testng/xml/XmlValidationTest.java | 33 ++++++--- 4 files changed, 99 insertions(+), 40 deletions(-) diff --git a/testng-core/src/main/java/org/testng/xml/XMLParser.java b/testng-core/src/main/java/org/testng/xml/XMLParser.java index f54545e6b..6b974a292 100644 --- a/testng-core/src/main/java/org/testng/xml/XMLParser.java +++ b/testng-core/src/main/java/org/testng/xml/XMLParser.java @@ -14,44 +14,66 @@ public abstract class XMLParser implements IFileParser { - private static final SAXParser m_saxParser; + private static final AutoCloseableLock lock = new AutoCloseableLock(); + + private static SAXParser m_saxParser; + + /** The mode {@link #m_saxParser} was configured for, so a change of mode can be noticed. */ + private static XmlValidationMode configuredFor; + + /** Whether {@link #m_saxParser} was built with DTD validation enabled. */ + private static boolean validating; /** - * Whether the shared parser was built with DTD validation enabled. Decided once, because the - * parser itself is a singleton, and 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. + * 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. */ - private static final boolean validating; + static boolean isValidating() { + try (AutoCloseableLock ignore = lock.lock()) { + parser(); + return validating; + } + } - static { + public void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException { + try (AutoCloseableLock ignore = lock.lock()) { + SAXParser parser = parser(); + if (parser == null) { + throw new TestNGException("No SAXParser could be configured to read suite files."); + } + parser.parse(is, dh); + } + } + + /** + * The shared parser, rebuilt when the validation mode has changed since it was created. The + * parser is a singleton because it is expensive, but pinning it to the mode that happened to be + * 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. + * + *

Must be called while holding {@link #lock}. + */ + private static SAXParser parser() { + XmlValidationMode mode = XmlValidationMode.current(); + if (m_saxParser != null && mode == configuredFor) { + return m_saxParser; + } SAXParserFactory spf = loadSAXParserFactory(); // 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. - validating = XmlValidationMode.current().isValidating() && supportsValidation(spf); + validating = mode.isValidating() && supportsValidation(spf); spf.setValidating(validating); - - SAXParser parser = null; try { - parser = spf.newSAXParser(); + m_saxParser = spf.newSAXParser(); } catch (ParserConfigurationException | SAXException e) { Logger.getLogger(XMLParser.class).error(e.getMessage(), e); + m_saxParser = null; } - m_saxParser = parser; - } - - private static final AutoCloseableLock lock = new AutoCloseableLock(); - - /** Whether the shared parser validates suite files against the TestNG DTD. */ - static boolean isValidating() { - return validating; - } - - public void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException { - try (AutoCloseableLock ignore = lock.lock()) { - m_saxParser.parse(is, dh); - } + configuredFor = mode; + return m_saxParser; } /** diff --git a/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java b/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java index 459dbc589..c8018bb31 100644 --- a/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java +++ b/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java @@ -16,12 +16,12 @@ * of {@code }, for instance -- so {@link #WARN} is the default for now and reports * violations without failing the run. * - *

The property is read at two different moments, which constrains when it can be changed. {@code - * XMLParser} decides whether to validate once, when it builds its single static {@code - * SAXParser}; {@code TestNGContentHandler.error} decides how to report a violation on - * every occurrence. Moving between {@link #WARN} and {@link #STRICT} at run time therefore takes - * effect, but moving away from {@link #OFF} does not, because no violation is ever raised to - * report. Set the property on the command line to be safe. + *

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 whether + * violations are raised at all, and {@code TestNGContentHandler} captures the mode when it is + * constructed, which decides how 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 { diff --git a/testng-core/src/test/java/org/testng/xml/SuiteDigest.java b/testng-core/src/test/java/org/testng/xml/SuiteDigest.java index a908e4a65..41d9013a4 100644 --- a/testng-core/src/test/java/org/testng/xml/SuiteDigest.java +++ b/testng-core/src/test/java/org/testng/xml/SuiteDigest.java @@ -43,6 +43,9 @@ public static String of(XmlSuite suite) { append(sb, "suite.parameters", sorted(suite.getParameters())); append(sb, "suite.includedGroups", suite.getIncludedGroups()); append(sb, "suite.excludedGroups", suite.getExcludedGroups()); + // Included/excluded groups only reflect . Without the defines and dependencies a round + // trip could drop a suite-level or block and still look identical. + appendGroups(sb, "suite", suite.getGroups()); appendPackages(sb, "suite", suite.getPackages()); appendMethodSelectors(sb, "suite", suite.getMethodSelectors()); @@ -89,6 +92,27 @@ private static void appendTest(StringBuilder sb, XmlTest test) { } } + private static void appendGroups(StringBuilder sb, String prefix, XmlGroups groups) { + if (groups == null) { + append(sb, prefix + ".groups", null); + return; + } + List defines = groups.getDefines(); + append(sb, prefix + ".groups.defines.count", defines.size()); + for (int i = 0; i < defines.size(); i++) { + append(sb, prefix + ".groups.define[" + i + "].name", defines.get(i).getName()); + append(sb, prefix + ".groups.define[" + i + "].includes", defines.get(i).getIncludes()); + } + List dependencies = groups.getDependencies(); + append(sb, prefix + ".groups.dependencies.count", dependencies.size()); + for (int i = 0; i < dependencies.size(); i++) { + append( + sb, + prefix + ".groups.dependencies[" + i + ']', + sorted(dependencies.get(i).getDependencies())); + } + } + /** * 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 diff --git a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java index 865a6a5c9..f550a746c 100644 --- a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java +++ b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java @@ -13,6 +13,7 @@ import java.util.Locale; import java.util.Objects; import javax.xml.parsers.SAXParserFactory; +import org.testng.SkipException; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -29,12 +30,11 @@ * reached. A test asserting only that valid files parse would have passed throughout, so the check * that matters is that an invalid file is rejected. * - *

The two halves of the wiring are asserted separately on purpose. {@code XMLParser} decides - * whether to validate once, when it builds its singleton parser, so that decision is read - * back directly rather than inferred from a parse; inferring it made this test fail intermittently - * across the CI matrix, because "no error was reported" and "validation is not enabled" look - * identical from the outside. How a violation is reported is exercised through a parser - * built here, which no other test can have initialised first. + *

The two halves of the wiring are asserted separately on purpose. Whether {@code XMLParser} + * validates is read back directly rather than inferred from a parse: inferring it made this test + * fail intermittently across the CI matrix, because "no error was reported" and "validation is not + * enabled" look identical from the outside. How a violation is reported is exercised + * through a parser built here, so it cannot depend on the state of the shared one. */ public class XmlValidationTest { @@ -68,15 +68,28 @@ public void restoreValidationMode() { */ @Test public void theSharedParserValidatesSuiteFilesByDefault() { + if (XmlValidationMode.current() == XmlValidationMode.OFF) { + throw new SkipException( + "the JVM is configured with -D" + + RuntimeBehavior.XML_VALIDATION_MODE + + "=off, which is a supported way to run the suite"); + } + assertThat(XMLParser.isValidating()) .as( - "the shared SAXParser must be built with DTD validation enabled; if this fails, either" - + " the JVM was started with -D%s=off or the JAXP implementation on the classpath" - + " does not support DTD validation", - RuntimeBehavior.XML_VALIDATION_MODE) + "the shared SAXParser must validate; if this fails, the JAXP implementation on the" + + " classpath does not support DTD validation") .isTrue(); } + /** Turning validation off must actually reach the parser, not only the reporting. */ + @Test + public void offModeStopsTheSharedParserFromValidating() { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "off"); + + assertThat(XMLParser.isValidating()).isFalse(); + } + @Test public void strictModeRejectsASuiteThatViolatesTheDtd() { System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "strict"); From 9b4e3420fe5691f9f8d8bfbe3b705a3858eb8887 Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 16:26:47 +0200 Subject: [PATCH 4/7] test(xml): clean up the temporary DTD fixture strictModeAlsoRejectsWhenTheSuitePointsAtItsOwnDtd created a temporary directory holding a copy of the DTD and never removed it. The suite runs in one fork per two cores, so every build left several behind: 73 had accumulated locally over this branch's test runs, each with an 8.5 KB copy of the DTD. Removed in a finally block so a failing assertion still reports rather than being masked by cleanup. Verified: ./gradlew build green, and the count of leftover directories under java.io.tmpdir is unchanged across a run instead of growing. --- .../org/testng/xml/XmlValidationTest.java | 43 +++++++++++-------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java index f550a746c..c9cc44b6d 100644 --- a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java +++ b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java @@ -129,24 +129,33 @@ public void strictModeAlsoRejectsWhenTheSuitePointsAtItsOwnDtd() throws Exceptio System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "strict"); Path directory = Files.createTempDirectory("testng-local-dtd"); Path dtd = directory.resolve(Parser.TESTNG_DTD); - try (InputStream shipped = getClass().getClassLoader().getResourceAsStream(Parser.TESTNG_DTD)) { - Files.copy(Objects.requireNonNull(shipped, "the DTD must be on the classpath"), dtd); - } Path suite = directory.resolve("local-dtd-wrong-order.xml"); - Files.write( - suite, - ("\n" - + "\n" - // may only be the first child of . - + "\n" - + " \n" - + " \n" - + "\n") - .getBytes(StandardCharsets.UTF_8)); - - assertThatThrownBy(() -> parseValidating(suite)).isInstanceOf(SAXParseException.class); + try { + try (InputStream shipped = + getClass().getClassLoader().getResourceAsStream(Parser.TESTNG_DTD)) { + Files.copy(Objects.requireNonNull(shipped, "the DTD must be on the classpath"), dtd); + } + Files.write( + suite, + ("\n" + + "\n" + // may only be the first child of . + + "\n" + + " \n" + + " \n" + + "\n") + .getBytes(StandardCharsets.UTF_8)); + + assertThatThrownBy(() -> parseValidating(suite)).isInstanceOf(SAXParseException.class); + } finally { + // The suite runs in one fork per two cores, so leaking a directory holding a copy of the + // DTD on every build adds up. + Files.deleteIfExists(suite); + Files.deleteIfExists(dtd); + Files.deleteIfExists(directory); + } } @Test From b8b3b7cf0ccf1388b7d02cad99c273eaa98ddf68 Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 16:45:53 +0200 Subject: [PATCH 5/7] test(xml): keep fixture cleanup from masking a test failure The finally block deleted the three paths in sequence, so a failure on the first left the other two behind, and any IOException it raised replaced the assertion failure that had actually happened. Deletion here is not hypothetically fallible: the entity resolver hands the DTD stream to InputSource without closing it, and on Windows a lingering handle blocks the delete. That would have turned a green test into an IOException with no trace of what was being asserted. Each path is now deleted independently and the failures are combined, and the cleanup runs as a try-with-resources resource so the JLS gives the wanted semantics for free: suppressed on the primary failure when there is one, propagated on its own when the test passed. Verified with a probe over the three cases -- a failing deletion still removes the remaining paths, a body failure stays primary with the cleanup attached as suppressed, and a cleanup failure propagates when the body passed -- plus ./gradlew build green with no leftover directories. --- .../org/testng/xml/XmlValidationTest.java | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java index c9cc44b6d..cc29a3e3f 100644 --- a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java +++ b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java @@ -5,6 +5,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static test.SimpleBaseTest.getPathToResource; +import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -130,7 +131,9 @@ public void strictModeAlsoRejectsWhenTheSuitePointsAtItsOwnDtd() throws Exceptio Path directory = Files.createTempDirectory("testng-local-dtd"); Path dtd = directory.resolve(Parser.TESTNG_DTD); Path suite = directory.resolve("local-dtd-wrong-order.xml"); - try { + // The cleanup is a resource so that a failure to delete is reported as suppressed on a test + // failure instead of replacing it, and only propagates on its own when the test passed. + try (AutoCloseable cleanup = () -> deleteAll(suite, dtd, directory)) { try (InputStream shipped = getClass().getClassLoader().getResourceAsStream(Parser.TESTNG_DTD)) { Files.copy(Objects.requireNonNull(shipped, "the DTD must be on the classpath"), dtd); @@ -149,12 +152,30 @@ public void strictModeAlsoRejectsWhenTheSuitePointsAtItsOwnDtd() throws Exceptio .getBytes(StandardCharsets.UTF_8)); assertThatThrownBy(() -> parseValidating(suite)).isInstanceOf(SAXParseException.class); - } finally { - // The suite runs in one fork per two cores, so leaking a directory holding a copy of the - // DTD on every build adds up. - Files.deleteIfExists(suite); - Files.deleteIfExists(dtd); - Files.deleteIfExists(directory); + } + } + + /** + * Deletes every path, so that one failure does not leave the rest behind. The suite runs in one + * fork per two cores, so leaking a directory holding a copy of the DTD on every build adds up. + * + *

Deletion can genuinely fail on Windows: the entity resolver hands the DTD stream to {@code + * InputSource} without closing it, and a lingering handle blocks the delete. + */ + private static void deleteAll(Path... paths) throws IOException { + IOException failures = null; + for (Path path : paths) { + try { + Files.deleteIfExists(path); + } catch (IOException e) { + if (failures == null) { + failures = new IOException("Failed to clean up the temporary DTD fixture"); + } + failures.addSuppressed(e); + } + } + if (failures != null) { + throw failures; } } From 93b504bf48b54e5eac66367cb667aa5300d710fd Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 18:36:14 +0200 Subject: [PATCH 6/7] refactor(test): extract the suite corpus helpers out of XmlRoundTripTest Three test classes now need to walk the same corpus, and the details are easy to get subtly wrong: opening a suite file without a system id is what lets xml/issue2501/2501.xml resolve its external entity, and resolving the doctype from the classpath is what keeps the tests off the network. The shared entity resolver also narrows the one XmlRoundTripTest had: it substituted the bundled DTD for every entity, which is harmless for serialized output but would feed the DTD to a ¶ms; reference. No behaviour change. --- .../test/java/org/testng/xml/SuiteCorpus.java | 112 ++++++++++++++++++ .../java/org/testng/xml/XmlRoundTripTest.java | 75 ++---------- 2 files changed, 121 insertions(+), 66 deletions(-) create mode 100644 testng-core/src/test/java/org/testng/xml/SuiteCorpus.java diff --git a/testng-core/src/test/java/org/testng/xml/SuiteCorpus.java b/testng-core/src/test/java/org/testng/xml/SuiteCorpus.java new file mode 100644 index 000000000..93ebc579f --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/SuiteCorpus.java @@ -0,0 +1,112 @@ +package org.testng.xml; + +import static test.SimpleBaseTest.getPathToResource; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Objects; +import java.util.stream.Stream; +import org.testng.annotations.DataProvider; +import org.testng.xml.internal.Parser; +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; + +/** + * The suite files of the test corpus, and the few things every test that walks them needs. + * + *

Shared rather than duplicated because the details are easy to get subtly wrong and each one + * was paid for: how a suite file is opened decides whether an external entity resolves, and how a + * doctype is resolved decides whether the test touches the network. + */ +final class SuiteCorpus { + + private SuiteCorpus() {} + + /** + * Fixtures that exist precisely because they are not valid, so they cannot be round tripped: with + * {@code testng.xml.validation=strict} the very first parse throws, which is what they are for. + */ + private static final String INVALID_ON_PURPOSE = "xml" + File.separator + "validation"; + + /** + * Every {@code .xml} file of the test corpus whose content contains a {@code The filter is deliberately content based rather than name based, so that suite files added + * later are picked up without touching this class. It also excludes, without needing an explicit + * list, the fixtures whose root element is {@code } on purpose ({@code xml/badWith*.xml}). + */ + @DataProvider(name = "suiteFiles") + public static Object[][] suiteFiles() throws IOException { + Path root = Paths.get(getPathToResource("")); + try (Stream paths = Files.walk(root)) { + return paths + .filter(Files::isRegularFile) + .filter(path -> path.getFileName().toString().endsWith(".xml")) + .filter(SuiteCorpus::declaresASuite) + .sorted() + .map(path -> root.relativize(path).toString()) + .filter(relativePath -> !relativePath.startsWith(INVALID_ON_PURPOSE)) + .map(relativePath -> new Object[] {relativePath}) + .toArray(Object[][]::new); + } + } + + private static boolean declaresASuite(Path path) { + try { + return new String(Files.readAllBytes(path), StandardCharsets.UTF_8).contains("That is what {@link org.testng.xml.internal.Parser} does, and {@code xml/issue2501/2501.xml} + * depends on it: its external entity is declared relative to the module directory, so it only + * resolves when the document has no base URI of its own. + */ + static InputStream open(String suiteFile) throws IOException { + return Files.newInputStream(pathOf(suiteFile)); + } + + static XmlSuite parseFile(String suiteFile) throws IOException { + try (InputStream stream = open(suiteFile)) { + return new SuiteXmlParser().parse(suiteFile, stream, false); + } + } + + static XmlSuite parseString(String suiteFile, String xml) { + byte[] bytes = xml.getBytes(StandardCharsets.UTF_8); + return new SuiteXmlParser().parse(suiteFile, new ByteArrayInputStream(bytes), false); + } + + /** + * Serves the bundled DTD for any doctype, so that no test ever reaches testng.org, and defers to + * the parser for everything else. + * + *

The {@code .dtd} test is what keeps {@code 2501.xml} working: substituting the DTD for + * every entity would feed it to its {@code ¶ms;} reference as well. + */ + static EntityResolver bundledDtdResolver() { + return (publicId, systemId) -> { + if (systemId == null || !systemId.endsWith(".dtd")) { + return null; + } + InputStream dtd = SuiteCorpus.class.getClassLoader().getResourceAsStream(Parser.TESTNG_DTD); + return new InputSource( + Objects.requireNonNull(dtd, Parser.TESTNG_DTD + " is not on the test classpath")); + }; + } +} diff --git a/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java b/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java index 5b4bc2de6..a1b367cbf 100644 --- a/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java +++ b/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java @@ -1,25 +1,19 @@ package org.testng.xml; import static org.assertj.core.api.Assertions.assertThat; -import static test.SimpleBaseTest.getPathToResource; +import static org.testng.xml.SuiteCorpus.parseFile; +import static org.testng.xml.SuiteCorpus.parseString; import java.io.ByteArrayInputStream; -import java.io.File; import java.io.IOException; -import java.io.InputStream; -import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; import java.util.ArrayList; import java.util.List; -import java.util.stream.Stream; import javax.xml.parsers.SAXParserFactory; -import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import org.testng.xml.internal.Parser; import org.xml.sax.InputSource; +import org.xml.sax.SAXException; import org.xml.sax.SAXParseException; import org.xml.sax.helpers.DefaultHandler; @@ -37,7 +31,7 @@ */ public class XmlRoundTripTest { - @Test(dataProvider = "suiteFiles") + @Test(dataProvider = "suiteFiles", dataProviderClass = SuiteCorpus.class) public void serializedSuiteIsAFixedPoint(String suiteFile) throws IOException { String firstPass = parseFile(suiteFile).toXml(); String secondPass = parseString(suiteFile, firstPass).toXml(); @@ -47,7 +41,7 @@ public void serializedSuiteIsAFixedPoint(String suiteFile) throws IOException { .isEqualTo(firstPass); } - @Test(dataProvider = "suiteFiles") + @Test(dataProvider = "suiteFiles", dataProviderClass = SuiteCorpus.class) public void suiteContentSurvivesTheRoundTrip(String suiteFile) throws IOException { XmlSuite parsedFromFile = parseFile(suiteFile); XmlSuite reparsed = parseString(suiteFile, parsedFromFile.toXml()); @@ -65,7 +59,7 @@ public void suiteContentSurvivesTheRoundTrip(String suiteFile) throws IOExceptio * produced by {@code toXml()} -- and the corpus round trip above would happily re-parse invalid * XML because the default mode only warns. */ - @Test(dataProvider = "suiteFiles") + @Test(dataProvider = "suiteFiles", dataProviderClass = SuiteCorpus.class) public void serializedSuiteIsValidAgainstTheDtd(String suiteFile) throws Exception { String xml = parseFile(suiteFile).toXml(); @@ -89,9 +83,9 @@ private static List validateAgainstDtd(String xml) throws Exception { new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)), new DefaultHandler() { @Override - public InputSource resolveEntity(String publicId, String systemId) { - return new InputSource( - XmlRoundTripTest.class.getClassLoader().getResourceAsStream(Parser.TESTNG_DTD)); + public InputSource resolveEntity(String publicId, String systemId) + throws IOException, SAXException { + return SuiteCorpus.bundledDtdResolver().resolveEntity(publicId, systemId); } @Override @@ -111,55 +105,4 @@ public void error(SAXParseException e) { public void theEmittedDoctypeNamesTheDtdTheParserResolves() { assertThat(new XmlSuite().toXml()).contains(Parser.TESTNG_DTD); } - - /** - * Fixtures that exist precisely because they are not valid, so they cannot be round tripped: with - * {@code testng.xml.validation=strict} the very first parse throws, which is what they are for. - */ - private static final String INVALID_ON_PURPOSE = "xml" + File.separator + "validation"; - - /** - * Every {@code .xml} file of the test corpus whose content contains a {@code The filter is deliberately content based rather than name based, so that suite files added - * later are picked up without touching this class. It also excludes, without needing an explicit - * list, the fixtures whose root element is {@code } on purpose ({@code xml/badWith*.xml}). - */ - @DataProvider(name = "suiteFiles") - public static Object[][] suiteFiles() throws IOException { - Path root = Paths.get(getPathToResource("")); - try (Stream paths = Files.walk(root)) { - return paths - .filter(Files::isRegularFile) - .filter(path -> path.getFileName().toString().endsWith(".xml")) - .filter(XmlRoundTripTest::declaresASuite) - .sorted() - .map(path -> root.relativize(path).toString()) - .filter(relativePath -> !relativePath.startsWith(INVALID_ON_PURPOSE)) - .map(relativePath -> new Object[] {relativePath}) - .toArray(Object[][]::new); - } - } - - private static boolean declaresASuite(Path path) { - try { - return new String(Files.readAllBytes(path), StandardCharsets.UTF_8).contains(" Date: Thu, 30 Jul 2026 18:36:25 +0200 Subject: [PATCH 7/7] feat(xml): ship testng-1.1.xsd alongside the DTD testng.xml has only ever had a DTD. #2594 shipped an XSD in 2021 but wired it to nothing, and DTD validation was itself silently dead until it was re-enabled, so the choice of schema language changed nothing for users. Now it does. The schema is salvaged from #2594 and corrected: no targetNamespace (suite files have never declared an xmlns, so one would break every existing testng.xml), no junit attribute, plus use-global-thread-pool and share-thread-pool-for-data-providers. It mirrors testng-1.1.dtd declaration for declaration, including where #2594 diverged: - and are a choice, not a sequence: the DTD writes (include?,exclude?)* and testng-all.xml does interleave them; - ANY stays ANY, because appears inside and inside ; - (true | false) maps to a named simpleType, not xsd:boolean, which would also accept 0 and 1 and make the schema looser than the DTD. The ordering constraint on the children of is kept rather than relaxed. XSD 1.0, the only level stock JAXP supports, cannot express "any order" and "at most one of each" at the same time, and relaxing the order would make the invalid-on-purpose fixture valid and lose the guard that caught the duplicate the writer used to emit. Two tests keep the two schemas from drifting, which they have done before: the whole corpus and every toXml() output must validate under both, and the declarations themselves -- elements, attributes, requiredness, defaults, enumerations -- are compared directly. --- CHANGES.txt | 1 + testng-core/src/main/resources/testng-1.1.xsd | 592 ++++++++++++++++++ .../org/testng/xml/SchemaConsistencyTest.java | 217 +++++++ .../org/testng/xml/XsdValidationTest.java | 147 +++++ testng-core/src/test/resources/testng.xml | 2 + 5 files changed, 959 insertions(+) create mode 100644 testng-core/src/main/resources/testng-1.1.xsd create mode 100644 testng-core/src/test/java/org/testng/xml/SchemaConsistencyTest.java create mode 100644 testng-core/src/test/java/org/testng/xml/XsdValidationTest.java diff --git a/CHANGES.txt b/CHANGES.txt index e13faa865..47077e1b0 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -5,6 +5,7 @@ Fixed: XmlSuite.toXml() dropped a priority of -1 while the pars 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 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: GITHUB-3319: testng.xml now has an XSD, testng-1.1.xsd, shipped next to testng-1.1.dtd and mirroring it declaration for declaration, for the tools that cannot consume a DTD. The DTD stays authoritative for files carrying a doctype; a test validates the whole suite corpus under both schemas and fails when the two stop agreeing (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) diff --git a/testng-core/src/main/resources/testng-1.1.xsd b/testng-core/src/main/resources/testng-1.1.xsd new file mode 100644 index 000000000..0edf441e6 --- /dev/null +++ b/testng-core/src/main/resources/testng-1.1.xsd @@ -0,0 +1,592 @@ + + + + + + + Here is a quick overview of the main parts of this schema. For more information, refer to + the main web site, https://testng.org. + + A suite is made of tests and parameters. + + A test is made of three parts: parameters, which override the suite parameters; groups, + made of two parts; and classes, defining which classes are going to be part of this test + run. + + In turn, groups are made of two parts: definitions, which allow you to group groups into + bigger groups, and runs, which define the groups that the methods must belong to in order + to be run during this test. + + Cedric Beust and Alexandru Popescu + + + + + + + A suite is the top-level element of a testng.xml file. + + + + + + + + + + + + + + + + + + The name of this suite (as it will appear in the reports). + + + + + + + How verbose the output on the console will be. This setting has no impact on the + HTML reports. + + + + + + + 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. + + + + + + + A module used to create the parent injector of all guice injectors used in tests of + the suite. + + + + + + + The stage with which the parent injector is created. + + + + + + + Whether to continue attempting Before/After Class/Methods after they have failed + once, or just skip the remaining ones. + + + + + + + An integer giving the size of the thread pool to use if you set parallel. + + + + + + + If "javadoc", TestNG will look for JavaDoc annotations in your sources, otherwise it + will use JDK5 annotations. + + + + + + + The time to wait in milliseconds before aborting the method (if parallel="methods") + or the test (if parallel="tests"). + + + + + + + Whether to skip failed invocations. + + + + + + + Whether TestNG should use a common thread pool for running both regular and data + driven tests in parallel. (Works only with TestNG versions 7.9.0 or higher.) + + + + + + + An integer giving the size of the thread pool to use for parallel data providers. + + + + + + + Whether TestNG should use a common thread pool for running parallel data providers. + (Works only with TestNG versions 7.9.0 or higher.) + + + + + + + A class that implements IObjectFactory that will be used to instantiate the test + objects. + + + + + + + + + If true, tests that return a value will be run as well. + + + + + + + + + + A list of XML files that contain more suite descriptions. + + + + + + + + + + + + + + + + + + + + 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 the suite. Parameters are used to link + Java method parameters to their actual value, defined here. + + + + + + + + + + + + + Method selectors define user classes used to select which methods to run. They need to + implement org.testng.IMethodSelector. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + A test contains parameters and classes. Additionally, you can define additional groups + ("groups of groups"). + + + + + + + + + + + + + + The name of this test (as it will appear in the reports). + + + + + + + How verbose the output on the console will be. This setting has no impact on the + HTML reports. Default value: suite level verbose. + + + + + + + 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. + + + + + + + An integer giving the size of the thread pool to be used if parallel mode is used. + Overrides the suite level value. + + + + + + + If "javadoc", TestNG will look for JavaDoc annotations in your sources, otherwise it + will use JDK5 annotations. + + + + + + + The time to wait in milliseconds before aborting the method (if parallel="methods") + or the test (if parallel="tests"). + + + + + + + Flag to enable/disable the current test. Default value: true. + + + + + + + Whether to skip failed invocations. + + + + + + + If true, the classes in this tag will be run in the same order as found in the XML + file. + + + + + + + + If true, tests that return a value will be run as well. + + + + + + + + + + Defines additional groups ("groups of groups") and also which groups to include in this + test run. + + + + + + + + + + + + + + + + + + + + + + + + Defines which groups to include in the current group of groups. + + + + + + + + + + + + + + Defines which groups to exclude from the current group of groups. + + + + + + + + + + + + The subtag of groups used to define which groups should be run. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + The list of classes to include in this test. + + + + + + + + + + + + + + + + + + + + + + + + The list of packages to include in this test. + + + + + + + + + + + + + The package description. If the package name ends with .* then subpackages are included + too. + + + + + + + + + + + + + + + The list of methods to include in or exclude from this test. + + + + + + + + + + + + + + + The list of listeners that will be passed to TestNG. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Deprecated. + + + + + Deprecated. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/testng-core/src/test/java/org/testng/xml/SchemaConsistencyTest.java b/testng-core/src/test/java/org/testng/xml/SchemaConsistencyTest.java new file mode 100644 index 000000000..f91b49154 --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/SchemaConsistencyTest.java @@ -0,0 +1,217 @@ +package org.testng.xml; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.testng.xml.XsdValidationTest.TESTNG_XSD; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.TreeMap; +import java.util.TreeSet; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import javax.xml.parsers.DocumentBuilderFactory; +import org.testng.annotations.Test; +import org.testng.xml.internal.Parser; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +/** + * Keeps {@code testng-1.1.dtd} and {@code testng-1.1.xsd} from drifting apart. + * + *

They already drifted once, in a way no test could see: the writer advertised {@code + * testng-1.0.dtd} while the reader always resolved {@code testng-1.1.dtd}. Two files describing the + * same language, edited independently, will do it again -- an attribute added to one and forgotten + * in the other is a single-line change nobody notices in review. + * + *

What is compared is the declarations: which elements exist, which attributes they + * carry, whether each is required, its default value and its enumerated values. Content models are + * deliberately left out -- they are expressed too differently in the two languages to compare + * mechanically without inventing a parser for both. They are covered from the other end instead: + * {@link XsdValidationTest} validates the whole corpus and every {@code toXml()} output under both + * schemas, and asserts that the DTD-invalid fixture is rejected by the XSD as well. + */ +public class SchemaConsistencyTest { + + @Test + public void bothSchemasDeclareTheSameElements() throws Exception { + assertThat(declarationsFromXsd().keySet()) + .as("the elements declared by %s and by %s", TESTNG_XSD, Parser.TESTNG_DTD) + .isEqualTo(declarationsFromDtd().keySet()); + } + + @Test + public void bothSchemasDeclareTheSameAttributes() throws Exception { + assertThat(declarationsFromXsd()) + .as( + "the attributes declared by %s and by %s, with their requiredness, default value and" + + " enumerated values", + TESTNG_XSD, Parser.TESTNG_DTD) + .isEqualTo(declarationsFromDtd()); + } + + /** + * An attribute rendered so that the two schema languages produce the same string, and so that a + * failure reads as a diff. For instance {@code optional default="skip" (continue|skip)}. + * + *

Enumerated values are sorted: which values exist is what has to match, the order they are + * written in is not something a reader can observe. + */ + private static String render(boolean required, String defaultValue, List enumeration) { + StringBuilder rendered = new StringBuilder(required ? "required" : "optional"); + if (defaultValue != null) { + rendered.append(" default=\"").append(defaultValue).append('"'); + } + rendered.append(' '); + if (enumeration.isEmpty()) { + rendered.append("CDATA"); + } else { + rendered.append('(').append(String.join("|", new TreeSet<>(enumeration))).append(')'); + } + return rendered.toString(); + } + + // ---------------------------------------------------------------- DTD + + /** {@code }; no content model in this DTD contains a {@code >}. */ + private static final Pattern ELEMENT = Pattern.compile("]*>"); + + private static final Pattern ATTLIST = Pattern.compile("]*)>"); + + /** One {@code name AttType DefaultDecl} triple of an {@code } body. */ + private static final Pattern ATTRIBUTE = + Pattern.compile( + "([\\w-]+)\\s+(CDATA|\\([^)]*\\))\\s+(#REQUIRED|#IMPLIED|\"[^\"]*\")", Pattern.DOTALL); + + private static final Pattern COMMENT = Pattern.compile("", Pattern.DOTALL); + + private static Map> declarationsFromDtd() throws IOException { + String dtd = COMMENT.matcher(read(Parser.TESTNG_DTD)).replaceAll(""); + + Map> declarations = new TreeMap<>(); + Matcher elements = ELEMENT.matcher(dtd); + while (elements.find()) { + declarations.put(elements.group(1), new TreeMap<>()); + } + + Matcher attributeLists = ATTLIST.matcher(dtd); + while (attributeLists.find()) { + Map attributes = + Objects.requireNonNull( + declarations.get(attributeLists.group(1)), + " has no matching "); + Matcher attribute = ATTRIBUTE.matcher(attributeLists.group(2)); + while (attribute.find()) { + String declaredDefault = attribute.group(3); + attributes.put( + attribute.group(1), + render( + "#REQUIRED".equals(declaredDefault), + declaredDefault.startsWith("\"") ? stripDelimiters(declaredDefault) : null, + enumerationOf(attribute.group(2)))); + } + } + return declarations; + } + + /** Drops the surrounding quotes of a default value, or the parentheses of an enumeration. */ + private static String stripDelimiters(String delimited) { + return delimited.substring(1, delimited.length() - 1); + } + + /** {@code CDATA} has no enumeration; {@code (a | b)} has one. */ + private static List enumerationOf(String attributeType) { + if (!attributeType.startsWith("(")) { + return new ArrayList<>(); + } + return Arrays.stream(stripDelimiters(attributeType).split("\\|")) + .map(String::trim) + .collect(Collectors.toList()); + } + + // ---------------------------------------------------------------- XSD + + private static Map> declarationsFromXsd() throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + Document schema; + try (InputStream stream = open(TESTNG_XSD)) { + schema = factory.newDocumentBuilder().parse(stream); + } + + Map> simpleTypes = new LinkedHashMap<>(); + for (Element simpleType : childrenNamed(schema.getDocumentElement(), "simpleType")) { + simpleTypes.put(simpleType.getAttribute("name"), enumerationValuesOf(simpleType)); + } + + Map> declarations = new TreeMap<>(); + for (Element element : childrenNamed(schema.getDocumentElement(), "element")) { + Map attributes = new TreeMap<>(); + for (Element complexType : childrenNamed(element, "complexType")) { + for (Element attribute : childrenNamed(complexType, "attribute")) { + attributes.put( + attribute.getAttribute("name"), + render( + "required".equals(attribute.getAttribute("use")), + attribute.hasAttribute("default") ? attribute.getAttribute("default") : null, + enumerationFor(attribute.getAttribute("type"), simpleTypes))); + } + } + declarations.put(element.getAttribute("name"), attributes); + } + return declarations; + } + + /** + * The schema has no imports, so a type name is either a built-in or one of its own named simple + * types; comparing the local part is enough to tell them apart. + */ + private static List enumerationFor(String type, Map> simpleTypes) { + String localName = type.substring(type.indexOf(':') + 1); + return simpleTypes.getOrDefault(localName, new ArrayList<>()); + } + + private static List enumerationValuesOf(Element simpleType) { + List values = new ArrayList<>(); + NodeList enumerations = simpleType.getElementsByTagNameNS("*", "enumeration"); + for (int i = 0; i < enumerations.getLength(); i++) { + values.add(((Element) enumerations.item(i)).getAttribute("value")); + } + return values; + } + + private static List childrenNamed(Element parent, String localName) { + List children = new ArrayList<>(); + NodeList nodes = parent.getChildNodes(); + for (int i = 0; i < nodes.getLength(); i++) { + Node node = nodes.item(i); + if (node.getNodeType() == Node.ELEMENT_NODE && localName.equals(node.getLocalName())) { + children.add((Element) node); + } + } + return children; + } + + // ---------------------------------------------------------------- resources + + private static String read(String resource) throws IOException { + try (InputStream stream = open(resource)) { + return new String(stream.readAllBytes(), StandardCharsets.UTF_8); + } + } + + private static InputStream open(String resource) { + return Objects.requireNonNull( + SchemaConsistencyTest.class.getClassLoader().getResourceAsStream(resource), + resource + " is not on the test classpath"); + } +} diff --git a/testng-core/src/test/java/org/testng/xml/XsdValidationTest.java b/testng-core/src/test/java/org/testng/xml/XsdValidationTest.java new file mode 100644 index 000000000..7e3b13ef6 --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/XsdValidationTest.java @@ -0,0 +1,147 @@ +package org.testng.xml; + +import static javax.xml.XMLConstants.W3C_XML_SCHEMA_NS_URI; +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.ByteArrayInputStream; +import java.io.InputStream; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.transform.stream.StreamSource; +import javax.xml.validation.Schema; +import javax.xml.validation.SchemaFactory; +import org.testng.annotations.Test; +import org.xml.sax.ErrorHandler; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; + +/** + * The XSD half of "the corpus validates under both schemas". + * + *

{@code testng-1.1.xsd} mirrors {@code testng-1.1.dtd} declaration for declaration, so anything + * the reader accepts must satisfy it, and so must anything the writer produces -- {@link + * XmlRoundTripTest#serializedSuiteIsValidAgainstTheDtd} asserts the same thing against the DTD. + * + *

{@link #theXsdRejectsASuiteThatViolatesTheDtd()} is the one that gives the other two their + * meaning: a schema that accepted everything would pass them both. DTD validation stayed silently + * dead for years precisely because the tests around it only ever asserted that valid files parse. + */ +public class XsdValidationTest { + + /** Shipped next to the DTD, in {@code testng-core/src/main/resources}. */ + static final String TESTNG_XSD = "testng-1.1.xsd"; + + private static final String INVALID_SUITE = "xml/validation/wrong-element-order.xml"; + + private static final Schema SCHEMA = loadSchema(); + + @Test(dataProvider = "suiteFiles", dataProviderClass = SuiteCorpus.class) + public void everySuiteFileOfTheCorpusValidatesAgainstTheXsd(String suiteFile) throws Exception { + List violations = validateFile(suiteFile); + + assertThat(violations).as("%s must satisfy %s", suiteFile, TESTNG_XSD).isEmpty(); + } + + @Test(dataProvider = "suiteFiles", dataProviderClass = SuiteCorpus.class) + public void serializedSuiteIsValidAgainstTheXsd(String suiteFile) throws Exception { + String xml = SuiteCorpus.parseFile(suiteFile).toXml(); + + List violations = validateXml(xml); + + assertThat(violations) + .as("the XML written for %s must satisfy %s:%n%s", suiteFile, TESTNG_XSD, xml) + .isEmpty(); + } + + /** + * The fixture is invalid because {@code } may only be the first child of {@code }. + * Asserting on the message as well, so that the test cannot start passing for some unrelated + * reason the day the fixture changes. + */ + @Test + public void theXsdRejectsASuiteThatViolatesTheDtd() throws Exception { + List violations = validateFile(INVALID_SUITE); + + assertThat(violations) + .as("%s violates the DTD, so it must violate %s too", INVALID_SUITE, TESTNG_XSD) + .isNotEmpty(); + assertThat(violations.toString()).contains("groups"); + } + + private static List validateFile(String suiteFile) throws Exception { + try (InputStream stream = SuiteCorpus.open(suiteFile)) { + // No system id, for the same reason SuiteCorpus.open() has none: xml/issue2501/2501.xml + // declares an external entity relative to the module directory. + return validate(new InputSource(stream)); + } + } + + private static List validateXml(String xml) throws Exception { + byte[] bytes = xml.getBytes(StandardCharsets.UTF_8); + return validate(new InputSource(new ByteArrayInputStream(bytes))); + } + + /** + * Validates while parsing, with the schema attached to the parser, as {@code JUnitReportsTest} + * does for the JUnit report schema. + * + *

Not through {@code Validator.validate(SAXSource)}: that implementation overwrites the entity + * resolver of the reader it is handed with one of its own, so the doctype would be fetched from + * testng.org instead of the classpath. Every suite file of the corpus declares one, and the + * published {@code testng-1.0.dtd} still has the {@code junit} attribute that 1.1 dropped, so the + * corpus failed to validate against a schema that was in fact correct. + * + *

The doctype must still be processed rather than ignored: it expands the external entity of + * {@code xml/issue2501/2501.xml} and supplies the defaulted attributes. + */ + private static List validate(InputSource source) throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + // Schema validation is defined in terms of namespaces, even for a schema without one. + factory.setNamespaceAware(true); + factory.setSchema(SCHEMA); + + List violations = new ArrayList<>(); + DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setEntityResolver(SuiteCorpus.bundledDtdResolver()); + builder.setErrorHandler(collectInto(violations)); + builder.parse(source); + return violations; + } + + private static ErrorHandler collectInto(List violations) { + return new ErrorHandler() { + @Override + public void warning(SAXParseException e) { + violations.add(e.getMessage()); + } + + @Override + public void error(SAXParseException e) { + violations.add(e.getMessage()); + } + + @Override + public void fatalError(SAXParseException e) throws SAXException { + throw e; + } + }; + } + + /** Fails loudly when the schema is missing, rather than turning every test below into a no-op. */ + private static Schema loadSchema() { + URL url = XsdValidationTest.class.getClassLoader().getResource(TESTNG_XSD); + Objects.requireNonNull(url, TESTNG_XSD + " is not on the test classpath"); + try { + return SchemaFactory.newInstance(W3C_XML_SCHEMA_NS_URI) + .newSchema(new StreamSource(url.toExternalForm())); + } catch (SAXException e) { + throw new IllegalStateException(TESTNG_XSD + " does not compile", e); + } + } +} diff --git a/testng-core/src/test/resources/testng.xml b/testng-core/src/test/resources/testng.xml index 06676d119..7d4975dc8 100644 --- a/testng-core/src/test/resources/testng.xml +++ b/testng-core/src/test/resources/testng.xml @@ -817,6 +817,8 @@ + +