feat(xml): ship an XSD for testng.xml alongside the DTD - #3325
Open
juherr wants to merge 7 commits into
Open
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.
Three test classes now need to walk the same corpus, and the details are easy to get subtly wrong: opening a suite file without a system id is what lets xml/issue2501/2501.xml resolve its external entity, and resolving the doctype from the classpath is what keeps the tests off the network. The shared entity resolver also narrows the one XmlRoundTripTest had: it substituted the bundled DTD for every entity, which is harmless for serialized output but would feed the DTD to a ¶ms; reference. No behaviour change.
testng.xml has only ever had a DTD. testng-team#2594 shipped an XSD in 2021 but wired it to nothing, and DTD validation was itself silently dead until it was re-enabled, so the choice of schema language changed nothing for users. Now it does. The schema is salvaged from testng-team#2594 and corrected: no targetNamespace (suite files have never declared an xmlns, so one would break every existing testng.xml), no junit attribute, plus use-global-thread-pool and share-thread-pool-for-data-providers. It mirrors testng-1.1.dtd declaration for declaration, including where testng-team#2594 diverged: - <run> and <package> are a choice, not a sequence: the DTD writes (include?,exclude?)* and testng-all.xml does interleave them; - ANY stays ANY, because <parameter> appears inside <suite-file> and inside <include>; - (true | false) maps to a named simpleType, not xsd:boolean, which would also accept 0 and 1 and make the schema looser than the DTD. The ordering constraint on the children of <suite> is kept rather than relaxed. XSD 1.0, the only level stock JAXP supports, cannot express "any order" and "at most one of each" at the same time, and relaxing the order would make the invalid-on-purpose fixture valid and lose the guard that caught the duplicate <groups> the writer used to emit. Two tests keep the two schemas from drifting, which they have done before: the whole corpus and every toXml() output must validate under both, and the declarations themselves -- elements, attributes, requiredness, defaults, enumerations -- are compared directly.
📝 WalkthroughWalkthroughTestNG adds configurable DTD validation, updates suite XML serialization, ships a matching ChangesSuite XML validation and serialization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RuntimeBehavior
participant XmlValidationMode
participant XMLParser
participant TestNGContentHandler
RuntimeBehavior->>XmlValidationMode: Read testng.xml.validation
XmlValidationMode->>XMLParser: Select OFF, WARN, or STRICT
XMLParser->>TestNGContentHandler: Parse suite with DTD validation
TestNGContentHandler->>XMLParser: Apply mode to validation errors
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (3)
testng-core/src/test/java/org/testng/xml/XsdValidationTest.java (1)
104-111: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueHarden
DocumentBuilderFactoryagainst XXE.Same static-analysis hint as
SchemaConsistencyTest; this instance parses corpus suite files andtoXml()output, still project-controlled/trusted content, but enabling secure processing is cheap.🤖 Prompt for 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. In `@testng-core/src/test/java/org/testng/xml/XsdValidationTest.java` around lines 104 - 111, Harden the DocumentBuilderFactory setup in XsdValidationTest by enabling secure XML processing before creating the DocumentBuilder. Update the factory configuration near setNamespaceAware and setSchema, preserving the existing schema validation and entity resolver behavior.Source: Linters/SAST tools
testng-core/src/test/java/org/testng/xml/SchemaConsistencyTest.java (1)
144-149: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueHarden
DocumentBuilderFactoryagainst XXE.Static analysis flags this as unhardened, though risk is low since only the bundled
testng-1.1.xsdresource is parsed here (no external/untrusted input).🤖 Prompt for 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. In `@testng-core/src/test/java/org/testng/xml/SchemaConsistencyTest.java` around lines 144 - 149, Harden the DocumentBuilderFactory setup in SchemaConsistencyTest by disabling external entity resolution and external DTD/schema access before parsing TESTNG_XSD. Configure these security features immediately after DocumentBuilderFactory.newInstance() and preserve the existing namespace-aware parsing flow.Source: Linters/SAST tools
testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java (1)
120-143: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win"Add a DOCTYPE" warning still keyed on
m_validate, not the newm_doctypeDeclared.
m_doctypeDeclaredcorrectly tracks "a DOCTYPE was declared, whoever provides the DTD" and fixes error-reporting for suites pointing at a local/mirrored DTD copy. However,startElement's "It is strongly recommended to add ... DOCTYPE" warning (Line 576) still checks!m_validate, which is onlytruewhen TestNG substitutes its own DTD (the classpath/https path). For the very scenario this PR added a test for — a suite whose DOCTYPE points at its own local DTD copy —m_validatestaysfalse, so this warning will fire even though the suite already declares a DOCTYPE. Consider gating it onm_doctypeDeclaredinstead, since that's precisely the condition it's checking for.♻️ Suggested fix (outside the selected range, at the `startElement` warning check)
if (!m_doctypeDeclared && !m_hasWarn) {🤖 Prompt for 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. In `@testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java` around lines 120 - 143, Update the DOCTYPE recommendation warning in startElement to gate on !m_doctypeDeclared instead of !m_validate, while retaining the existing m_hasWarn guard. This ensures suites using local or mirrored DTDs are not warned when a DOCTYPE is already declared.
🤖 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 57-77: Update parser() to configure the SAXParserFactory with
XMLConstants.FEATURE_SECURE_PROCESSING before enabling validation, and disable
external DTD access unless explicitly required. Apply these settings to spf
before spf.newSAXParser(), preserving the existing validation-mode behavior and
error handling.
---
Nitpick comments:
In `@testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java`:
- Around line 120-143: Update the DOCTYPE recommendation warning in startElement
to gate on !m_doctypeDeclared instead of !m_validate, while retaining the
existing m_hasWarn guard. This ensures suites using local or mirrored DTDs are
not warned when a DOCTYPE is already declared.
In `@testng-core/src/test/java/org/testng/xml/SchemaConsistencyTest.java`:
- Around line 144-149: Harden the DocumentBuilderFactory setup in
SchemaConsistencyTest by disabling external entity resolution and external
DTD/schema access before parsing TESTNG_XSD. Configure these security features
immediately after DocumentBuilderFactory.newInstance() and preserve the existing
namespace-aware parsing flow.
In `@testng-core/src/test/java/org/testng/xml/XsdValidationTest.java`:
- Around line 104-111: Harden the DocumentBuilderFactory setup in
XsdValidationTest by enabling secure XML processing before creating the
DocumentBuilder. Update the factory configuration near setNamespaceAware and
setSchema, preserving the existing schema validation and entity resolver
behavior.
🪄 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: bc87aa86-b0fd-4ce8-9d60-23cf74af530e
📒 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/xml/TestNGContentHandler.javatestng-core/src/main/java/org/testng/xml/XMLParser.javatestng-core/src/main/java/org/testng/xml/XmlValidationMode.javatestng-core/src/main/resources/testng-1.1.xsdtestng-core/src/test/java/org/testng/xml/SchemaConsistencyTest.javatestng-core/src/test/java/org/testng/xml/SuiteCorpus.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/org/testng/xml/XsdValidationTest.javatestng-core/src/test/resources/testng-all.xmltestng-core/src/test/resources/testng.xmltestng-core/src/test/resources/xml/validation/wrong-element-order.xml
Comment on lines
+57
to
+77
| private static SAXParser parser() { | ||
| XmlValidationMode mode = XmlValidationMode.current(); | ||
| if (m_saxParser != null && mode == configuredFor) { | ||
| return m_saxParser; | ||
| } | ||
| SAXParserFactory spf = loadSAXParserFactory(); | ||
|
|
||
| // Namespace awareness is deliberately left off: DTD validation does not need it, suite files | ||
| // are not namespaced, and turning it on would make an unbound prefix fatal and an xmlns | ||
| // attribute a validity error -- neither of which has anything to do with validating a suite. | ||
| validating = mode.isValidating() && supportsValidation(spf); | ||
| spf.setValidating(validating); | ||
| try { | ||
| m_saxParser = spf.newSAXParser(); | ||
| } catch (ParserConfigurationException | SAXException e) { | ||
| Logger.getLogger(XMLParser.class).error(e.getMessage(), e); | ||
| m_saxParser = null; | ||
| } | ||
| configuredFor = mode; | ||
| return m_saxParser; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
JAXP SAXParserFactory FEATURE_SECURE_PROCESSING billion laughs protection default
💡 Result:
In the Java Development Kit (JDK), the SAXParserFactory has the FEATURE_SECURE_PROCESSING (FSP) feature enabled by default [1][2][3]. When FSP is enabled—either by default or through explicit configuration—the JDK enforces various security limits to protect against common XML-based attacks, including the "billion laughs" (exponential entity expansion) attack [1][3][4]. Key aspects of this default behavior include: 1. Processing Limits: With FSP enabled, the XML parser adheres to specific security properties that limit resource consumption, such as entity expansion limits and maximum occurrences of elements [1][2][5]. These limits directly mitigate the risk of denial-of-service attacks like the billion laughs attack [3][4]. 2. Default Status: While FSP is true by default for SAX, DOM, and validation factories, the JDK differentiates between this "default enabled" state and "explicitly set" state in older versions [6]. However, modern JDK security guidelines emphasize that these parsers observe secure processing limits by default [3][4]. 3. External Connections: Enabling FSP by default provides a baseline of protection; however, it does not automatically disable external connections (such as external entity resolution) [1][2][5]. To fully restrict external access, you must explicitly configure properties such as accessExternalDTD and accessExternalSchema [1][2][4]. 4. Explicit Configuration: To ensure a consistent security posture, it is a recommended practice to explicitly set the feature in your application code [3]: SAXParserFactory spf = SAXParserFactory.newInstance; spf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Citations:
- 1: https://docs.oracle.com/en/java/javase/21/security/java-api-xml-processing-jaxp-security-guide.html
- 2: https://docs.oracle.com/en/java/javase/25/security/java-api-xml-processing-jaxp-security-guide.html
- 3: https://docs.oracle.com/en/java/javase/23/security/java-api-xml-processing-jaxp-security-guide.html
- 4: https://docs.oracle.com/en/java/javase/22/security/java-api-xml-processing-jaxp-security-guide.html
- 5: https://docs.oracle.com/javase/8/docs/technotes/guides/security/jaxp/jaxp.html
- 6: https://docs.oracle.com/javase/tutorial/jaxp/properties/scope.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files of interest =="
git ls-files | rg '(^|/)(XMLParser\.java|xml|pom\.xml|maven\.config|gradle\.properties|.*\.java$|.*\.md$)' | head -200
echo "== XMLParser outline =="
ast-grep outline testng-core/src/main/java/org/testng/xml/XMLParser.java 2>/dev/null || true
echo "== XMLParser relevant section =="
sed -n '1,160p' testng-core/src/main/java/org/testng/xml/XMLParser.java | cat -n
echo "== TestNGContentHandler outline/section candidates =="
for f in $(git ls-files | rg 'TestNGContentHandler\.java$'); do
echo "--- $f ---"
ast-grep outline "$f" 2>/dev/null || true
sed -n '1,240p' "$f" | cat -n
done
echo "== XMLConstants / FEATURE_SECURE_PROCESSING references =="
rg -n "FEATURE_SECURE_PROCESSING|XMLConstants|setFeature|setValidating|accessExternalDTD|accessExternalSchema|loadSAXParserFactory|supportsValidation|m_saxParser|configuredFor" testng-core/src/main/java/org/testng/xml testng-core/src/main/java 2>/dev/null || true
echo "== build java config references =="
for f in $(git ls-files | rg '(^|/)(pom\.xml|build\.gradle|build\.gradle\.kts|gradle\.properties|settings\.gradle|Makefile|.*\.md$)$'); do
if rg -q 'maven\.compiler|source>|target>|java\.version|compileOptions|sourceCompatibility|targetCompatibility|java|JDK|17|21|23|25' "$f" 2>/dev/null; then
echo "--- $f ---"
rg -n 'maven\.compiler|source>|target>|java\.version|compileOptions|sourceCompatibility|targetCompatibility|java|JDK|17|21|23|25' "$ f" 2>/dev/null || true
fi
done 2>/dev/nullRepository: testng-team/testng
Length of output: 12957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== targeted file list =="
git ls-files | grep -E '(^|/)XMLParser\.java$|(^|/)XmlUtils\.java$|^gradle\.properties$|(^|/)pom\.xml$' || true
echo "== XMLParser relevant section =="
if [ -f testng-core/src/main/java/org/testng/xml/XMLParser.java ]; then
sed -n '1,180p' testng-core/src/main/java/org/testng/xml/XMLParser.java | cat -n
fi
echo "== TestNGContentHandler references =="
git ls-files | grep -E 'TestNGContentHandler\.java$' | while read -r f; do
echo "--- $f ---"
sed -n '1,280p' "$f" | cat -n
done
echo "== relevant XML/JAXP code references =="
rg -n "FEATURE_SECURE_PROCESSING|XMLConstants|setFeature|setValidating|accessExternalDTD|accessExternalSchema|loadSAXParserFactory|supportsValidation|m_saxParser|configuredFor|resolveEntity|EntityResolver|SAXParserFactory|newSAXParser" testng-core/src/main/java testng-core-api/src/main/java testng-*.gradle* pom.xml gradle.properties -S || trueRepository: testng-team/testng
Length of output: 28819
Secure the SAX factory before enabling DTD validation.
spf.newSAXParser() is created without XMLConstants.FEATURE_SECURE_PROCESSING; the EntityResolver only controls external DTD/entity fetching and does not protect against internal-entity-exansion attacks such as “billion laughs”. With DTD validation re-enabled, configure the SAX parser with secure processing, and disable external DTD access unless it is intentionally required.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 69-69: SAXParser created without secure processing is vulnerable to XXE
Context: spf.newSAXParser()
Note: [CWE-611] Improper Restriction of XML External Entity Reference.
(xml-parsing-xxe-saxparser)
🤖 Prompt for 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.
In `@testng-core/src/main/java/org/testng/xml/XMLParser.java` around lines 57 -
77, Update parser() to configure the SAXParserFactory with
XMLConstants.FEATURE_SECURE_PROCESSING before enabling validation, and disable
external DTD access unless explicitly required. Apply these settings to spf
before spf.newSAXParser(), preserving the existing validation-mode behavior and
error handling.
Source: Linters/SAST tools
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 #3319. Step 3 of #3317.
Important
Stacked on #3315, which is not merged yet. GitHub cannot base a pull request on a branch of a fork, so the diff below also carries #3315's 14 files. The commits that belong to this pull request are the last two; everything else is #3315. Merge that one first, then this becomes a five-file change.
testng.xmlhas only ever had a DTD. #2594 shipped an XSD in 2021 but wired it to nothing, and DTD validation was itself silently dead until #3315 re-enabled it — so until now the choice of schema language changed nothing for anyone. Now it does.The schema
testng-core/src/main/resources/testng-1.1.xsd, salvaged from #2594 (typed attributes, namedsimpleTypes reused betweensuiteandtest, DTD prose as realxsd:documentation) and brought in line with the DTD the reader actually resolves:junitdropped,use-global-thread-poolandshare-thread-pool-for-data-providersadded, every default copied across;targetNamespace, and noelementFormDefaulteither — dead weight without a namespace. No suite file in the corpus declares anxmlns, and adding one would break every existingtestng.xml. That is reserved for a hypothetical 2.0;Three places where #2594's schema rejects files the DTD accepts, found by running it against the corpus rather than by reading it:
<run>,<package>sequence(include*, exclude*)(include?,exclude?)*, andtestng-all.xmlhas a<run>with<exclude>before<include>ANYelements<parameter>appears inside<suite-file>(parametertest/issue_581/parent_suite.xml) and inside<include>(yaml/1787.xml)(true | false)xsd:boolean0and1— quietly looser than the DTD it mirrors. It is a namedsimpleTypehereWhy the ordering constraint was kept
#3319 asked for
<groups>to join the choice so that validation stops depending on element order, and for the singleton children of<suite>not to be repeatable. Those two are mutually exclusive in XSD 1.0, the only level stock JAXP supports: order-independence over a set that also holds the repeatabletestandparameterneeds<xsd:choice maxOccurs="unbounded">, which cannot bound a branch to one occurrence, and<xsd:all>— the construct that could — forbidsmaxOccurs="unbounded"inside it.This ships the ordering constraint as the DTD has it. Relaxing it instead would have cost two things:
xml/validation/wrong-element-order.xml, the fixture #3315 added to prove validation is wired in at all, becomes valid; and the DTD'sgroups?is what caughttoXml()emitting two sibling<groups>elements, also fixed in #3315. The full reasoning is on #3319.The ordering trap is real and still open. It is a change to the DTD, and it deserves its own issue rather than riding along behind a new file.
Keeping the two from drifting
They have drifted before: the writer advertised
testng-1.0.dtdwhile the reader always resolvedtestng-1.1.dtd. Two tests, because neither alone is enough:XsdValidationTest— every suite file of the corpus, and everytoXml()output, must validate against the XSD, mirroring what fix(xml): re-enable DTD validation and stop losing data in toXml() #3315 asserts against the DTD. Plus a negative test that the DTD-invalid fixture is rejected by the XSD too: without it, a schema matching everything would pass the other two. That is exactly how DTD validation stayed dead for years — the tests around it only ever asserted that valid files parse.SchemaConsistencyTest— compares the declarations directly: element names, attribute names, requiredness, default values, enumerated values. Content models are deliberately left out and the class javadoc says why; the corpus covers them from the other end.SuiteCorpusextracts the corpus data provider and the parse helpers fromXmlRoundTripTest, since three classes now need them. Its shared entity resolver is also narrower than the one it replaces, which substituted the bundled DTD for every entity — harmless for serialized output, but it would feed the DTD to the¶ms;reference ofxml/issue2501/2501.xml.One trap worth recording
Validator.validate(SAXSource)overwrites the entity resolver of the reader it is handed. The first run therefore fetched the publishedtestng-1.0.dtdover the network, which still declaresjunit, and 224 corpus assertions failed against a schema that was correct. Validating with the schema attached to the parser (DocumentBuilderFactory.setSchema, astest.junitreports.JUnitReportsTestalready does) keeps the resolver, so no test touches the network.Verification
./gradlew build: BUILD SUCCESSFUL, 16062 tests, 0 failed.The XSD is packaged by the existing resource convention, with no build change:
Four control experiments, each reverted, because a schema test that cannot fail is worth nothing:
include/@descriptionfrom the XSDSchemaConsistencyTest.bothSchemasDeclareTheSameAttributes, showing the missing attribute<run>back into asequenceeverySuiteFileOfTheCorpusValidatesAgainstTheXsd(testng-all.xml)— and nothing else<suite>accept anythingtheXsdRejectsASuiteThatViolatesTheDtdNullPointerException: testng-1.1.xsd is not on the test classpath— loud, not skippedNot in this pull request
https://testng.org/testng-1.1.xsdis a website-repo action.xmlnsand noxsi:noNamespaceSchemaLocation, so nothing keys a schema to atestng.xml, and emitting one is not an option becausexmlns:xsiis undeclared in the DTD.XmlDependencies.toXml()writes<include name= depends-on=>inside<dependencies>where the DTD and the reader expect<group>, andTestNGContentHandler.xmlGroupdereferencesm_currentTest, so a suite-level<dependencies>would NPE. Neither is reachable from the corpus; noted for Move XML serialization out of the XmlSuite domain model #3320.Did you remember to?
CHANGES.txt./gradlew autostyleApplySummary by CodeRabbit
off,warn(default), andstrictmodes.