refactor: declare org.testng.xml null-marked

[juherr]

Continues the JSpecify/NullAway stack. Base is master — #3382 merged while this was being
prepared, so this one stands alone. Rebasing dropped its commit as a duplicate rather than
reapplying it, and the merged version is byte-identical to the commit this was written on.

One package: org.testng.xml — the suite model, its parser and its weaver. 27 main files, 5112
lines, and the first batch where three things are true at once:

• it spans three modules — 17 files in testng-core-api, 9 in testng-core, 1 (SuiteDigest)
  in testng-test-kit, the last reached only through a compileOnly edge;
• 17 of those files are published API: XmlSuite, XmlTest and XmlClass are what users build
  suites with programmatically, so a @Nullable here is a published contract;
• the ripple lands in ten packages that are already marked and merged, so errors show up in
  shipped code — org.testng.internal.invokers, xml.internal, reporters.jq, cli,
  cli.jcommander and five others.

org.testng.xml.internal was marked in #3374. @NullMarked neither descends into sub-packages nor
climbs out of them, so that green said nothing about the parent.

The coverage was proved in all three modules

testng-core declares api(projects.testngCoreApi) and testng-test-kit declares
compileOnly(projects.testngCore), so the package-info.java goes in testng-core-api. The
compileOnly edge is the new variable: it puts testng-core and its api dependencies on the
compile classpath, but no package-info.class had ever been asked to travel it.

testng-test-kit is also easy to write off as out of scope — it is named "test-kit" and it is never
published — but the convention only disables NullAway for tasks whose name contains Test, and
its task is compileJava. It is under the check.

Per the procedure in AGENTS.md, the negative control runs once per module traversed. A throwaway

private static Object nullAwayProbe() { return null; }

went into one file of each. Before the package-info.java all three compiled clean, zero NullAway.
After it all three failed with [NullAway] returning @Nullable expression from method with @NonNull return type:

module	reached via	probe fails at
testng-core-api	holds the package-info.java	XmlUtils.java:9
testng-core	api(projects.testngCoreApi)	TestNGURLs.java:10
testng-test-kit	compileOnly(projects.testngCore)	SuiteDigest.java:23

All three reverted. The third row is the one that had to be measured rather than assumed, and it is
red — so a second package-info.java in testng-test-kit is not needed, and the counts below cover
the whole package. SuiteDigest then produced 16 real errors of its own, which is the same fact
arriving a second way.

What the check reported

	count
errors	112 (44 testng-core-api, 51 testng-core, 16 testng-test-kit, 1 testng-jcommander)
@Nullable	118
of which (E) demanded by NullAway	110
of which demanded by the Kotlin compiler	7
of which (C) contract	1
Objects.requireNonNull	31
restructured instead of annotated	10

Every annotation is classified by deletion and recompilation — each one stripped on its own and
the five modules force-recompiled, 111 times, then 79 of them a second time after the cleanup pass
below. 110 bring back a named NullAway error; the 7 Kotlin ones bring back a Kotlin error (see the
next section); 1 brings back nothing and is argued below. The files were copied to a separate
directory first, restored from that copy, and checked byte-identical per file afterwards.

Every number above comes from a forced recompile. The fast form —
:testng-core-api:compileJava --rerun and the four other modules by name — was validated once by
deleting an annotation known to be demanded and confirming the error came back.

Restructured rather than annotated

The lazy <groups>/<run> initialisation was written four times in XmlTest and three times in
XmlSuite, each copy followed by m_xmlGroups.getRun().getX(). A private groupsRun() that returns
the XmlRun it just guaranteed replaces all of them and removes seven dereferences with no
annotation and no assertion.

The parser is the other half. TestNGContentHandler keeps eighteen m_currentXxx fields set at a
start tag and cleared at the matching end tag; seven accessors now read them, so the invariant is
stated once instead of at twenty-five call sites.

The rest: XmlClass drops a redundant = null on m_name (every constructor calls init, which
assigns it) and its three-argument init overload, which had one caller and hid the assignment from
NullAway's initializer analysis; loadClass() returns the class it resolved so getSupportClass()
needs no assertion; XmlPackage.getXmlClasses and XmlTest.getInvocationNumbers cache through a
local; and XmlTest.equals loses a two-armed condition whose second arm re-tested a value the line
above had already established, so only the first arm could ever fire.

A @Nullable getter without its setter is a Kotlin source break

Seven setters here are annotated for a reason that has nothing to do with NullAway, and it is worth
recording because it will recur in every later batch that touches a bean.

Kotlin synthesises a mutable property from a Java getter/setter pair only when the two agree on
nullability. Annotate getName() @Nullable and leave setName(String) alone, and
XmlPackage.name silently stops being a var and becomes a val — so XmlPackage().apply { name = p }
no longer compiles. That is a source-incompatible change to published API, and this repository has a
Kotlin consumer of exactly that shape: testng-test-kit/src/main/kotlin/test/SimpleBaseTest.kt.

The pairs, all seven of them proved by a throwaway Kotlin file that assigns each property — with the
annotations it compiles, without them it reports 7 errors, one per line:

class	property
XmlDefine	name
XmlPackage	name
XmlGroups	run
XmlMethodSelector	className, script
XmlScript	expression
XmlTest	script

Each is truthful independently of Kotlin — every backing field is @Nullable, and XmlTest.setScript
already had an explicit else if (script != null) branch, so null was always its contract.

The trap worth passing on: ./gradlew build does not catch this. Kotlin's incremental compilation
treats a type-use annotation change on the Java classpath as non-ABI, so :testng-test-kit:compileKotlin
runs, reports nothing, and stays green on code that cannot compile from clean. It took
--rerun-tasks to see it. The guard set for a batch that annotates a bean must include
:testng-test-kit:compileKotlin --rerun-tasks, not just the Java compiles.

The one annotation without a compiler demand

XmlSuite.FailurePolicy.getValidPolicy(@Nullable String policy) — the parameter. Its return is
demanded twice over; the parameter is not, and it would be easy to read that as decoration.

The null is real and routine: TestNGContentHandler:378 calls it with
attributes.getValue("configfailurepolicy"), which is null for every suite file that omits the
attribute — the normal case. NullAway cannot see it because org.xml.sax.Attributes is unannotated
and read optimistically. The body has tested policy == null on its first line since it was written.

Worth recording: CliConfigurer:170 looked like the demanding caller and is not. It guards
cli.configFailurePolicy != null first, so NullAway sees a non-null value there.

The ripple

Ten merged packages read this one. Four needed something:

• org.testng.xml.internal — Parser:154 passes no stream when the suite is not a file: URL, so
  IFileParser.parse's stream parameter is @Nullable. SuiteXmlParser then had to widen to match:
  NullAway does check that an implementation does not narrow a @Nullable interface parameter. It is
  reached either through accept(), which requires a file: scheme and so a stream, or as
  Parser.getParser's fallback for a scheme nothing claims — which has never been readable here, so
  the requireNonNull is the throw that was already happening rather than a new precondition.
• org.testng.reporters.jq — TestNgXmlPanel.getHeader returns XmlSuite.getFileName(), absent for
  a suite built in code. XMLStringBuffer.addOptional already declares its value @Nullable and
  skips null, so the abstract getHeader in BaseMultiSuitePanel follows; the other six overrides
  stay non-null.
• org.testng.cli.jcommander — Converter:70 derives an output name from getFileName(). Every
  suite it sees was read from a file and both readers set the name (TestNGContentHandler for XML,
  Yaml for YAML), which the requireNonNull records.
• org.testng.internal.invokers and the six others needed nothing.

One item for whoever takes the next batch: YamlParser (testng-yaml) and the test
FakeHttpXmlParser implement IFileParser with a non-@Nullable stream parameter. Both are in
unmarked packages so nothing checks them today, and both will report the narrowing the moment their
package is marked.

Two latent NullPointerExceptions, recorded and not fixed

An XmlGroups on an XmlTest can have no XmlRun — setGroups(new XmlGroups()) and addMetaGroup
both leave one that way — and two methods dereference getRun() without testing it:

site	reproduction
XmlTest.addIncludedGroup	t.addMetaGroup("m", List.of("a")); t.addIncludedGroup("g")
XmlTest.equals	t1.addExcludedGroup("x") against t2.setGroups(new XmlGroups())

Both were run against these sources and against the pre-change sources: same
NullPointerException either way, only the message differs. addExcludedGroup,
setIncludedGroups and setExcludedGroups all repair a missing <run>, and every XmlSuite
equivalent does too, so the asymmetry is XmlTest-only. Repairing it would turn a long-standing
throw into a silent success, which is a behaviour change and does not belong in this batch —
requireNonNull keeps the throw and makes the asymmetry compiler-visible. Filed as #3385.

Also noted, not reproducible from the suite: XmlWeaver.getInstance() returned null in test mode
when -Dtestng.xml.weaver names a third-party class, and the callers dereferenced it. TestNG's own
tests only ever select the two bundled weavers. And XmlSuite.m_test has no writer anywhere in the
repository, so getTest() has always returned null — annotated truthfully rather than removed, since
it is published API.

Residue left alone, as agreed for this stack: == null guards on values NullAway now proves
non-null, in XmlClass.equals/hashCode and XmlInclude.equals/hashCode.

Verification

./gradlew build --rerun-tasks: BUILD SUCCESSFUL, 16854 completed, 0 failed, 12 skipped — the
twelve are the pre-existing environment-dependent skips, unchanged from #3382. --rerun-tasks rather
than a plain build for the reason given in the Kotlin section: the plain form went green on a tree
that did not compile from clean. autostyleApply run as its own invocation.

No behaviour change.

Summary by CodeRabbit

• Bug Fixes

  • Improved XML parsing validation with clearer errors for missing input streams, suite declarations, and required suite filenames.
  • Added safer handling for optional XML configuration values and nullable headers.
  • Improved lazy initialization and access to suite, test, and group configuration.

• Documentation

  • Added comprehensive nullability information to XML configuration APIs.
  • Documented parser behavior for XML sources and optional configuration elements.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ab40a914-5afd-4a0e-aec3-80a2494432a5

📥 Commits

Reviewing files that changed from the base of the PR and between d815bd2 and e11f78e.

📒 Files selected for processing (10)

• testng-core-api/src/main/java/org/testng/xml/XmlClass.java
• testng-core-api/src/main/java/org/testng/xml/XmlDefine.java
• testng-core-api/src/main/java/org/testng/xml/XmlGroups.java
• testng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.java
• testng-core-api/src/main/java/org/testng/xml/XmlPackage.java
• testng-core-api/src/main/java/org/testng/xml/XmlScript.java
• testng-core-api/src/main/java/org/testng/xml/XmlSuite.java
• testng-core-api/src/main/java/org/testng/xml/XmlTest.java
• testng-core/src/main/java/org/testng/xml/SuiteXmlParser.java
• testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java

🚧 Files skipped from review as they are similar to previous changes (8)

• testng-core/src/main/java/org/testng/xml/SuiteXmlParser.java
• testng-core-api/src/main/java/org/testng/xml/XmlDefine.java
• testng-core-api/src/main/java/org/testng/xml/XmlPackage.java
• testng-core-api/src/main/java/org/testng/xml/XmlMethodSelector.java
• testng-core-api/src/main/java/org/testng/xml/XmlScript.java
• testng-core-api/src/main/java/org/testng/xml/XmlClass.java
• testng-core-api/src/main/java/org/testng/xml/XmlTest.java
• testng-core-api/src/main/java/org/testng/xml/XmlSuite.java

Included review availability: Your plan includes up to 8 reviews per rolling hour; 6 remain after this review.

---

📝 Walkthrough

Walkthrough

The pull request applies JSpecify nullability annotations across XML APIs, adds @NullMarked package metadata, centralizes lazy group state, and adds explicit null validation to XML parsing and related consumers.

Changes

XML model contracts

Layer / File(s)	Summary
XML model nullability contracts testng-core-api/src/main/java/org/testng/xml/*	XML fields, constructors, parameters, and accessors now declare nullable values. XmlClass and lazy XML class storage use cached typed values.
Lazy XML group state testng-core-api/src/main/java/org/testng/xml/XmlSuite.java, testng-core-api/src/main/java/org/testng/xml/XmlTest.java	Suite and test group mutation uses lazy group/run helpers. Invocation caches and group comparisons validate nullable state explicitly.

Parser and consumer boundaries

Layer / File(s)	Summary
Null-safe XML parsing testng-core/src/main/java/org/testng/xml/IFileParser.java, testng-core/src/main/java/org/testng/xml/SuiteXmlParser.java, testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java, testng-core/src/main/java/org/testng/xml/XMLParser.java	Parser inputs and state are annotated as nullable. Required parser state, attributes, suite elements, and collections now use explicit null checks.
Nullable consumer boundaries testng-core-api/src/main/java/org/testng/xml/XmlWeaver.java, testng-core/src/main/java/org/testng/reporters/jq/*, testng-jcommander/src/main/java/org/testng/cli/jcommander/Converter.java, testng-test-kit/src/main/java/org/testng/xml/SuiteDigest.java	Consumers declare nullable values and reject missing weavers or suite filenames before downstream processing.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to e11f7

The PR adds nullness contracts and localized supporting refactors across the XML suite model, parser, and weaver, with the full build passing and no reported behavior change; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant SuiteXmlParser
  participant TestNGContentHandler
  participant XmlSuite
  SuiteXmlParser->>TestNGContentHandler: parse XML input
  TestNGContentHandler->>TestNGContentHandler: validate parser state and attributes
  TestNGContentHandler->>XmlSuite: construct and populate suite
  TestNGContentHandler-->>SuiteXmlParser: return nullable suite
  SuiteXmlParser->>SuiteXmlParser: require a suite result

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 29.05% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: declaring the org.testng.xml package as null-marked.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch juherr/nullmarked-xml

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}