fix(yaml): rewrite the YAML writer so its output can be read back - #3327
Open
juherr wants to merge 11 commits into
Open
fix(yaml): rewrite the YAML writer so its output can be read back#3327juherr wants to merge 11 commits into
juherr wants to merge 11 commits into
Conversation
DTD validation of suite files has been silently dead: XMLParser probed the SAX validation feature under "https://xml.org/sax/features/validation". The feature name is a plain identifier, not a URL to dereference, so every conforming parser raises SAXNotRecognizedException, supportsValidation() returned false, and neither setValidating(true) nor setNamespaceAware(true) was ever reached. TestNGContentHandler.error() could therefore never fire and only well-formedness errors surfaced. Turning validation back on can reject suite files that have been accepted for years -- the DTD constrains the order of the children of <suite>, for instance -- so introduce testng.xml.validation=off|warn|strict and default it to warn, which reports violations without failing the run. Add round trip characterization tests over every suite file of the corpus (112 files, two invariants each: the serialized form must be a fixed point, and the parsed model must survive unchanged). They compare a canonical digest rather than XmlSuite.equals(), which ignores 11 of its 26 fields, among them the parameters, the groups and the method selectors. Those tests immediately exposed two data losses in the writer: - <include description="..."> was never written, so regenerating a suite (testng-failed.xml, for instance) dropped method descriptions. - <selector-class> omitted a priority of -1 while the parser reads a missing priority as 0. A negative method-selector priority is meaningful, since RunInfo#includeMethod short-circuits on it, so serializing a suite and reading it back changed its behaviour. Reader and writer now share XmlMethodSelector.DEFAULT_PRIORITY. Also align the emitted doctype with the DTD the parser actually resolves (1.1, not 1.0) and reorder testng-all.xml, the single real DTD violation in the corpus.
…M state
Follow-up to the previous commit, from reviewing it. Four CI jobs failed on a
single test, and the axes ruled out any single culprit: fr_FR failed on one job
and passed on another, and so did Windows and JDK 11. There were two causes.
Turkish locale. XmlValidationMode parsed the property with toUpperCase() and no
locale, so "strict" became "STRIC[I-with-dot]T", matched nothing, and silently
degraded to warn. Only strict was affected, since "off" and "warn" have no 'i' --
which is why exactly one test failed. The matrix has a tr_TR axis, so this was
live rather than theoretical. XmlSuite.getValidParallel already had the answer:
Locale.ROOT.
An inference that cannot be made. The test asked "does parsing an invalid file
throw?" to decide whether validation was wired in. But the shared SAXParser is
configured once per JVM, in a static initializer, so "this file is valid" and
"validation is off in this JVM" are indistinguishable from the outside. XMLParser
now records its decision and a test asserts it directly, with a message saying
what a failure means; the reporting tests drive a parser built in the test, so
they no longer depend on when the singleton was initialised.
Three defects in the feature itself, found while reviewing:
- Violations were discarded unless TestNG had substituted its own copy of the
DTD. m_validate means "we provided the DTD", not "a doctype was declared", so
a suite pointing at a local copy or a corporate mirror was never validated --
silently, even under strict. Tracked separately as m_doctypeDeclared.
- toXml() emitted two sibling <groups> elements for any suite with suite-level
groups, because the synthesized run block and XmlGroups both read the same
XmlGroups. The DTD allows one, so TestNG's own output did not validate and
strict would have rejected a regenerated testng-failed.xml. A new test now
validates the serialized form of every suite in the corpus against the DTD.
- setNamespaceAware(true) rode along with the validation switch. DTD validation
does not need it, and it makes an unbound prefix fatal and an xmlns attribute
a validity error. Removed.
The mode is now resolved once per parse instead of once per violation, so a
malformed suite no longer re-reads the property and re-logs for every error.
Verified: ./gradlew build is green, and so is ./gradlew :testng-core:test
-Dtestng.xml.validation=strict (14694 tests), which is the claim that matters --
strict has to accept everything TestNG itself produces.
Review follow-up. Three points were still valid against the current code; the fourth was already fixed and two more are noted below. XMLParser pinned the parser to whichever mode happened to be set when the class was first loaded. Anything setting testng.xml.validation later -- a second suite in a reused Surefire fork, an embedder, a test -- got no validation and no diagnostic, which is precisely the silent no-op this setting was introduced to remove. The parser is now rebuilt when the mode has changed since the last parse, under the lock it already holds, so moving away from OFF works. SuiteDigest compared only the included and excluded groups, which come from <run>. A round trip dropping a suite-level <define> or <dependencies> block was therefore invisible: verified by making the writer skip defines, which now fails two corpus files and previously failed none. The test asserting that the shared parser validates now skips under -Dtestng.xml.validation=off, a supported configuration it used to fail, and a new test pins that off actually reaches the parser rather than only the reporting. Not changed: the mode is already parsed with Locale.ROOT. Tracking the doctype via LexicalHandler.startDTD instead of resolveEntity would additionally cover a DOCTYPE with only an internal subset, but that means inlining the whole DTD in the suite file, which no TestNG suite does, and it would put a setProperty call on the parse path of every user for no practical gain. Verified: ./gradlew build, plus :testng-core:test under both -Dtestng.xml.validation=strict and =off, all green.
strictModeAlsoRejectsWhenTheSuitePointsAtItsOwnDtd created a temporary directory holding a copy of the DTD and never removed it. The suite runs in one fork per two cores, so every build left several behind: 73 had accumulated locally over this branch's test runs, each with an 8.5 KB copy of the DTD. Removed in a finally block so a failing assertion still reports rather than being masked by cleanup. Verified: ./gradlew build green, and the count of leftover directories under java.io.tmpdir is unchanged across a run instead of growing.
The finally block deleted the three paths in sequence, so a failure on the first left the other two behind, and any IOException it raised replaced the assertion failure that had actually happened. Deletion here is not hypothetically fallible: the entity resolver hands the DTD stream to InputSource without closing it, and on Windows a lingering handle blocks the delete. That would have turned a green test into an IOException with no trace of what was being asserted. Each path is now deleted independently and the failures are combined, and the cleanup runs as a try-with-resources resource so the JLS gives the wanted semantics for free: suppressed on the primary failure when there is one, propagated on its own when the test passed. Verified with a probe over the three cases -- a failing deletion still removes the remaining paths, a body failure stays primary with the cleanup attached as suppressed, and a cleanup failure propagates when the body passed -- plus ./gradlew build green with no leftover directories.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change rewrites YAML serialization with SnakeYAML, adds configurable XML DTD validation, fixes suite XML round-trip serialization, updates fixtures, and adds XML/YAML corpus characterization tests. ChangesSerialization and validation pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testng-core/src/main/java/org/testng/xml/XMLParser.java`:
- Around line 39-77: Refactor XMLParser.parse and parser so the static lock is
held only while obtaining or configuring the SAXParserFactory, not during
parser.parse(is, dh). Create a fresh SAXParser for each parse invocation after
leaving the lock, preserving the current validation-mode handling while avoiding
sharing a non-thread-safe SAXParser or serializing potentially blocking
entity-resolution I/O.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7e030a8-a210-4cab-ab79-3c38423ea275
📒 Files selected for processing (18)
CHANGES.txttestng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.javatestng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.javatestng-core-api/src/main/java/org/testng/xml/XmlInclude.javatestng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.javatestng-core/src/main/java/org/testng/internal/Yaml.javatestng-core/src/main/java/org/testng/xml/TestNGContentHandler.javatestng-core/src/main/java/org/testng/xml/XMLParser.javatestng-core/src/main/java/org/testng/xml/XmlValidationMode.javatestng-core/src/test/java/org/testng/xml/SuiteDigest.javatestng-core/src/test/java/org/testng/xml/XmlRoundTripTest.javatestng-core/src/test/java/org/testng/xml/XmlValidationTest.javatestng-core/src/test/java/test/yaml/YamlRoundTripTest.javatestng-core/src/test/java/test/yaml/YamlTest.javatestng-core/src/test/resources/testng-all.xmltestng-core/src/test/resources/testng.xmltestng-core/src/test/resources/xml/validation/wrong-element-order.xmltestng-core/src/test/resources/yaml/2078.yaml
XMLParser kept a single SAXParser behind a class wide lock that was held for the whole of parse(), so every suite parse in the JVM waited for every other one. That is not a contention concern only. Entity resolution happens inside parse(), and TestNGContentHandler resolves an unknown system id over HTTP with neither a connect nor a read timeout -- so one suite pointing at an unreachable DTD mirror blocked suite parsing everywhere until the socket gave up. The parser was shared because building one was assumed to be expensive. Measured on the build JDK, SAXParserFactory.newInstance() and newSAXParser() cost about 20 microseconds each, which is nothing against reading a suite file and less than nothing against fetching a DTD. Each parse now builds its own, and the lock and the mutable statics behind it are gone. Reading the validation mode per parse also drops the invalidation bookkeeping the previous commit had to add: there is no longer a cached configuration that can go stale when testng.xml.validation changes. The regression test does not fail an assertion under the old code, it hangs, so it carries a time-out. The two XmlValidationTest methods naming "the shared parser" are renamed, since there no longer is one.
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.
…ings 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 "- ", "<test>" 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<String, String>. 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 <groups> 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 testng-team#3318
XmlSuite has no metaGroups property, unlike XmlTest, so a suite level <define> 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 <dependencies> block has the same shape and was already only written for a test. Both gaps are now listed in the javadoc of toYaml.
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 <define> 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. 112 fixtures, all green.
juherr
force-pushed
the
juherr/issue-3318
branch
from
July 30, 2026 17:23
dcf5442 to
6f1b551
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3318. Step 2 of #3317.
Depends on #3315 — it brings
SuiteDigest, which the round trip test compares suitesthrough. This branch is stacked on it; rebase on
masteronce #3315 has landed.The problem
Yaml.toYaml()built YAML by string concatenation and produced output that no YAML parseraccepts. Its own golden file,
yaml/2078.yaml, did not parse. The only test that looked at thewriter compared its output to that file, so it pinned the broken output rather than detecting it.
The only production consumer is the
ConverterCLI (xml <-> yaml), whose YAML output wastherefore unusable; the javadoc also advertises the method as public API for external tools.
It was lossy in ways a string comparison cannot see, either:
What changed
The document is now built as plain maps and lists and handed to snakeyaml, which owns quoting,
escaping and indentation. A bean
RepresenteroverXmlSuitewas rejected:XmlPackage .getXmlClasses()scans the classpath, and roughly fifteen bean-readable properties have nosetter, so the reader would reject them. Building the document by hand also keeps the emitted
vocabulary an explicit, reviewable list — which is what #3321 is about.
Beyond the syntax, the writer used to disagree with the reader in five places:
includes/excludes; the reader bindsinclude/exclude;suite-fileswas written under a key the reader does not know, and only when the suite hadchild suites although it is filled from
getSuiteFiles();preserve-order,parent-module,guice-stage,allow-return-values,share-thread-pool-for-data-providers, the method selectors at both levels, class parametersand include descriptions were never written at all;
configFailurePolicycompared aStringagainst aFailurePolicyand was always written;<define>came out undermetaGroups, which onlyXmlTesthas — that one madethe whole file unloadable.
Values a test inherits from its suite are compared against the suite and dropped when they match,
and the
<groups>block is read from the model rather than throughgetIncludedGroups(), whichreturns the union with the suite's groups. Otherwise the writer materialises the suite into every
test.
The verbosity is compared against the level actually in effect rather than against
XmlSuite.DEFAULT_VERBOSE:getVerbose()falls back to-Dtestng.default.verbose, so comparingagainst 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: 0ofyaml/2078.yamlcame from. Outputis now byte-identical with and without the property.
Tests
test.yaml.YamlRoundTripTest, registered intestng.xml. The first commit is deliberatelyred: 37 of its 64 cases fail, and those failures are the defect list of the issue made
executable. The fix is the second commit.
Four invariants over the 16 YAML fixtures, none sufficient alone:
packages:emitted three timesSuiteDigestequalityXmlSuite.equalsignores&idanchorPlus one over the 112 XML fixtures: they must convert to YAML the reader accepts. The YAML corpus
can only contain what YAML already expresses, so it cannot cover a key that nothing reads back —
that is how the suite-level
<define>slipped through until it was found by reading the code.Only loadability is asserted there, not the digest: XML says more than the YAML schema does.
yaml/2078.yamlis regenerated. Being a.yamlfile under the test resources it is now part ofthe round trip corpus, so the golden checks itself. Its test keeps the golden comparison and adds
the assertion the golden cannot make: the dependency
a bsurvives with both spaces.The GITHUB-1787 test counted occurrences of
parameters:and then re-parsed the original XML filerather than the YAML it had just written, which made its second assertion vacuous. It now
re-parses the emitted YAML.
Not in scope
testng-core-apiis untouched. Seven things a suite file can carry still have no YAML key, becausethe reader has no way to bind them: a test
time-out(Stringgetter, privateStringsetter),an include's invocation numbers (no setter), the suite-level
group-by-instances(
Boolean/booleanmismatch), the object factory (snakeyaml cannot build aClass),use-global-thread-pool(not a bean accessor), and suite-level<define>/<dependencies>.They are listed in the javadoc of
toYamland tracked in #3326.Summary by CodeRabbit
New Features
testng.xml.validation.Bug Fixes
Tests