fix(reports): keep a passed configuration's snapshot for the reports that list it

[juherr]

Phase 7b of #3406.

#3416 made XMLSuiteResultWriter read the invocation-time snapshots. That turned a true premise
into a false one.

ParameterSnapshotRecorder.onConfigurationSuccess discards the snapshot of a configuration method
that passed, and said why:

Only failed and skipped configurations are listed, so nothing will read this one again -- and the
reporters that print a configuration as it passes already have.

XMLSuiteResultWriter writes testContext.getPassedConfigurations(), and it runs during
IReporter#generateReport, long after the discard. So it found no snapshot and fell back to
ITestResult#getParameters() — the late read the migration exists to remove.

On the existing PassingConfigurationParameterSample, a @BeforeMethod handed the row its test
method will run with, which mutates it and passes. One default run, before this change:

[TestNG] PASSED CONFIGURATION: ... prepare([Ljava.lang.Object;)(value(s): [before-configuration])
testng-results.xml:  <![CDATA[[mutated]]]>

Contradictory answers from the same run, and the XML one is wrong.

The fix

The store now records whether anything will read it after the invocation lifecycle is over. That
distinction already existed in the wiring and was simply not kept:

• ParameterSnapshotReader.requestCaptureIfAnyReads asks on behalf of reporters that only run at
  generateReport;
• ParameterSnapshots.requestCaptureFor is called by TextReporter and VerboseReporter, which
  sit in the invocation lifecycle.

Discarding is correct for the second and wrong for the first, so discard consults the store rather
than the caller. The recorder holds only half the information — it knows the live reporters have
been told; only the store knows whether one that has not run yet still wants it. Having the recorder
scan the run's reporters instead would put a run-wide walk on the single most frequent event in a
suite.

Two independent monotone flags rather than one ordered value. Both are writes of true and neither
clears the other, so a run with both kinds of reader gets the same answer whichever asks first.
Folding them into one field updated to a maximum would turn two race-free writes into a
read-modify-write, and <suite parallel="tests"> makes those calls from two runners at once.

What a large suite now retains, and whether it is bounded

Bounded by what TestNG already holds, and dominated by it.

A configuration method that declares no parameter — the common @BeforeMethod() — retains
nothing at all: Parameters.createParametersForMethod returns new Object[0], ParameterSnapshot.of
answers null for an empty array, and captureIfAbsent stores only non-null. That is the frequent
case, and it costs zero bytes.

For one that does declare a parameter, the retained graph is the rendered text plus roughly 200
bytes of map entry and wrappers — about 250 B for @BeforeMethod(Method m), about 400 B for
@BeforeMethod(ITestResult r). Injected values do land in result.getParameters() for a
configuration method, so those shapes are included. A 10,000-test suite with a parameterised
@BeforeMethod and @AfterMethod retains on the order of 5 MB, against the 4–8 MB of TestResult
objects the same run already held for the same results.

It is bounded because those ITestResults are already retained for the whole run in TestRunner's
passed-configurations ResultMap — that is where XMLSuiteResultWriter reads them — and nothing
evicts it before reporting: MethodHelper.clear clears a name cache, and memory-friendly mode drops
method maps, not result maps. The map is identity-keyed, so it holds one entry per invocation, the
same granularity as the snapshot store. ParameterSnapshots.detachFrom empties the store in a
finally immediately after generateReports.

The one value whose size is user-unbounded is @BeforeMethod(Object[] parameters), which retains
Arrays.deepToString(row). The row objects themselves were already pinned by the same map; what is
new is their rendering. And the fallback allocated the identical ParameterSnapshot transiently
anyway — the change converts allocation into retention rather than introducing it.

The default path also gets cheaper: it no longer allocates a ResultKey and runs a
ConcurrentHashMap.remove per passing configuration, and it renders the value once instead of
twice.

Shapes considered and rejected

• Have the recorder ask and skip the call. Widens the internal surface and moves the invariant
  away from the state it depends on; any future caller of discard re-acquires the bug.
• Drop discard entirely. It is not dead. Surefire and TestNG's own test kit set
  useDefaultListeners=false; such a run has no XMLReporter, nothing reads late, and retaining
  every passing @BeforeMethod snapshot would be pure waste.
• Narrow what is retained. Already as narrow as it goes: discard has exactly one caller, so
  the flag is the configuration-only narrowing. "Only if mutable" is undecidable, and
  ParameterValue already shares one String instance for both forms unless the declared type is
  String.

Comments this corrects

Both claimed more than the code does, and the first is what made this wrong in the first place.

ParameterSnapshotRecorder.onConfigurationSuccess asserted a property of what is listed that was
never the recorder's to know. It now states what it does know and defers the decision to the store.

TestRunner.addInternalConfigurationListener repeats the claim that being registered first means
being told last. That holds only while nothing reorders the listeners: ListenerComparator sorts
the list before reversedOrder reverses it, preferential listeners are merged in after the regular
ones, and TestListenerHelper appends the invoker's own listener — the one that files the result
into m_passedConfigurations — after the reversal. So at the moment discard fires, the result
has not yet been recorded as something the report will list. That is the bug in one sentence, and
for the reports that read late the fix no longer depends on the order at all.

Coverage

• XmlReporterParametersTest — a passing configuration is reported with the values it was announced
  with; and the value is rendered exactly twice, once per invocation handed it (the configuration's
  row and the test's own parameter), where capture-then-discard-then-fallback made it three.
• PassedConfigurationSnapshotTest — a reader that reads once the invocations are over still finds
  the passing configuration's snapshot; a run whose only readers are live ones still drops it; and a
  store that reached detachFrom still holding one is released all the same.
• ParameterSnapshotsTest — retention at the store level, and that the two requests do not cancel
  each other in either order.
• VerboseReporterTest and TextReporterTest are unchanged and still pass: the console output,
  including the existing PassingConfigurationParameterSample expectation, is untouched.

CountedConfigurationParameterSample carries its own counter rather than reusing
RenderingCountSample's: adding a configuration method there would move the exact counts five
assertions across three test classes rely on.

What is left

Unchanged in this PR, as Phase 7 continues: TestHTMLReporter, the jq panels and
EmailableReporter2 still render for themselves. jq's Model also lists passed configurations,
so when Main is migrated it must declare the reading — in a default run XMLReporter has already
asked, which would hide the miss. FailedReporter is untouched.

./gradlew build passes: 0 failures, 0 errors, testng-test-osgi included.

Summary by CodeRabbit

• Bug Fixes

  • Reports now preserve configuration parameter values from invocation time, preventing later mutations from appearing in final report output.
  • XML reporting maintains accurate parameter values and avoids unnecessary re-rendering.
  • Emailable reports no longer add an empty filler row when factory parameters are already reported.
  • Retained reporting data is cleaned up after reporters finish processing.

• Documentation

  • Clarified configuration listener ordering and parameter snapshot retention behavior.

• Changelog

  • Added release notes for version 7.13.0.

[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: f7ac180b-7d5b-4f62-86fc-3c8f83d00a5e

📥 Commits

Reviewing files that changed from the base of the PR and between 6af5899 and fa2d98b.

📒 Files selected for processing (1)

• CHANGES.txt

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

Configuration parameter snapshots remain available through report generation when a late reader requests retention. Snapshot discard still removes values when no late reader exists. Tests verify invocation-time values, rendering counts, late reads, and cleanup.

Changes

Configuration snapshot retention

Layer / File(s)	Summary
Snapshot store retention contract testng-core/src/main/java/org/testng/internal/reporters/ParameterSnapshots.java	Adds held-until-reporting state and APIs. Snapshot discard preserves captured values until reporting when requested.
Reporter capture integration testng-core/src/main/java/org/testng/internal/reporters/ParameterSnapshotReader.java, testng-core/src/main/java/org/testng/internal/reporters/ParameterSnapshotRecorder.java, testng-core/src/main/java/org/testng/TestRunner.java	Snapshot readers use retained capture requests. Documentation describes late reads, listener ordering, and configuration result dispatch.
Retention and XML reporting coverage testng-core/src/test/java/org/testng/internal/reporters/*, testng-core/src/test/java/org/testng/reporters/snapshot/CountedConfigurationParameterSample.java, testng-core/src/test/java/test/reports/XmlReporterParametersTest.java, testng-core/src/test/resources/testng.xml, CHANGES.txt	Adds tests for request ordering, late reads, cleanup, invocation-time values, and exact rendering counts. Registers the new reporter test class and documents the behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to fa2d9

The change preserves passed configuration snapshots for late reports and includes targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant TestInvocation
  participant ParameterSnapshotReader
  participant ParameterSnapshots
  participant XmlReporter
  TestInvocation->>ParameterSnapshots: capture invocation parameters
  ParameterSnapshotReader->>ParameterSnapshots: request capture held until reporting
  XmlReporter->>ParameterSnapshots: read snapshot during generateReport
  XmlReporter->>ParameterSnapshots: detachFrom after reporting

Loading

Suggested reviewers: krmahadevan

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 8 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: retaining passed configuration snapshots for later reporting.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 37.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 8 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/keep-passed-configuration-snapshots

---

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.}