diff --git a/CHANGES.txt b/CHANGES.txt index f0895d9f3..b7cfe4f79 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,13 @@ Current (7.13.0) +Fixed: GITHUB-3318: Yaml.toYaml() produced YAML that could not be read back -- a duplicated "packages" key, sequence items written without "- ", keys indented at the column of the item they belong to, package filters written without a colon and under the plural keys "includes"/"excludes" the reader does not bind, and "suite-files" written under an unknown key and only for a suite that has child suites. The writer now builds a document and lets snakeyaml emit it, so quoting, escaping and indentation are correct by construction: a parameter valued "a,b" no longer reads back as two entries, and one valued "44.0" no longer reads back as a Double (Julien Herr) +New: GITHUB-3318: The YAML writer now also emits the suite-level groups, preserve-order, parent-module, guice-stage, allow-return-values, share-thread-pool-for-data-providers, the method selectors at both levels, class parameters and include descriptions, all of which were silently dropped (Julien Herr) +Fixed: DTD validation of suite files was silently disabled: the SAX validation feature was probed under an "https" identifier that no parser recognizes, so setValidating(true) was never reached and violations went unreported. Validation is enabled again, with a new testng.xml.validation=off|warn|strict system property; the default "warn" reports violations without failing the run (Julien Herr) +Fixed: XmlSuite.toXml() dropped the "description" attribute of , 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) 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/internal/Yaml.java b/testng-core/src/main/java/org/testng/internal/Yaml.java index 9e67dc9e8..ef3a9cd94 100644 --- a/testng-core/src/main/java/org/testng/internal/Yaml.java +++ b/testng-core/src/main/java/org/testng/internal/Yaml.java @@ -3,17 +3,24 @@ import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.InputStream; +import java.util.ArrayList; +import java.util.Collection; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.TreeMap; import java.util.function.Consumer; import org.testng.TestNGException; import org.testng.internal.objects.InstanceCreator; import org.testng.xml.XmlClass; +import org.testng.xml.XmlDefine; +import org.testng.xml.XmlGroups; import org.testng.xml.XmlInclude; import org.testng.xml.XmlPackage; import org.testng.xml.XmlScript; import org.testng.xml.XmlSuite; import org.testng.xml.XmlTest; +import org.yaml.snakeyaml.DumperOptions; import org.yaml.snakeyaml.LoaderOptions; import org.yaml.snakeyaml.TypeDescription; import org.yaml.snakeyaml.constructor.Constructor; @@ -73,236 +80,322 @@ public static XmlSuite parse(String filePath, InputStream is, boolean loadClasse return result; } - private static void maybeAdd(StringBuilder sb, String key, Object value, Object def) { - maybeAdd(sb, "", key, value, def); + /** + * Converts an {@link XmlSuite} into YAML. This method is allowed to be used by external tools + * (e.g. Eclipse). + * + *

The document is built as plain maps and lists and then handed to snakeyaml, which owns + * quoting, escaping and indentation. Writing the text by hand is what made the output of this + * method unreadable for years: a parameter valued {@code a,b}, {@code off} or {@code 2.0} needs a + * different treatment in each context, and the emitter already knows all of them. + * + *

Only the keys the YAML reader can bind are written, so that {@code parse -> toYaml -> parse} + * is lossless. What a suite file can carry and YAML cannot express is therefore left out, because + * no key would read it back: a test {@code time-out}, an include's invocation numbers, the suite + * level {@code group-by-instances} (the test level one is written), the object factory, {@code + * use-global-thread-pool}, a suite level {@code } or {@code } block (both + * are written for a test), and a test {@code script} -- which is already covered by the method + * selectors it is stored in. + * + * @param suite the suite to serialize + * @return the YAML representation of the suite + */ + public static StringBuilder toYaml(XmlSuite suite) { + return new StringBuilder(new org.yaml.snakeyaml.Yaml(dumperOptions()).dump(suiteToMap(suite))); } - private static void maybeAdd(StringBuilder sb, String sp, String key, Object value, Object def) { - if (value != null && !value.equals(def)) { - sb.append(sp).append(key).append(": ").append(value).append("\n"); - } + private static DumperOptions dumperOptions() { + DumperOptions options = new DumperOptions(); + options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK); + options.setIndent(2); + // Indent sequence items under the key they belong to, the shape every hand written suite file + // in the corpus already uses. + options.setIndicatorIndent(2); + options.setIndentWithIndicator(true); + // Written files must not differ between platforms. + options.setLineBreak(DumperOptions.LineBreak.UNIX); + // Never fold a line. Folding a plain scalar at the first of two consecutive spaces collapses + // them, which would silently rewrite a "depends-on" listing several groups. + options.setWidth(Integer.MAX_VALUE); + options.setSplitLines(false); + return options; } - /* - * The main entry point to convert an XmlSuite into YAML. This method is allowed to be used by - * external tools (e.g. Eclipse). - */ - public static StringBuilder toYaml(XmlSuite suite) { - StringBuilder result = new StringBuilder(); - - maybeAdd(result, "name", suite.getName(), null); - maybeAdd(result, "verbose", suite.getVerbose(), XmlSuite.DEFAULT_VERBOSE); - maybeAdd(result, "threadCount", suite.getThreadCount(), XmlSuite.DEFAULT_THREAD_COUNT); - maybeAdd( + private static Map suiteToMap(XmlSuite suite) { + Map result = new LinkedHashMap<>(); + result.put("name", suite.getName()); + // The verbosity is compared against the level that is actually in effect rather than against + // XmlSuite.DEFAULT_VERBOSE: getVerbose() falls back to -Dtestng.default.verbose, so comparing + // against the constant would write out a value the suite never declared, and would make the + // output depend on the JVM it was produced in. + putIfDifferent(result, "verbose", suite.getVerbose(), RuntimeBehavior.getDefaultVerboseLevel()); + putIfDifferent(result, "parallel", suite.getParallel(), XmlSuite.DEFAULT_PARALLEL); + putIfDifferent(result, "threadCount", suite.getThreadCount(), XmlSuite.DEFAULT_THREAD_COUNT); + putIfDifferent( result, "dataProviderThreadCount", suite.getDataProviderThreadCount(), - XmlSuite.DEFAULT_DATA_PROVIDER_THREAD_COUNT); - maybeAdd(result, "timeOut", suite.getTimeOut(), null); - maybeAdd(result, "parallel", suite.getParallel(), XmlSuite.DEFAULT_PARALLEL); - maybeAdd( + defaultDataProviderThreadCount()); + putIfPresent(result, "timeOut", suite.getTimeOut()); + putIfDifferent( result, "configFailurePolicy", - suite.getConfigFailurePolicy().toString(), + suite.getConfigFailurePolicy(), XmlSuite.DEFAULT_CONFIG_FAILURE_POLICY); - maybeAdd( + putIfDifferent( result, "skipFailedInvocationCounts", suite.skipFailedInvocationCounts(), XmlSuite.DEFAULT_SKIP_FAILED_INVOCATION_COUNTS); - - toYaml(result, "", suite.getParameters()); - toYaml(result, suite.getPackages()); - - if (!suite.getListeners().isEmpty()) { - result.append("listeners:\n"); - toYaml(result, " ", suite.getListeners()); - } - - if (!suite.getPackages().isEmpty()) { - result.append("packages:\n"); - toYaml(result, suite.getPackages()); - } - if (!suite.getTests().isEmpty()) { - result.append("tests:\n"); - for (XmlTest t : suite.getTests()) { - toYaml(result, t); - } - } - - if (!suite.getChildSuites().isEmpty()) { - result.append("suite-files:\n"); - toYaml(result, " ", suite.getSuiteFiles()); + putIfDifferent( + result, "preserveOrder", suite.getPreserveOrder(), XmlSuite.DEFAULT_PRESERVE_ORDER); + putIfDifferent( + result, + "allowReturnValues", + suite.getAllowReturnValues(), + XmlSuite.DEFAULT_ALLOW_RETURN_VALUES); + putIfDifferent( + result, + "shareThreadPoolForDataProviders", + suite.isShareThreadPoolForDataProviders(), + XmlSuite.DEFAULT_SHARE_THREAD_POOL_FOR_DATA_PROVIDERS); + putIfPresent(result, "parentModule", suite.getParentModule()); + putIfPresent(result, "guiceStage", suite.getGuiceStage()); + putIfPresent(result, "parameters", parameters(suite.getParameters())); + putIfPresent(result, "listeners", copyOf(suite.getListeners())); + putRunGroups(result, suite.getGroups()); + putIfPresent(result, "packages", packagesToNodes(suite.getXmlPackages())); + putIfPresent(result, "methodSelectors", selectorsToNodes(suite.getMethodSelectors())); + putIfPresent(result, "suiteFiles", copyOf(suite.getSuiteFiles())); + + List tests = new ArrayList<>(); + for (XmlTest test : suite.getTests()) { + tests.add(testToMap(test)); } - + putIfPresent(result, "tests", tests); return result; } - /** Convert a XmlTest into YAML */ - private static void toYaml(StringBuilder result, XmlTest t) { - String sp2 = " ".repeat(2); - result.append(" ").append("- name: ").append(t.getName()).append("\n"); - - maybeAdd(result, sp2, "verbose", t.getVerbose(), XmlSuite.DEFAULT_VERBOSE); - maybeAdd(result, sp2, "timeOut", t.getTimeOut(), null); - maybeAdd(result, sp2, "parallel", t.getParallel(), XmlSuite.DEFAULT_PARALLEL); - maybeAdd( + /** + * Values a test inherits from its suite are compared against the suite rather than against the + * defaults, and dropped when they match. The getters of {@link XmlTest} fall back to the suite, + * so writing them unconditionally would materialize the suite's values into every test. + */ + private static Map testToMap(XmlTest test) { + XmlSuite suite = test.getSuite(); + Map result = new LinkedHashMap<>(); + result.put("name", test.getName()); + putIfDifferent(result, "verbose", test.getVerbose(), suite.getVerbose()); + putIfDifferent(result, "parallel", test.getParallel(), suite.getParallel()); + putIfDifferent(result, "threadCount", test.getThreadCount(), suite.getThreadCount()); + putIfDifferent(result, "preserveOrder", test.getPreserveOrder(), suite.getPreserveOrder()); + putIfDifferent( + result, "groupByInstances", test.getGroupByInstances(), suite.getGroupByInstances()); + putIfDifferent( + result, "allowReturnValues", test.getAllowReturnValues(), suite.getAllowReturnValues()); + putIfDifferent( result, - sp2, "skipFailedInvocationCounts", - t.skipFailedInvocationCounts(), - XmlSuite.DEFAULT_SKIP_FAILED_INVOCATION_COUNTS); - - maybeAdd(result, "preserveOrder", sp2, t.getPreserveOrder(), XmlSuite.DEFAULT_PRESERVE_ORDER); - - toYaml(result, sp2, t.getLocalParameters()); + test.skipFailedInvocationCounts(), + suite.skipFailedInvocationCounts()); + putIfPresent(result, "parameters", parameters(test.getLocalParameters())); + putRunGroups(result, test.getXmlGroups()); + putMetaGroups(result, test.getXmlGroups()); + putIfPresent(result, "xmlDependencyGroups", sorted(test.getXmlDependencyGroups())); + putIfPresent(result, "methodSelectors", selectorsToNodes(test.getMethodSelectors())); + putIfPresent(result, "packages", packagesToNodes(test.getXmlPackages())); + putIfPresent(result, "classes", classesToNodes(test.getXmlClasses())); + return result; + } - if (!t.getIncludedGroups().isEmpty()) { - result - .append(sp2) - .append("includedGroups: [ ") - .append(Utils.join(t.getIncludedGroups(), ",")) - .append(" ]\n"); + /** + * The {@code } block is read from the model it was parsed into, never from {@code + * getIncludedGroups()}: on a test that getter returns the union with the suite's groups, and on a + * suite it delegates to the parent suite. Either one would duplicate groups on the way out. + */ + private static void putRunGroups(Map result, XmlGroups groups) { + if (groups == null || groups.getRun() == null) { + return; } + putIfPresent(result, "includedGroups", copyOf(groups.getRun().getIncludes())); + putIfPresent(result, "excludedGroups", copyOf(groups.getRun().getExcludes())); + } - if (!t.getExcludedGroups().isEmpty()) { - result - .append(sp2) - .append("excludedGroups: [ ") - .append(Utils.join(t.getExcludedGroups(), ",")) - .append(" ]\n"); + /** + * Meta groups are written for a test only. {@code XmlSuite} has no {@code metaGroups} property, + * so a suite level {@code } has no key to be read back through and writing one would make + * the file unloadable. + */ + private static void putMetaGroups(Map result, XmlGroups groups) { + if (groups == null) { + return; } - - if (!t.getXmlDependencyGroups().isEmpty()) { - result.append(sp2).append(sp2).append("xmlDependencyGroups:\n"); - t.getXmlDependencyGroups() - .forEach( - (k, v) -> - result - .append(sp2) - .append(sp2) - .append(sp2) - .append(k) - .append(": ") - .append(v) - .append("\n")); + Map metaGroups = new TreeMap<>(); + for (XmlDefine define : groups.getDefines()) { + metaGroups.put(define.getName(), copyOf(define.getIncludes())); } + putIfPresent(result, "metaGroups", metaGroups); + } - Map> mg = t.getMetaGroups(); - if (!mg.isEmpty()) { - result.append(sp2).append("metaGroups: { "); - boolean first = true; - for (Map.Entry> entry : mg.entrySet()) { - if (!first) { - result.append(", "); - } - result - .append(entry.getKey()) - .append(": [ ") - .append(Utils.join(entry.getValue(), ",")) - .append(" ] "); - first = false; - } - result.append(" }\n"); + private static List packagesToNodes(List packages) { + List result = new ArrayList<>(); + for (XmlPackage xmlPackage : packages) { + result.add(packageToNode(xmlPackage)); } + return result; + } - if (!t.getXmlPackages().isEmpty()) { - result.append(sp2).append(sp2).append("xmlPackages:\n"); - for (XmlPackage xp : t.getXmlPackages()) { - toYaml(result, sp2 + " - ", xp); - } + /** + * A package with no filter collapses to its name, the form the reader builds through {@code + * XmlPackage(String)} and the one the hand written fixtures use. + * + *

{@code getXmlClasses()} is deliberately not called: it scans the classpath, which has + * nothing to do with what the suite file says. + */ + private static Object packageToNode(XmlPackage xmlPackage) { + List include = xmlPackage.getInclude(); + List exclude = xmlPackage.getExclude(); + if (include.isEmpty() && exclude.isEmpty()) { + return xmlPackage.getName(); } + Map result = new LinkedHashMap<>(); + result.put("name", xmlPackage.getName()); + // Singular, because that is what the reader binds -- XmlPackage.setInclude/setExclude. + putIfPresent(result, "include", copyOf(include)); + putIfPresent(result, "exclude", copyOf(exclude)); + return result; + } - if (!t.getXmlClasses().isEmpty()) { - result.append(sp2).append("classes:\n"); - for (XmlClass xc : t.getXmlClasses()) { - toYaml(result, sp2 + " ", xc); - } + private static List classesToNodes(List classes) { + List result = new ArrayList<>(); + for (XmlClass xmlClass : classes) { + result.add(classToNode(xmlClass)); } - - result.append("\n"); + return result; } - private static void toYaml(StringBuilder result, String sp2, XmlClass xc) { - List im = xc.getIncludedMethods(); - List em = xc.getExcludedMethods(); - String name = im.isEmpty() && em.isEmpty() ? "" : "name: "; - - result.append(sp2).append("- ").append(name).append(xc.getName()).append("\n"); - if (!im.isEmpty()) { - result.append(sp2).append(" includedMethods:\n"); - for (XmlInclude xi : im) { - toYaml(result, sp2 + " ", xi); - } + private static Object classToNode(XmlClass xmlClass) { + Map parameters = parameters(xmlClass.getLocalParameters()); + List includedMethods = includesToNodes(xmlClass.getIncludedMethods()); + List excludedMethods = xmlClass.getExcludedMethods(); + if (parameters.isEmpty() && includedMethods.isEmpty() && excludedMethods.isEmpty()) { + return xmlClass.getName(); } + Map result = new LinkedHashMap<>(); + result.put("name", xmlClass.getName()); + putIfPresent(result, "parameters", parameters); + putIfPresent(result, "includedMethods", includedMethods); + putIfPresent(result, "excludedMethods", copyOf(excludedMethods)); + return result; + } - if (!em.isEmpty()) { - result.append(sp2).append(" excludedMethods:\n"); - toYaml(result, sp2 + " ", em); + private static List includesToNodes(List includes) { + List result = new ArrayList<>(); + for (XmlInclude include : includes) { + result.add(includeToNode(include)); } + return result; } - private static void toYaml(StringBuilder result, String sp, XmlInclude xi) { - result.append(sp).append("- name: ").append(xi.getName()).append("\n"); - String sp2 = sp + " "; - toYaml(result, sp2, xi.getLocalParameters()); + /** + * The invocation numbers of an include are not written: {@link XmlInclude} exposes them through + * {@code addInvocationNumbers}, not through a setter, so no key would read them back. + */ + private static Object includeToNode(XmlInclude include) { + Map parameters = parameters(include.getLocalParameters()); + if (parameters.isEmpty() && include.getDescription() == null) { + return include.getName(); + } + Map result = new LinkedHashMap<>(); + result.put("name", include.getName()); + putIfPresent(result, "description", include.getDescription()); + putIfPresent(result, "parameters", parameters); + return result; } - private static void toYaml(StringBuilder result, String sp, List strings) { - for (String l : strings) { - result.append(sp).append("- ").append(l).append("\n"); + private static List selectorsToNodes(List selectors) { + List result = new ArrayList<>(); + for (org.testng.xml.XmlMethodSelector selector : selectors) { + result.add(selectorToMap(selector)); } + return result; } - private static void toYaml(StringBuilder sb, List packages) { - if (!packages.isEmpty()) { - sb.append("packages:\n"); - for (XmlPackage p : packages) { - toYaml(sb, " ", p); - } - } - for (XmlPackage p : packages) { - toYaml(sb, " ", p); + /** + * Method selectors are written flat, because that is how the reader takes them apart: {@code + * ConstructXmlScript} reads {@code className}, {@code priority}, {@code expression} and {@code + * language} off the mapping itself and ignores anything else. + */ + private static Map selectorToMap(org.testng.xml.XmlMethodSelector selector) { + Map result = new LinkedHashMap<>(); + putIfPresent(result, "className", selector.getClassName()); + putIfDifferent( + result, + "priority", + selector.getPriority(), + org.testng.xml.XmlMethodSelector.DEFAULT_PRIORITY); + XmlScript script = selector.getScript(); + if (script != null) { + putIfPresent(result, "expression", script.getExpression()); + putIfPresent(result, "language", script.getLanguage()); } + return result; } - private static void toYaml(StringBuilder sb, String sp, XmlPackage p) { - sb.append(sp).append("name: ").append(p.getName()).append("\n"); + /** + * Parameters are read through a raw map on purpose. The reader has no type description for them, + * so snakeyaml resolves {@code true} or {@code 44.0} to a {@link Boolean} or a {@link Double} and + * stores it in a {@code Map} through an erased setter -- iterating it as strings + * would throw. Handing those values back to the emitter as they are makes it quote them, which is + * what puts a {@link String} back in the map on the next read. + */ + @SuppressWarnings({"unchecked", "rawtypes"}) + private static Map parameters(Map parameters) { + return sorted((Map) parameters); + } - generateIncludeExclude(sb, sp, "includes", p.getInclude()); - generateIncludeExclude(sb, sp, "excludes", p.getExclude()); + /** Sorted, because the model stores these in hash maps and a file must not depend on that. */ + private static Map sorted(Map map) { + return new TreeMap<>(map); } - private static void generateIncludeExclude( - StringBuilder sb, String sp, String key, List includes) { - if (!includes.isEmpty()) { - sb.append(sp).append(" ").append(key).append("\n"); - for (String inc : includes) { - sb.append(sp).append(" ").append(inc); + private static List copyOf(List values) { + return new ArrayList<>(values); + } + + private static int defaultDataProviderThreadCount() { + String property = RuntimeBehavior.getDefaultDataProviderThreadCount(); + try { + if (!property.trim().isEmpty()) { + return Integer.parseInt(property); } + } catch (NumberFormatException ignored) { + // getDataProviderThreadCount() falls back to the suite's value in that case, so do we. } + return XmlSuite.DEFAULT_DATA_PROVIDER_THREAD_COUNT; } - private static void mapToYaml(Map map, StringBuilder out) { - if (!map.isEmpty()) { - out.append("{ "); - boolean first = true; - for (Map.Entry e : map.entrySet()) { - if (!first) { - out.append(", "); - } - first = false; - out.append(e.getKey()).append(": ").append(e.getValue()); - } - out.append(" }\n"); + private static void putIfDifferent( + Map result, String key, Object value, Object defaultValue) { + if (value != null && !value.equals(defaultValue)) { + result.put(key, value instanceof Enum ? value.toString() : value); } } - private static void toYaml(StringBuilder sb, String sp, Map parameters) { - if (!parameters.isEmpty()) { - sb.append(sp).append("parameters").append(": "); - mapToYaml(parameters, sb); + private static void putIfPresent(Map result, String key, Object value) { + if (value == null) { + return; + } + if (value instanceof String && ((String) value).isEmpty()) { + return; + } + if (value instanceof Collection && ((Collection) value).isEmpty()) { + return; + } + if (value instanceof Map && ((Map) value).isEmpty()) { + return; } + result.put(key, value); } private static class TestNGConstructor extends Constructor { 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..bc307b4be 100644 --- a/testng-core/src/main/java/org/testng/xml/XMLParser.java +++ b/testng-core/src/main/java/org/testng/xml/XMLParser.java @@ -7,38 +7,62 @@ import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; import org.testng.TestNGException; -import org.testng.internal.AutoCloseableLock; import org.testng.log4testng.Logger; import org.xml.sax.SAXException; import org.xml.sax.helpers.DefaultHandler; public abstract class XMLParser implements IFileParser { - private static final SAXParser m_saxParser; + /** + * Whether the next parse will validate against the TestNG DTD. Exposed so that tests can tell + * "validation is off in this JVM" apart from "this file is valid", instead of inferring it from a + * parse that does not fail -- an inference that cannot be made. + */ + static boolean isValidating() { + return configureValidation(loadSAXParserFactory()); + } - static { + /** + * Each parse gets its own parser. + * + *

A single shared {@link SAXParser} was kept behind a class wide lock, which made every parse + * in the JVM wait for every other one. That is not a matter of contention only: entity resolution + * happens inside {@code parse}, and {@link TestNGContentHandler} resolves an unknown system id + * over HTTP with no connect or read timeout, so one suite pointing at an unreachable DTD mirror + * would block suite parsing everywhere until the socket gave up. + * + *

The parser was shared because building one was assumed to be expensive. Measured, {@code + * SAXParserFactory.newInstance()} and {@code newSAXParser()} cost about 20 microseconds each -- + * nothing against reading a suite file, let alone against fetching a DTD. Building per parse also + * removes the mutable static state that had to be invalidated whenever the validation mode + * changed, so the mode in effect is now simply read at each parse. + */ + public void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException { SAXParserFactory spf = loadSAXParserFactory(); - - if (supportsValidation(spf)) { - spf.setNamespaceAware(true); - spf.setValidating(true); - } - - SAXParser parser = null; + configureValidation(spf); + SAXParser parser; try { parser = spf.newSAXParser(); } catch (ParserConfigurationException | SAXException e) { Logger.getLogger(XMLParser.class).error(e.getMessage(), e); + throw new TestNGException("No SAXParser could be configured to read suite files.", e); } - m_saxParser = parser; + parser.parse(is, dh); } - private static final AutoCloseableLock lock = new AutoCloseableLock(); - - public void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException { - try (AutoCloseableLock ignore = lock.lock()) { - m_saxParser.parse(is, dh); - } + /** + * Configures the factory for the validation mode currently in effect, and reports whether the + * parsers it builds will validate. Pinning the mode to whatever was set when this class was first + * loaded made {@code testng.xml.validation} silently ineffective for anything that sets it later + * -- the very failure mode this setting exists to fix. + */ + private static boolean configureValidation(SAXParserFactory spf) { + // Namespace awareness is deliberately left off: DTD validation does not need it, suite files + // are not namespaced, and turning it on would make an unbound prefix fatal and an xmlns + // attribute a validity error -- neither of which has anything to do with validating a suite. + boolean validating = XmlValidationMode.current().isValidating() && supportsValidation(spf); + spf.setValidating(validating); + return validating; } /** @@ -57,12 +81,20 @@ private static SAXParserFactory loadSAXParserFactory() { } } - /** Tests if the current 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/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/XMLParserTest.java b/testng-core/src/test/java/org/testng/xml/XMLParserTest.java new file mode 100644 index 000000000..3750e0c6b --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/XMLParserTest.java @@ -0,0 +1,97 @@ +package org.testng.xml; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.ByteArrayInputStream; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import org.testng.annotations.Test; +import org.xml.sax.Attributes; +import org.xml.sax.helpers.DefaultHandler; + +public class XMLParserTest { + + private static final String SUITE = + ""; + + /** + * A parse must not wait for another one to finish. + * + *

{@code XMLParser} used to keep one shared {@link javax.xml.parsers.SAXParser} behind a class + * wide lock held for the whole parse. Entity resolution happens inside that call, and {@link + * TestNGContentHandler} resolves an unknown system id over HTTP with no timeout, so a single + * suite pointing at an unreachable DTD mirror blocked suite parsing everywhere. + * + *

The blocking is simulated in the content handler rather than through a DTD, so the test does + * not depend on the JAXP implementation resolving an external subset, nor on the network. + * + *

Under the old code this test does not fail an assertion -- it hangs, which is the point, so + * it carries a timeout. + */ + @Test(timeOut = 30_000) + public void aBlockedParseDoesNotHoldUpAnotherOne() throws Exception { + CountDownLatch parsing = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + AtomicReference blockedFailure = new AtomicReference<>(); + + Thread blocked = + new Thread( + () -> { + try { + parse( + new DefaultHandler() { + @Override + public void startElement(String u, String l, String name, Attributes a) + throws org.xml.sax.SAXException { + parsing.countDown(); + try { + release.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new org.xml.sax.SAXException(e); + } + } + }); + } catch (Throwable t) { + blockedFailure.set(t); + } + }, + "blocked-parse"); + blocked.start(); + + try { + assertThat(parsing.await(10, TimeUnit.SECONDS)) + .as("the first parse should have reached the content handler") + .isTrue(); + + // Under the old code this call waits for the lock the blocked parse is holding, forever. + NameCollector collector = new NameCollector(); + parse(collector); + + assertThat(collector.suiteName).isEqualTo("s"); + } finally { + release.countDown(); + blocked.join(10_000); + } + + assertThat(blockedFailure.get()).isNull(); + } + + private static void parse(DefaultHandler handler) throws Exception { + byte[] bytes = SUITE.getBytes(StandardCharsets.UTF_8); + new SuiteXmlParser().parse(new ByteArrayInputStream(bytes), handler); + } + + private static final class NameCollector extends DefaultHandler { + private String suiteName; + + @Override + public void startElement(String uri, String localName, String name, Attributes attributes) { + if ("suite".equals(name)) { + suiteName = attributes.getValue("name"); + } + } + } +} 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..5b4bc2de6 --- /dev/null +++ b/testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java @@ -0,0 +1,165 @@ +package org.testng.xml; + +import static org.assertj.core.api.Assertions.assertThat; +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.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 + * 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)); + } + + /** + * 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 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("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 theParserValidatesSuiteFilesByDefault() { + 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 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 offModeStopsTheParserFromValidating() { + 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/test/yaml/YamlRoundTripTest.java b/testng-core/src/test/java/test/yaml/YamlRoundTripTest.java new file mode 100644 index 000000000..e61473a32 --- /dev/null +++ b/testng-core/src/test/java/test/yaml/YamlRoundTripTest.java @@ -0,0 +1,163 @@ +package test.yaml; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static test.SimpleBaseTest.getPathToResource; + +import java.io.ByteArrayInputStream; +import java.io.FileNotFoundException; +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.stream.Stream; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; +import org.testng.internal.Yaml; +import org.testng.xml.SuiteDigest; +import org.testng.xml.SuiteXmlParser; +import org.testng.xml.XmlRoundTripTest; +import org.testng.xml.XmlSuite; +import org.yaml.snakeyaml.LoaderOptions; +import org.yaml.snakeyaml.constructor.SafeConstructor; + +/** + * Characterization tests over every YAML file of the test corpus, pinning the YAML reader ({@link + * Yaml#parse}) and the YAML writer ({@link Yaml#toYaml}) as a pair -- the counterpart of {@code + * XmlRoundTripTest} for the other suite format. + * + *

Four invariants are checked over the YAML corpus, because none of them is sufficient on its + * own: the output must load under a plain YAML parser, which is the property the writer used to + * violate outright; it must be a fixed point, which pins key selection and layout; the parsed model + * must survive unchanged, which pins the data (see {@link SuiteDigest}); and it must contain no + * anchor, since an accidentally shared collection produces an alias that loads perfectly well and + * would slip past the other three. + * + *

A fifth one runs over the XML corpus, since that is what the {@code Converter} CLI converts + * and it reaches constructs no YAML fixture can declare. + */ +public class YamlRoundTripTest { + + /** + * The predicate of GITHUB-3318, stated so that it does not depend on TestNG's own binding: what + * {@code toYaml} writes must be readable by any YAML parser. + * + *

Duplicate keys are rejected rather than tolerated, because a writer that emits the same + * mapping key several times -- {@code packages:} used to come out three times -- produces a + * document that snakeyaml accepts by default, silently keeping the last occurrence. + */ + @Test(dataProvider = "yamlSuites") + public void emittedYamlLoadsUnderAPlainYamlParser(String suiteFile) throws IOException { + String emitted = Yaml.toYaml(parseFile(suiteFile)).toString(); + + LoaderOptions options = new LoaderOptions(); + options.setAllowDuplicateKeys(false); + org.yaml.snakeyaml.Yaml plainYaml = new org.yaml.snakeyaml.Yaml(new SafeConstructor(options)); + + assertThat(plainYaml.load(emitted)) + .as("the YAML written for %s must load under a plain YAML parser:%n%s", suiteFile, emitted) + .isInstanceOf(java.util.Map.class); + } + + @Test(dataProvider = "yamlSuites") + public void emittedYamlIsAFixedPoint(String suiteFile) throws IOException { + String firstPass = Yaml.toYaml(parseFile(suiteFile)).toString(); + String secondPass = Yaml.toYaml(parseString(suiteFile, firstPass)).toString(); + + assertThat(secondPass) + .as("re-writing the suite parsed back from %s must be a fixed point", suiteFile) + .isEqualTo(firstPass); + } + + @Test(dataProvider = "yamlSuites") + public void suiteContentSurvivesTheRoundTrip(String suiteFile) throws IOException { + XmlSuite parsedFromFile = parseFile(suiteFile); + XmlSuite reparsed = parseString(suiteFile, Yaml.toYaml(parsedFromFile).toString()); + + assertThat(SuiteDigest.of(reparsed)) + .as( + "the suite parsed back from the YAML written for %s must carry the same data", + suiteFile) + .isEqualTo(SuiteDigest.of(parsedFromFile)); + } + + /** + * Putting the same collection instance in two places of the document makes snakeyaml emit an + * anchor and an alias. That still loads, and it still round trips, so only an assertion on the + * text catches it -- and a suite file full of {@code *id001} is not something to hand to a user. + */ + @Test(dataProvider = "yamlSuites") + public void emittedYamlUsesNoAnchors(String suiteFile) throws IOException { + String emitted = Yaml.toYaml(parseFile(suiteFile)).toString(); + + assertThat(emitted) + .as("the YAML written for %s must not reference shared nodes through aliases", suiteFile) + .doesNotContainPattern("&id\\d+"); + } + + /** + * The other direction, which is what the {@code Converter} CLI does: an XML suite must convert to + * YAML the reader accepts. + * + *

Only loadability is asserted, not the round trip. XML expresses more than the YAML schema + * does -- a suite level {@code } has no key, and the reader numbers includes from zero + * whereas the XML parser numbers them across the whole class -- so comparing digests would fail + * for reasons that have nothing to do with the writer. Loadability alone is enough to catch a key + * being written that nothing can read back, which the YAML corpus cannot: it can only contain + * what YAML can already express. + */ + @Test(dataProvider = "suiteFiles", dataProviderClass = XmlRoundTripTest.class) + public void xmlSuitesConvertToLoadableYaml(String suiteFile) throws IOException { + Path path = Paths.get(getPathToResource(suiteFile)); + XmlSuite xmlSuite; + try (InputStream stream = Files.newInputStream(path)) { + xmlSuite = new SuiteXmlParser().parse(suiteFile, stream, false); + } + String emitted = Yaml.toYaml(xmlSuite).toString(); + + assertThatCode(() -> parseString(suiteFile, emitted)) + .as("the YAML written for %s must be readable back:%n%s", suiteFile, emitted) + .doesNotThrowAnyException(); + } + + /** + * Every YAML file of the test corpus. + * + *

The filter is the extension alone, because that is exactly what {@code YamlParser.accept} + * promises: a {@code .yaml} or {@code .yml} file under the resources root is a suite file. Adding + * one therefore extends the corpus without touching this class. + */ + @DataProvider(name = "yamlSuites") + public static Object[][] yamlSuites() throws IOException { + Path root = Paths.get(getPathToResource("")); + try (Stream paths = Files.walk(root)) { + return paths + .filter(Files::isRegularFile) + .filter(YamlRoundTripTest::isYaml) + .sorted() + .map(path -> new Object[] {root.relativize(path).toString()}) + .toArray(Object[][]::new); + } + } + + private static boolean isYaml(Path path) { + String name = path.getFileName().toString(); + return name.endsWith(".yaml") || name.endsWith(".yml"); + } + + private static XmlSuite parseFile(String suiteFile) throws IOException { + Path path = Paths.get(getPathToResource(suiteFile)); + try (InputStream stream = Files.newInputStream(path)) { + // Classes are not loaded, so that fixtures naming a class that does not exist -- which is + // what yaml/suiteWithNonExistentTest.yaml is for -- are part of the corpus like any other. + return Yaml.parse(suiteFile, stream, false); + } + } + + private static XmlSuite parseString(String suiteFile, String yaml) throws FileNotFoundException { + byte[] bytes = yaml.getBytes(StandardCharsets.UTF_8); + return Yaml.parse(suiteFile, new ByteArrayInputStream(bytes), false); + } +} diff --git a/testng-core/src/test/java/test/yaml/YamlTest.java b/testng-core/src/test/java/test/yaml/YamlTest.java index c96a8595f..d9bffaab0 100644 --- a/testng-core/src/test/java/test/yaml/YamlTest.java +++ b/testng-core/src/test/java/test/yaml/YamlTest.java @@ -2,16 +2,16 @@ import static org.assertj.core.api.Assertions.assertThat; +import java.io.ByteArrayInputStream; import java.io.File; import java.io.FileInputStream; +import java.io.FileNotFoundException; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Paths; import java.util.Collection; import java.util.List; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import org.testng.internal.Yaml; @@ -54,21 +54,17 @@ public void compareFiles(String name) throws IOException { @Test(description = "GITHUB-1787") public void testParameterInclusion() throws IOException { - SuiteXmlParser parser = new SuiteXmlParser(); String file = "src/test/resources/yaml/1787.xml"; - XmlSuite xmlSuite = parser.parse(file, new FileInputStream(file), false); - StringBuilder yaml = org.testng.internal.Yaml.toYaml(xmlSuite); - Matcher m = Pattern.compile("parameters:").matcher(yaml.toString()); - int count = 0; - while (m.find()) { - count++; - } - assertThat(count).isEqualTo(5); - File newSuite = File.createTempFile("suite", ".xml"); - newSuite.deleteOnExit(); - Files.write(newSuite.toPath(), yaml.toString().getBytes(StandardCharsets.UTF_8)); - assertThat(parser.parse(newSuite.getAbsolutePath(), new FileInputStream(file), false)) - .isEqualTo(xmlSuite); + XmlSuite xmlSuite = new SuiteXmlParser().parse(file, new FileInputStream(file), false); + + XmlSuite reparsed = parseYaml(file, Yaml.toYaml(xmlSuite).toString()); + + assertThat(reparsed.getParameters()).containsEntry("suiteLevel", "suiteValue"); + XmlTest test = reparsed.getTests().get(0); + assertThat(test.getLocalParameters()).containsEntry("testLevel", "testValue"); + assertThat(test.getClasses().get(0).getIncludedMethods()) + .extracting(include -> include.getLocalParameters().get("teqUid")) + .containsExactly("Teq1", "Teq2", "Teq3"); } @Test(description = "GITHUB-2078") @@ -78,9 +74,31 @@ public void testXmlDependencyGroups() throws IOException { new SuiteXmlParser().parse(actualXmlFile, new FileInputStream(actualXmlFile), false); String expectedYamlFile = "src/test/resources/yaml/2078.yaml"; String expectedYaml = - new String( - java.nio.file.Files.readAllBytes(Paths.get(expectedYamlFile)), StandardCharsets.UTF_8); - assertThat(Yaml.toYaml(actualXmlSuite).toString()).isEqualToNormalizingNewlines(expectedYaml); + new String(Files.readAllBytes(Paths.get(expectedYamlFile)), StandardCharsets.UTF_8); + + String actualYaml = Yaml.toYaml(actualXmlSuite).toString(); + + assertThat(actualYaml).isEqualToNormalizingNewlines(expectedYaml); + // The golden file cannot make this distinction on its own: folding the line at the first of + // the two spaces would collapse them, and the result would still read as a plausible list of + // dependencies. + assertThat(parseYaml(actualXmlFile, actualYaml).getTests().get(0).getXmlDependencyGroups()) + .containsEntry("c", "a b"); + } + + /** + * A suite level {@code } has no YAML key: {@code XmlSuite} exposes no {@code metaGroups} + * property, unlike {@code XmlTest}. Writing one anyway produced a file the reader rejects + * outright, and no YAML fixture can cover it because no YAML fixture can declare one. + */ + @Test + public void suiteLevelMetaGroupsAreNotWritten() throws IOException { + String file = "src/test/resources/xml/issue174.xml"; + XmlSuite xmlSuite = new SuiteXmlParser().parse(file, new FileInputStream(file), false); + + XmlSuite reparsed = parseYaml(file, Yaml.toYaml(xmlSuite).toString()); + + assertThat(reparsed.getIncludedGroups()).containsExactly("PlatformTests"); } @Test(description = "GITHUB-2689") @@ -113,6 +131,11 @@ public void testXmlTestIndex() throws IOException { } } + private static XmlSuite parseYaml(String fileName, String yaml) throws FileNotFoundException { + byte[] bytes = yaml.getBytes(StandardCharsets.UTF_8); + return Yaml.parse(fileName, new ByteArrayInputStream(bytes), false); + } + private Throwable getRootCause(Throwable throwable) { return throwable.getCause() != null ? getRootCause(throwable.getCause()) : throwable; } 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..00a6c2e2a 100644 --- a/testng-core/src/test/resources/testng.xml +++ b/testng-core/src/test/resources/testng.xml @@ -806,6 +806,7 @@ + @@ -815,6 +816,9 @@ + + + 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 @@ + + + + + + + + + + + diff --git a/testng-core/src/test/resources/yaml/2078.yaml b/testng-core/src/test/resources/yaml/2078.yaml index 566a49862..0eb47e8e0 100644 --- a/testng-core/src/test/resources/yaml/2078.yaml +++ b/testng-core/src/test/resources/yaml/2078.yaml @@ -1,12 +1,9 @@ name: My_Suite -verbose: 0 -configFailurePolicy: skip +guiceStage: DEVELOPMENT tests: - name: My_test - verbose: 0 xmlDependencyGroups: c: a b z: c - xmlPackages: - - name: test.yaml - + packages: + - test.yaml