From caca2bb1ae09ce78aa599196dc036cda5d55c96c Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 18:27:30 +0200 Subject: [PATCH 1/5] test(yaml): add round trip characterization for the YAML writer Yaml.toYaml() builds YAML by string concatenation and its output cannot be read back. Nothing catches that today: the only test that looks at the writer compares its output to a golden file, so it pins the broken output rather than detecting it -- and that golden file, yaml/2078.yaml, does not itself parse. The counterpart of XmlRoundTripTest for the other suite format, over every .yaml file of the corpus. Four invariants, none of them sufficient alone: - the output loads under a plain YAML parser, with duplicate keys rejected so that "packages:" being emitted three times is a failure rather than a silent last-one-wins; - re-writing the suite parsed back from it is a fixed point, which pins key selection and layout; - the parsed model survives unchanged, compared through SuiteDigest rather than XmlSuite.equals, which ignores 11 of the 26 fields; - no anchor is emitted, since a shared collection produces an alias that loads perfectly well and would slip past the other three. This commit is deliberately red: 37 of the 64 cases fail, and the failures are the defect list of GITHUB-3318 made executable. The fix follows. --- .../java/test/yaml/YamlRoundTripTest.java | 132 ++++++++++++++++++ testng-core/src/test/resources/testng.xml | 1 + 2 files changed, 133 insertions(+) create mode 100644 testng-core/src/test/java/test/yaml/YamlRoundTripTest.java 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 0000000000..970b7d716a --- /dev/null +++ b/testng-core/src/test/java/test/yaml/YamlRoundTripTest.java @@ -0,0 +1,132 @@ +package test.yaml; + +import static org.assertj.core.api.Assertions.assertThat; +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.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, 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. + */ +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+"); + } + + /** + * 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/resources/testng.xml b/testng-core/src/test/resources/testng.xml index 723e92166f..91a2b84097 100644 --- a/testng-core/src/test/resources/testng.xml +++ b/testng-core/src/test/resources/testng.xml @@ -805,6 +805,7 @@ + From 2c539d0ea2f131c8e3dfbe6c809af9e883189f10 Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 18:34:09 +0200 Subject: [PATCH 2/5] fix(yaml): emit the suite with snakeyaml instead of concatenating strings The document is now built as plain maps and lists and handed to snakeyaml, which owns quoting, escaping and indentation. Writing the text by hand is what made the output unparseable: keys emitted without a colon, sequence items without a "- ", "" keys indented at the column of the item they belong to, "packages:" written three times with each package written four times. It was also lossy in ways a string comparison cannot see. A parameter valued "a,b" came out unquoted inside a flow mapping and read back as the two entries "a" and "b=null"; one valued "44.0" came back as a Double in a Map. Both are now quoted by the emitter, because the value is a String whose plain form would resolve to another tag. Beyond the syntax: - package filters were written as "includes"/"excludes"; the reader binds "include"/"exclude", so they were dropped even once the layout was fixed; - "suite-files" was written under a key the reader does not know, and only when the suite had child suites although it is filled from getSuiteFiles(), so a suite parsed on its own lost them; - 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 were never written at all; - configFailurePolicy compared a String against a FailurePolicy and was therefore always written. Values a test inherits from its suite are compared against the suite and dropped when they match, and the block is read from the model it was parsed into rather than through getIncludedGroups(), which returns the union with the suite's groups. Otherwise the writer would materialize the suite into every test. Maps are sorted, since the model stores them in hash maps. The verbosity is compared against the level actually in effect rather than against XmlSuite.DEFAULT_VERBOSE: getVerbose() falls back to -Dtestng.default.verbose, so comparing against the constant wrote out a value the suite never declared and made the output depend on the JVM that produced it. That is where the stale "verbose: 0" of yaml/2078.yaml came from. Six values still have no key, because none would read them back: a test time-out, an include's invocation numbers, the suite level group-by-instances, the object factory, use-global-thread-pool, and a test script -- the last one already covered by the method selectors it is stored in. They are listed in the javadoc of toYaml. yaml/2078.yaml is regenerated. Being a .yaml file under the test resources it is now picked up by the round trip corpus, so the golden checks itself instead of pinning whatever the writer happened to produce. Its test keeps the golden comparison and adds the assertion the golden cannot make on its own: the dependency "a b" survives with both spaces. The GITHUB-1787 test counted occurrences of "parameters:" and then re-parsed the original XML file rather than the YAML it had just written, which made its second assertion vacuous. It now re-parses the emitted YAML and checks the parameters it was written for. Closes #3318 --- .../main/java/org/testng/internal/Yaml.java | 430 +++++++++++------- .../src/test/java/test/yaml/YamlTest.java | 46 +- testng-core/src/test/resources/yaml/2078.yaml | 9 +- 3 files changed, 286 insertions(+), 199 deletions(-) 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 9e67dc9e8a..316a2fa4c9 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,311 @@ 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. Six values a suite file can carry are therefore left out, because no key would + * read them 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}, 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())); + putGroups(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())); + putGroups(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 putGroups(Map result, XmlGroups groups) { + if (groups == null) { + return; } - - if (!t.getExcludedGroups().isEmpty()) { - result - .append(sp2) - .append("excludedGroups: [ ") - .append(Utils.join(t.getExcludedGroups(), ",")) - .append(" ]\n"); + if (groups.getRun() != null) { + putIfPresent(result, "includedGroups", copyOf(groups.getRun().getIncludes())); + putIfPresent(result, "excludedGroups", copyOf(groups.getRun().getExcludes())); } - - 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/test/java/test/yaml/YamlTest.java b/testng-core/src/test/java/test/yaml/YamlTest.java index c96a8595f1..c6492d2a18 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,16 @@ 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"); } @Test(description = "GITHUB-2689") @@ -113,6 +116,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/yaml/2078.yaml b/testng-core/src/test/resources/yaml/2078.yaml index 566a49862c..0eb47e8e0f 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 From 03137b4b0f0784dc5d705c5ad4ed35ad75e957cc Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 18:34:38 +0200 Subject: [PATCH 3/5] docs: record the YAML writer rewrite in CHANGES.txt --- CHANGES.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES.txt b/CHANGES.txt index c914c40548..f0f6580797 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,6 @@ 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) From 05fca203cec5a6b20c3ab6ab900c5ef377a102ea Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 18:36:57 +0200 Subject: [PATCH 4/5] fix(yaml): stop writing meta groups a suite cannot read back XmlSuite has no metaGroups property, unlike XmlTest, so a suite level came out under a key the reader rejects with "Unable to find property" -- the whole file became unloadable, not just that block. No YAML fixture can cover this, since no YAML fixture can declare a suite level define in the first place; the regression test converts xml/issue174.xml, which has one, and reads the result back. The block has the same shape and was already only written for a test. Both gaps are now listed in the javadoc of toYaml. --- .../main/java/org/testng/internal/Yaml.java | 35 ++++++++++++------- .../src/test/java/test/yaml/YamlTest.java | 15 ++++++++ 2 files changed, 38 insertions(+), 12 deletions(-) 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 316a2fa4c9..ef3a9cd945 100644 --- a/testng-core/src/main/java/org/testng/internal/Yaml.java +++ b/testng-core/src/main/java/org/testng/internal/Yaml.java @@ -90,10 +90,11 @@ public static XmlSuite parse(String filePath, InputStream is, boolean loadClasse * 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. Six values a suite file can carry are therefore left out, because no key would - * read them 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}, and a test {@code script} -- which is already covered by the method + * 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 @@ -162,7 +163,7 @@ private static Map suiteToMap(XmlSuite suite) { putIfPresent(result, "guiceStage", suite.getGuiceStage()); putIfPresent(result, "parameters", parameters(suite.getParameters())); putIfPresent(result, "listeners", copyOf(suite.getListeners())); - putGroups(result, suite.getGroups()); + putRunGroups(result, suite.getGroups()); putIfPresent(result, "packages", packagesToNodes(suite.getXmlPackages())); putIfPresent(result, "methodSelectors", selectorsToNodes(suite.getMethodSelectors())); putIfPresent(result, "suiteFiles", copyOf(suite.getSuiteFiles())); @@ -198,7 +199,8 @@ private static Map testToMap(XmlTest test) { test.skipFailedInvocationCounts(), suite.skipFailedInvocationCounts()); putIfPresent(result, "parameters", parameters(test.getLocalParameters())); - putGroups(result, test.getXmlGroups()); + 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())); @@ -207,17 +209,26 @@ private static Map testToMap(XmlTest test) { } /** - * The {@code } block is read from the model it was parsed into, never from {@code + * 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 putGroups(Map result, XmlGroups groups) { - if (groups == null) { + private static void putRunGroups(Map result, XmlGroups groups) { + if (groups == null || groups.getRun() == null) { return; } - if (groups.getRun() != null) { - putIfPresent(result, "includedGroups", copyOf(groups.getRun().getIncludes())); - putIfPresent(result, "excludedGroups", copyOf(groups.getRun().getExcludes())); + putIfPresent(result, "includedGroups", copyOf(groups.getRun().getIncludes())); + putIfPresent(result, "excludedGroups", copyOf(groups.getRun().getExcludes())); + } + + /** + * 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; } Map metaGroups = new TreeMap<>(); for (XmlDefine define : groups.getDefines()) { diff --git a/testng-core/src/test/java/test/yaml/YamlTest.java b/testng-core/src/test/java/test/yaml/YamlTest.java index c6492d2a18..d9bffaab0f 100644 --- a/testng-core/src/test/java/test/yaml/YamlTest.java +++ b/testng-core/src/test/java/test/yaml/YamlTest.java @@ -86,6 +86,21 @@ public void testXmlDependencyGroups() throws IOException { .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") public void testLoadClassesFlag() throws IOException { YamlParser yamlParser = new YamlParser(); From 743938255fe124a5adee863a9b7266ea732483ed Mon Sep 17 00:00:00 2001 From: Julien Herr Date: Thu, 30 Jul 2026 18:38:41 +0200 Subject: [PATCH 5/5] test(yaml): check that every XML suite converts to loadable YAML The YAML corpus can only contain what YAML already expresses, so it cannot cover a writer that emits a key nothing reads back -- that is how the suite level slipped through until it was found by reading the code. Converting the XML corpus is the other direction, and the one the Converter CLI actually performs. Only loadability is asserted, not the round trip: XML says more than the YAML schema does, so digests would differ for reasons that have nothing to do with the writer. Every suite fixture of the corpus, discovered by content so the set follows the tree rather than a list kept in this class. --- .../java/test/yaml/YamlRoundTripTest.java | 43 ++++++++++++++++--- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/testng-core/src/test/java/test/yaml/YamlRoundTripTest.java b/testng-core/src/test/java/test/yaml/YamlRoundTripTest.java index 970b7d716a..e61473a32b 100644 --- a/testng-core/src/test/java/test/yaml/YamlRoundTripTest.java +++ b/testng-core/src/test/java/test/yaml/YamlRoundTripTest.java @@ -1,6 +1,7 @@ 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; @@ -16,6 +17,8 @@ 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; @@ -25,12 +28,15 @@ * 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, 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. + *

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 { @@ -91,6 +97,31 @@ public void emittedYamlUsesNoAnchors(String suiteFile) throws IOException { .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. *