diff --git a/CHANGES.txt b/CHANGES.txt index f0895d9f3..47077e1b0 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,12 @@ 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) +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) 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..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 @@ -15,8 +15,13 @@ */ class DefaultXmlWeaver implements IWeaveXml { // TODO: move constants to XmlSuite? - /** The name of the TestNG DTD. */ - private static final String TESTNG_DTD = "testng-1.0.dtd"; + /** + * The name of the TestNG DTD. Must stay in sync with {@code Parser.TESTNG_DTD}, which is the + * version the reader resolves from the classpath. The two had drifted apart, so the emitted + * doctype advertised a schema that was never the one used to read the file back. They cannot + * share a constant: {@code Parser} lives in testng-core, which depends on this module. + */ + private static final String TESTNG_DTD = "testng-1.1.dtd"; private static final String HTTPS_TESTNG_DTD_URL = "https://testng.org/" + TESTNG_DTD; @@ -99,23 +104,28 @@ public String asXml(XmlSuite xmlSuite) { DEFAULT_ALLOW_RETURN_VALUES.toString()); xsb.push("suite", p); - List 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-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..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(); @@ -495,10 +508,8 @@ public void xmlSelectorClass(boolean start, Attributes attributes) { if (start) { m_currentSelector.setName(attributes.getValue("name")); String priority = attributes.getValue("priority"); - if (priority == null) { - priority = "0"; - } - m_currentSelector.setPriority(Integer.parseInt(priority)); + m_currentSelector.setPriority( + priority == null ? XmlMethodSelector.DEFAULT_PRIORITY : Integer.parseInt(priority)); } } @@ -781,8 +792,29 @@ public void endElement(String uri, String localName, String qName) { @Override public void error(SAXParseException e) throws SAXException { - if (m_validate) { - throw e; + if (!m_doctypeDeclared) { + // Without a doctype a validating parser only ever complains that no grammar was found, which + // would turn the existing "you should add a " hint into a hard failure. + return; + } + switch (m_validationMode) { + case STRICT: + throw e; + case WARN: + Logger.getLogger(TestNGContentHandler.class) + .warn( + "The suite file [" + + m_fileName + + "] does not conform to " + + Parser.TESTNG_DTD + + ": " + + e.getMessage() + + ". Run with [-D" + + RuntimeBehavior.XML_VALIDATION_MODE + + "=strict] to turn this into a failure."); + break; + case OFF: + break; } } 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..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,33 +14,68 @@ public abstract class XMLParser implements IFileParser { - private static final SAXParser m_saxParser; + private static final AutoCloseableLock lock = new AutoCloseableLock(); - static { - SAXParserFactory spf = loadSAXParserFactory(); + private static SAXParser m_saxParser; - if (supportsValidation(spf)) { - spf.setNamespaceAware(true); - spf.setValidating(true); - } + /** The mode {@link #m_saxParser} was configured for, so a change of mode can be noticed. */ + private static XmlValidationMode configuredFor; - SAXParser parser = null; - try { - parser = spf.newSAXParser(); - } catch (ParserConfigurationException | SAXException e) { - Logger.getLogger(XMLParser.class).error(e.getMessage(), e); + /** Whether {@link #m_saxParser} was built with DTD validation enabled. */ + private static boolean validating; + + /** + * Whether the next parse will validate against the TestNG DTD. Exposed so that tests can tell + * "validation is off in this JVM" apart from "this file is valid", instead of inferring it from a + * parse that does not fail -- an inference that cannot be made. + */ + static boolean isValidating() { + try (AutoCloseableLock ignore = lock.lock()) { + parser(); + return validating; } - m_saxParser = parser; } - private static final AutoCloseableLock lock = new AutoCloseableLock(); - public void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException { try (AutoCloseableLock ignore = lock.lock()) { - m_saxParser.parse(is, dh); + 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 = mode.isValidating() && supportsValidation(spf); + spf.setValidating(validating); + try { + m_saxParser = spf.newSAXParser(); + } catch (ParserConfigurationException | SAXException e) { + Logger.getLogger(XMLParser.class).error(e.getMessage(), e); + m_saxParser = null; + } + configuredFor = mode; + return m_saxParser; + } + /** * Tries to load a SAXParserFactory via SAXParserFactory.newInstance(). * @@ -57,12 +92,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..c8018bb31 --- /dev/null +++ b/testng-core/src/main/java/org/testng/xml/XmlValidationMode.java @@ -0,0 +1,69 @@ +package org.testng.xml; + +import java.util.Arrays; +import java.util.Locale; +import org.testng.internal.RuntimeBehavior; +import org.testng.log4testng.Logger; + +/** + * How strictly a suite file is checked against the TestNG DTD, selected with the {@code + * testng.xml.validation} system property. + * + *

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

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 { + + /** Do not validate at all. */ + OFF, + + /** Validate and report violations as warnings. The default. */ + WARN, + + /** Validate and fail on the first violation. */ + STRICT; + + private static final XmlValidationMode DEFAULT = WARN; + + public boolean isValidating() { + return this != OFF; + } + + /** + * The mode requested by the {@code testng.xml.validation} system property, falling back to {@link + * #WARN} when the property is absent or holds an unknown value. + */ + public static XmlValidationMode current() { + String requested = RuntimeBehavior.getXmlValidationMode(); + if (requested == null || requested.trim().isEmpty()) { + return DEFAULT; + } + try { + return valueOf(requested.trim().toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + Logger.getLogger(XmlValidationMode.class) + .warn( + "Unknown value [" + + requested + + "] for the system property [" + + RuntimeBehavior.XML_VALIDATION_MODE + + "]. Expected one of " + + Arrays.toString(values()) + + ". Falling back to [" + + DEFAULT + + "]."); + return DEFAULT; + } + } +} 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/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/SuiteDigest.java b/testng-core/src/test/java/org/testng/xml/SuiteDigest.java new file mode 100644 index 000000000..41d9013a4 --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/SuiteDigest.java @@ -0,0 +1,158 @@ +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()); + // 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()); + + 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())); + } + } + } + + 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 + * 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..a1b367cbf --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java @@ -0,0 +1,108 @@ +package org.testng.xml; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.testng.xml.SuiteCorpus.parseFile; +import static org.testng.xml.SuiteCorpus.parseString; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import javax.xml.parsers.SAXParserFactory; +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; + +/** + * 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", dataProviderClass = SuiteCorpus.class) + 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", dataProviderClass = SuiteCorpus.class) + 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)); + } + + /** + * 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", dataProviderClass = SuiteCorpus.class) + 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) + throws IOException, SAXException { + return SuiteCorpus.bundledDtdResolver().resolveEntity(publicId, systemId); + } + + @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); + } +} diff --git a/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java new file mode 100644 index 000000000..cc29a3e3f --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/XmlValidationTest.java @@ -0,0 +1,250 @@ +package org.testng.xml; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +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.SkipException; +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. + * + *

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

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 { + + 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 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() { + 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 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"); + + 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(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 strictModeAlsoRejectsWhenTheSuitePointsAtItsOwnDtd() throws Exception { + System.setProperty(RuntimeBehavior.XML_VALIDATION_MODE, "strict"); + Path directory = Files.createTempDirectory("testng-local-dtd"); + Path dtd = directory.resolve(Parser.TESTNG_DTD); + Path suite = directory.resolve("local-dtd-wrong-order.xml"); + // 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); + } + 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); + } + } + + /** + * 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; + } + } + + @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(); + } + + /** + * {@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(); + } +} 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-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..7d4975dc8 100644 --- a/testng-core/src/test/resources/testng.xml +++ b/testng-core/src/test/resources/testng.xml @@ -815,6 +815,10 @@ + + + + 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 @@ + + + + + + + + + + +