fix(runner): keep the injected XmlTest out of the ITestResult parameter snapshot - #3401
Merged
Conversation
ReportTest#github1090 drives GitHub447Sample through GitHub447Listener and asserts that each result kept the data-provider row as it was at invocation time, which is exactly the GITHUB-447 guarantee. GITHUB-1090, per CHANGES.txt, is an unrelated preserve-order fix, so the label sent readers to the wrong issue and left the snapshot behaviour looking uncovered. Name and description only; the fixtures and assertions are unchanged.
…er snapshot TestResult#setParameters stored a clone of every Cloneable parameter. XmlTest is Cloneable and TestNG injects it into configuration and test methods, so it went through that path too -- and XmlTest#clone() is a suite-building helper rather than a copy: it builds its result with new XmlTest(suite), whose constructor registers the new test in that suite. Snapshotting the parameter therefore appended a phantom <test> to the suite that was running, once per invocation that received one, and handed reporters a clone instead of the real test. XmlTest is now kept by reference. XmlTest#clone() itself is untouched, so an IAlterSuiteListener can still rely on it to add a test. The reflective clone-if-Cloneable rule moves out of TestResult into LegacyParameterSnapshotter, whose stated responsibility is to preserve the historical ITestResult parameter representation for backward compatibility. Behaviour is otherwise identical, quirks included: getDeclaredMethod rather than getMethod, nulls passed through, failures degrading to the original reference. Separating the arguments a method was invoked with from the representation reporting needs is the follow-up; naming the compatibility layer is the first step towards it. Fix #1994
Cover the shape GITHUB-1994 was reported with -- native injection mixed into an @optional parameter list -- rather than a third plain injection point, and fold the collector listener into the test that uses it, matching the nested-listener style of Github1649Test and FactoryInstanceTest. The list it fills is no longer wrapped in synchronizedList: the sample suite is single-threaded, and the getter handed the live list to an unsynchronized iteration anyway, so the wrapper advertised a guarantee it did not provide. Trim the argumentative tail of the LegacyParameterSnapshotter javadoc down to the fact it was resting on, rename the private overload to snapshotParameter so resolution between Object[] and Object is not a question, and correct the unit test's javadoc, which claimed to use only public API while calling an internal factory.
|
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 (8)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThis change adds centralized parameter snapshotting for ChangesXmlTest parameter snapshot handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change narrowly prevents injected XmlTest values from being cloned into phantom suite entries while preserving existing clone behavior elsewhere. No actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
This was referenced Aug 20, 2026
krmahadevan
approved these changes
Aug 21, 2026
2 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
2 participants
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.
Fix #1994
Problem
TestResult#setParametersstores a clone of every parameter that implementsCloneable.XmlTestisCloneable, and TestNG injects it into configuration and test methods, so it went through that path too.XmlTest#clone()is a suite-building helper rather than a copy: it builds its result withnew XmlTest(suite), and that constructor doesm_suite.getTests().add(this). Snapshotting an injectedXmlTesttherefore appended a phantom<test>to the suite that was running — with empty<classes>, exactly as reported — and handed reporters a clone instead of the test they asked about.A sample injecting
XmlTestinto@BeforeTest,@Testand@AfterTestends the run with 5 tests in a one-test suite. Four phantoms for three invocations, because the test-method path snapshots twice (TestInvoker→TestResult.newTestResult, thenTestResult.newTestResultFrom).Change
XmlTestis now kept by reference — which is the issue's stated expectation, "Current XmlTest is returned".XmlTest#clone()itself is untouched, so anIAlterSuiteListenercan still rely on its registering side effect —org.testng.xml.issue2866.ThreadCountingSuiteAlteringListenerdoes exactly that and stays green.The reflective clone-if-
Cloneablerule moves out ofTestResultinto a new package-privateLegacyParameterSnapshotter, whose stated responsibility is to preserve the historicalITestResultparameter representation for backward compatibility. That rule is type-blind — it applies to values TestNG injected as readily as to user data — so giving it a name and a single owner is the first step towards separating the arguments a method was invoked with from the representation reporting needs.Behaviour is otherwise identical, quirks included:
getDeclaredMethodrather thangetMethod(a type that merely inheritsclone()is still not snapshotted), nulls passed through, failures degrading to the original reference.Scope is deliberately narrow: no public API, no new annotation, no opt-out mechanism, no reporter migration, no change to
XmlTest#clone()semantics or itsCloneable-ness.LegacyParameterSnapshotteris package-private, so nothing is added to the OSGi surface.XmlTestis the only injectable type this affects:Parametersfixes the natively-injectable set asITestContext, XmlTest, Method, Object[], ITestResult, and of those onlyXmlTestisCloneablewith a reachable declaredclone().Tests
test.inject.issue1994.IssueTest— functional. InjectsXmlTestinto@BeforeTest,@Test, and into an@AfterTestthat mixes the injection into an@Optionalparameter list, which is the shape the issue was reported with. Asserts the suite still holds exactly oneXmlTest, that it is the original instance, and that all three reported parameters are that same instance. Confirmed red without the fix: 5 tests instead of 1.org.testng.internal.TestResultParametersTest— unit. Pins the four rules throughsetParameters/getParametersrather than through the helper:XmlTestby reference,Cloneablesnapshotted, non-Cloneableby reference,nullpreserved.Assertions use
hasSize+isSameAs, nevercontainsExactly:XmlTestoverridesequals, so a clone compares equal to its original and equality-based assertions do not discriminate.The GITHUB-447 mutable-data-provider coverage is unchanged and stays green; its test was labelled
GITHUB-1090(an unrelated preserve-order fix perCHANGES.txt), corrected in a separate commit.Follow-ups for the "execution vs reporting snapshots" refactoring
TestResult.newTestResultFromand the twosetParameters(source.getParameters())calls inTestInvokerfeed an already-snapshotted array back through the whole loop — this is why the repro produces four phantoms for three invocations.getParameters()returns the live internal array, so a listener can corrupt the snapshot the mechanism exists to protect.FactoryInstance#getParameters()already returnsparameters.clone().XmlTestmisbehaves today by luck —Object[].class.getDeclaredMethod("clone")throws, andITestContext/ITestResult/Methodare notCloneable. Building the reporting snapshot only over user-supplied slots would delete theinstanceof XmlTestcase.getDeclaredMethod("clone")only sees the exact runtime class, so Issue with TextReporter output #447's guarantee is silently conditional on a detail users cannot see.setParameters(null)throws NPE rather than clearing, on a public interface method.Verification
./gradlew build— BUILD SUCCESSFUL, 410 test classes, 0 failures.Summary by CodeRabbit
nullparameters retain expected behavior.XmlTestinstances remain linked to the original running test configuration.