fix(xml): re-enable DTD validation and stop losing data in toXml() - #3315
Open
juherr wants to merge 6 commits into
Open
fix(xml): re-enable DTD validation and stop losing data in toXml()#3315juherr wants to merge 6 commits into
juherr wants to merge 6 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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSuite XML serialization preserves include descriptions and selector priorities, emits the resolved DTD, avoids duplicate ChangesSuite XML behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 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: 3
🤖 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 20-25: The static initialization of the SAXParserFactory freezes
validation based on the initial XmlValidationMode. Move validation-mode
selection or parser caching into parse() under the existing lock so each parse
uses the current mode, including switching between OFF and STRICT; otherwise
explicitly enforce startup-only configuration and prevent runtime mode changes.
In `@testng-core/src/main/java/org/testng/xml/XmlValidationMode.java`:
- Around line 44-46: Update the normalization in XmlValidationMode to call
toUpperCase with Locale.ROOT, ensuring requested mode matching is
locale-independent while preserving the existing trim and enum-name comparison
behavior.
In `@testng-core/src/test/java/org/testng/xml/SuiteDigest.java`:
- Around line 43-47: Update SuiteDigest’s suite-field assembly to include
suite.getGroups() alongside the existing included and excluded group entries,
using the canonical digest append/sorting approach for XmlGroups so definitions
and run state are represented.
🪄 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: 31b3d42b-1e4b-497d-9593-3ee67de2254b
📒 Files selected for processing (14)
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/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/resources/testng-all.xmltestng-core/src/test/resources/testng.xmltestng-core/src/test/resources/xml/validation/wrong-element-order.xml
…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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/TestNGContentHandler.java`:
- Line 120: Update TestNGContentHandler’s DTD tracking to implement/register SAX
LexicalHandler callbacks and set m_doctypeDeclared in startDTD, covering
internal as well as external DOCTYPE declarations. Preserve the existing
external-entity handling while ensuring strict validation recognizes any
declared DTD.
In `@testng-core/src/main/java/org/testng/xml/XmlValidationMode.java`:
- Around line 19-24: Update the lifecycle documentation in XmlValidationMode to
state that TestNGContentHandler captures XmlValidationMode.current() once when
each handler or parse is initialized, rather than on every error. Remove the
implication that changing WARN or STRICT during an active parse affects
subsequent violations, while preserving the distinction that XMLParser
determines whether validation occurs.
In `@testng-core/src/test/java/org/testng/xml/XmlValidationTest.java`:
- Around line 70-78: TheSharedParserValidatesSuiteFilesByDefault should only
assert XMLParser.isValidating() when RuntimeBehavior.XML_VALIDATION_MODE is not
configured as off; skip the default-validation assertion under the supported OFF
configuration while preserving the existing assertion and diagnostic message
otherwise.
🪄 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: 685445c1-f610-4a0a-b4c3-dd8f4881ae5a
📒 Files selected for processing (7)
CHANGES.txttestng-core-api/src/main/java/org/testng/xml/DefaultXmlWeaver.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/XmlRoundTripTest.javatestng-core/src/test/java/org/testng/xml/XmlValidationTest.java
| private final String m_fileName; | ||
| private final boolean m_loadClasses; | ||
| private boolean m_validate = false; | ||
| private boolean m_doctypeDeclared = false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'resolveEntity|m_doctypeDeclared|LexicalHandler|startDTD' \
testng-core/src/main/java/org/testng/xml \
testng-core/src/test/java/org/testng/xmlRepository: testng-team/testng
Length of output: 5583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant state-machine sections of TestNGContentHandler.
sed -n '110,180p' testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
printf '\n--- lexical handlers ---\n'
rg -n -C 5 'implements .*Handler|startDocument|startDTD|LexicalHandler|setLexicalHandler|setEntityResolver|setErrorHandler|setFeature|setValidating|error\(' \
testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java \
testng-core/src/main/java/org/testng/xml/Parser.java \
testng-core/src/test/java/org/testng/xml/XmlRoundTripTest.java \
testng-core/src/test/java/org/testng/xml
printf '\n--- constructor and features ---\n'
sed -n '1,120p' testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
sed -n '1,260p' testng-core/src/main/java/org/testng/xml/Parser.java | sed -n '1,220p'Repository: testng-team/testng
Length of output: 11650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Standalone SAX behavioral probe: internal-only DOCTYPE does not call EntityResolver.resolveEntity,
# while explicit external DTDs do. No repository code is executed; only the JDK SAX API is exercised.
java -cp /tmp:
<<'JS'
import javax.xml.parsers.SAXParserFactory;
import java.io.*;
import org.xml.sax.*;
import org.xml.sax.helpers.DefaultHandler;
public class Probe extends DefaultHandler implements EntityResolver {
int seenResolveEntity = 0;
int seenStartDTD = 0;
String errorMessage;
public Probe(int seenResolveEntity, int seenStartDTD, String errorMessage) {
this.seenResolveEntity = seenResolveEntity;
this.seenStartDTD = seenStartDTD;
this.errorMessage = errorMessage;
}
public InputSource resolveEntity(String publicId, String systemId) { unseenResolveEntity++; return super.resolveEntity(publicId, systemId); }
public void startDTD(String name, String publicId, String systemId) { seenStartDTD++; }
public void error(SAXParseException e) { errorMessage += e.getMessage() + " "; }
}
JS
echo "JVM not available via shell direct Java invocation from this interface; no probe ran."Repository: testng-team/testng
Length of output: 5081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- TestNGContentHandler imports and class declaration ---"
sed -n '1,70p' testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
printf '\n--- error handler and warning handling ---\n'
sed -n '760,850p' testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
printf '\n--- XML parsing/validation flow references ---\n'
rg -n -C 4 'new SAXParserFactory|setValidating|TestNGContentHandler|startDTD|LexicalHandler|setEntityResolver|createSAXParser|parse\(' \
testng-core/src/main/java testng-core/src/test/java/org/testng/xml
printf '\n--- current working directory files (sanity) ---\n'
pwd
git ls-files | rg '(^|/)TestNGContentHandler\.java$|(^|/)XmlValidationTest\.java$|(^|/)Parser\.java$' | head -50Repository: testng-team/testng
Length of output: 37641
🌐 Web query:
Java SAX EntityResolver resolveEntity internal DTD subset startDTD LexicalHandler invoked internal DTD
💡 Result:
In Java SAX parsing, the interaction between the EntityResolver, LexicalHandler, and DTD subsets works as follows: 1. EntityResolver resolveEntity: The EntityResolver is primarily used for intercepting and resolving external entities [1]. The SAX parser calls resolveEntity before opening any external entity, which includes the external DTD subset (if present) and external parameter entities [1][2]. However, the EntityResolver is not invoked for the internal DTD subset, as it is part of the main document and is already available to the parser [1][2]. 2. LexicalHandler startDTD: The LexicalHandler.startDTD method is used to report the beginning of DTD declarations [3][4]. It is invoked by the parser regardless of whether the DTD contains only an internal subset or both internal and external subsets [3][4]. - When startDTD is called, the name parameter provides the document type name [3][5]. - If an external DTD subset exists, the publicId and systemId parameters provide its identifiers [3][4]. - If only an internal DTD subset exists (i.e., no external DTD subset is declared), the publicId and systemId parameters will be null [3][6]. 3. Internal DTD Subset Scope: Declarations are generally assumed to belong to the internal DTD subset unless they appear between startEntity and endEntity events (which indicate the inclusion of an external entity) [3][5]. Comments and processing instructions found within the DTD are reported by the LexicalHandler between the startDTD and endDTD events [3][7]. In summary, resolveEntity is specifically for external entities and does not handle the internal DTD subset, whereas startDTD is a lifecycle event that reports the DTD's presence and its external subset identifiers, if applicable, even when an internal subset is present [1][3].
Citations:
- 1: https://docs.oracle.com/en/java/javase/21/docs/api/java.xml/org/xml/sax/EntityResolver.html
- 2: https://docs.oracle.com/en/java/javase/20/docs/api/java.xml/org/xml/sax/EntityResolver.html
- 3: https://docs.oracle.com/en/java/javase/26/docs/api/java.xml/org/xml/sax/ext/LexicalHandler.html
- 4: https://docs.oracle.com/en/java/javase/21/docs/api/java.xml/org/xml/sax/ext/LexicalHandler.html
- 5: https://docs.oracle.com/en/java/javase/25/docs/api/java.xml/org/xml/sax/ext/LexicalHandler.html
- 6: https://docs.oracle.com/en/java/javase/24/docs/api/java.xml/org/xml/sax/ext/LexicalHandler.html
- 7: https://docs.oracle.com/javase/tutorial/jaxp/sax/events.html
Track internal DTD declarations with lexical DTD handling.
m_doctypeDeclared is only set from EntityResolver.resolveEntity(), but SAX only resolves external entities; an internal <!DOCTYPE suite [ ... ]> can still establish the DTD grammar. As a result, XML validation violations in internal-only DTD suites are ignored in strict mode. Implement/set LexicalHandler (or another DTD lifecycle callback) and mark the document declared from startDTD.
🤖 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` at line
120, Update TestNGContentHandler’s DTD tracking to implement/register SAX
LexicalHandler callbacks and set m_doctypeDeclared in startDTD, covering
internal as well as external DOCTYPE declarations. Preserve the existing
external-entity handling while ensuring strict validation recognizes any
declared DTD.
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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testng-core/src/test/java/org/testng/xml/XmlValidationTest.java (1)
130-150: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the temporary DTD fixture.
Files.createTempDirectory()persists after the test, leaving the copied DTD and suite file behind on every run. Delete the files and directory in afinallyblock.🤖 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/XmlValidationTest.java` around lines 130 - 150, Update the test around the temporary directory fixture in XmlValidationTest to clean up the copied DTD, suite file, and temporary directory in a finally block, while preserving the existing parseValidating assertion behavior.
🤖 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.
Outside diff comments:
In `@testng-core/src/test/java/org/testng/xml/XmlValidationTest.java`:
- Around line 130-150: Update the test around the temporary directory fixture in
XmlValidationTest to clean up the copied DTD, suite file, and temporary
directory in a finally block, while preserving the existing parseValidating
assertion behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 965a8217-764b-485e-9e22-a05374973ea7
📒 Files selected for processing (4)
testng-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/XmlValidationTest.java
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.
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/test/java/org/testng/xml/XmlValidationTest.java`:
- Around line 152-157: Update the finally cleanup in XmlValidationTest to
attempt deletion of suite, dtd, and directory independently, collecting any
cleanup exceptions instead of allowing one to stop subsequent deletions.
Preserve the original assertion or parse failure when present by attaching
cleanup failures as suppressed exceptions; propagate cleanup failure only when
no primary failure exists.
🪄 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: a6b2679c-bf97-47a5-94e0-34ecb001731f
📒 Files selected for processing (1)
testng-core/src/test/java/org/testng/xml/XmlValidationTest.java
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.
Member
Author
|
@krmahadevan ready for your review |
This was referenced Jul 30, 2026
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.
Member
Author
|
Added The lock-held-across- The review asked to narrow the lock to factory acquisition and configuration. Two things pushed it further than that:
So each parse builds its own factory and parser, and the lock and its mutable statics are gone. This also drops the mode-invalidation bookkeeping added earlier in this PR: the validation mode is simply read at each parse, so it can no longer go stale. Worth flagging separately, because the fix does not address it: |
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.
The bug nobody noticed
DTD validation of suite files has been silently dead.
XMLParserprobes the SAX validation feature like this:The feature name is a plain identifier, not a URL to dereference, so it keeps its historical
httpscheme. Probing it underhttpsmakes every conforming parser raiseSAXNotRecognizedException:So
supportsValidation()always returnedfalse, neithersetValidating(true)norsetNamespaceAware(true)was ever reached, andTestNGContentHandler.error()(if (m_validate) throw e) could never fire. Only well-formedness errors surfaced; every DTD violation passed unnoticed.This came from commit
8bafc195a("All the other http:// -> https://"), a blanket scheme rewrite that caught a URI identifier rather than a URL.Turning it back on, carefully
Re-enabling validation can reject suite files that have worked for years — the DTD constrains the order of the children of
<suite>, for instance. So this adds a mode:warnvalidates and logs violations without failing the run, which makes the problem visible without breaking anyone.strictfails on the first violation.Measured blast radius on this repository's own corpus, against
testng-1.1.dtd: 111 valid, 1 invalid out of 112 suite files (plus 11 more undersrc/test/java, all valid). The single violation istestng-all.xml, which puts<parameter>before<groups>; it is reordered here.A safety net, and the two bugs it found
Refactoring anything around
XmlSuiteis risky: it is hard public API (IAlterSuiteListenerhands users a mutableList<XmlSuite>, and the OSGi manifest exportsorg.testng.xmlandorg.testng.xml.internal) and there is no binary-compatibility tooling in CI. So this adds round trip characterization tests over every suite file of the corpus — 112 files × 2 invariants:parse -> toXml -> parse -> toXmlis stable), which pins attribute selection and layout;XmlSuite.equals()— which ignores 11 of its 26 fields, among themm_parameters,m_xmlGroups,m_xmlMethodSelectors,preserveOrder,guiceStageandparentModule. A round trip that dropped any of those would still have compared equal.Those tests failed on first run and exposed two genuine data losses in the writer:
1.
<include description="...">was never written. The attribute is declared in the DTD and read by the handler, butXmlInclude.toXml()never emitted it, so regenerating a suite —testng-failed.xml, for instance — dropped method descriptions.2.
<selector-class>lost a priority of-1. The writer omittedprioritywhen it equalled-1, while the parser reads a missing priority as0. A negative method-selector priority is meaningful —RunInfo#includeMethodshort-circuits on it — so serializing a suite and reading it back silently changed its runtime behaviour. Reader and writer now shareXmlMethodSelector.DEFAULT_PRIORITY.Also
The doctype emitted by
toXml()advertisedtestng-1.0.dtdwhile the parser always resolvestestng-1.1.dtdfrom the classpath, so the declared schema was never the one used to read the file back. Now aligned.Verification
./gradlew build: BUILD SUCCESSFUL, 14010 tests, 0 failed.strictmode — a test asserting only that valid files parse would have passed throughout the years the feature was dead. Control run: reverting the one-characterhttps->httpchange makes that test fail.Not in this PR
The same round trip test applied to YAML shows
Yaml.toYaml()is broken well beyond a few patches (its own golden file,yaml/2078.yaml, is not loadable YAML). That deserves its own PR and is not mixed in here.Did you remember to?
CHANGES.txt./gradlew autostyleApplySummary by CodeRabbit
testng.xml.validation(off,warndefault,strict).<include>descriptionduring XML serialization.prioritydefaulting/omission and improved selector priority parsing when absent.<groups>emission; improved validation behavior and messaging based on whether a doctype is declared.testng-1.1.dtdand aligned validation with the bundled DTD.