Streamline native injection w.r.t cloning #2001
Conversation
|
@juherr - As decided in #1996 I have added a copy constructor, deprecated the |
| @@ -0,0 +1,16 @@ | |||
| package org.testng.annotations; | |||
There was a problem hiding this comment.
Move it in the internal/annotations package
There was a problem hiding this comment.
@juherr - Here's my rationale behind leaving this exposed ( I also would like to evangelize about this lesser known behavior of TestNG )
Currently when it comes to parameter injection and associating that with the TestResult object, we resort to cloning() if the user's parameter object class implements the Cloneable interface. So there could be a chance wherein the parameters that the user is altering in their tests is perhaps not reflected in their listeners or in their reports because we cloned them.
I think we should enrich our documentation to first call out this behavior explicitly ( a most common use case is a data driven test that is run via a @DataProvider annotation) and via this newly introduced annotation, still let the user control whether they want or do not want their test objects to be cloned by TestNG.
There was a problem hiding this comment.
I disagree because you want to make public something which wasn't asked.
Then, the clone solution was just a quick workaround for a design issue when the same object is used in reports. But the clone solution doesn't cover all the need.
That's why I have some doubts to publish a workaround of a workaround before be really sure how we need to fix the root issue.
There was a problem hiding this comment.
I disagree because you want to make public something which wasn't asked.
The changeset that introduced setting cloned variants of parameters was to fix #447 It was done by you via commit 78001fd
Via this commit we ended up implicitly cloning a parameter (if it was clone able) but never made this information public. IMO we should have done this, because an end-user should know if the real parameter that is being passed to the test method is being used or if its a cloned one.
We haven't heard of anyone mention about this problem only because usually our users dont implement the Cloneable interface to the parameter that they are passing to a data provider.
The moment someone does that, this implicit behavior will become evident.
So now it looks like the root cause was us introducing the Cloneable parameter.
What if the user provided parameter was clone able but the user didn't want it to be clone able ? How would they be able to support that. With collections it may not be possible because a user cannot add that annotation to a collection class (I think this also should be possible if we just enrich @SkipCloning annotation such that it can be added to a parameter just as how a user adds @Optional), but at least with a user's test project defined objects it should be something that they should be able to do.
This was my thinking.
There was a problem hiding this comment.
Yes, I understand your point of view. But mine is the #447 was just a not complete workaround of the root issue.
I agree to release another workaround to fix the first workaround but I'm not feeling it good to make it public because we will have to manage deprecation.
Instead, I prefer to make it private and to find a good solution for the root issue.
Furthermore, I'm not sure my old commit will still work on with Java9 modules due to clone.setAccessible(true).
For the current PR:
- private: OK and waiting for a better fix for Issue with TextReporter output #447
- public: not yet ok :)
There was a problem hiding this comment.
Ok.. how about this one.
For the current issue.
- Rename
SkipCloningtoClone(This annotation will still be public) - Revamp our logic to not look for
Cloneableimplementation alone, but also expect that the user annotate the parameter using@Cloneannotation and only if both these are satisfied we resort to cloning. (This will break Issue with TextReporter output #447 but we are fixing it in the right way. All the user would need to do is add the newly created annotation as well to theirCollectionbased parameters) - Remove
XmlTestfrom being annotated with any annotation.
For a permanent fix for #447 the following would be the recommendation (We can call this out in the documentation)
If TestNG should use a copy of the parameters then the following should be done
- Annotate the parameter using
@Clone - The parameter should implement
Cloneableinterface.
If for this also there's no agreement then I will leave this PR open and then start with the next issue (because I guess we would have come to consensus that Lets agree to disagree)
There was a problem hiding this comment.
SkipCloning or Clone, the problem is still the same: I don't see the use case of the feature.
The Cloneable fix was a quick fix which should be think again.
There was a problem hiding this comment.
Without giving the control of deciding which parameter to be cloned and which not to, how do you propose the problem will be fixed ? Can you please help elaborate that ?
@cbeust what are you thoughts on this ?
Closes testng-team#1994 Currently as part of supporting native injection TestNG resorts to invoking the “clone()” method on the parameter being natively injected. This becomes a problem because XmlTest is the only internal TestNG object that implements the Cloneable interface wherein it resorts to something like a deep copy and adds the cloned instance to the XmlSuite. This causes a lot of phony XmlTest objects to be added up to the suite and causes issues as detailed in the bug. Fixed this by introducing a marker interface to indicate to TestNG to skip cloning and use the object as is. Annotated “XmlTest” with this annotation so that we dont clone xmltest but instead use it as is. Yes this will expose the XmlTest object, but its already exposed to the end user via various different means. So I guess we are ok here.
|
Rebased off of |
|
@krmahadevan Currently testing a new tool |
|
@juherr, based on the output from the GitHub CLI, the files changed in PR #2001 include:
These changes seem to align with the description provided by @krmahadevan, including the addition of a copy constructor, deprecation of the Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Fetch the list of files changed in PR #2001
gh pr view 2001 --repo testng-team/testng --json files
Length of output: 574 Action performedReview triggered. |
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (6)
- CHANGES.txt (1 hunks)
- src/main/java/org/testng/annotations/SkipCloning.java (1 hunks)
- src/main/java/org/testng/internal/TestResult.java (3 hunks)
- src/main/java/org/testng/xml/XmlTest.java (3 hunks)
- src/test/java/test/parameters/ParameterTest.java (3 hunks)
- src/test/java/test/parameters/issue1994/TestclassSample.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/org/testng/annotations/SkipCloning.java
Additional comments: 5
src/test/java/test/parameters/ParameterTest.java (2)
- 17-17: Ensure that
TestclassSampleis used within this test class. If not directly used, consider removing the import statement to keep the code clean.- 134-139: The test
testToEnsureNativeInjectionDoesnotResortToCloningcorrectly asserts thatTestclassSample.countequals 1, aligning with the objective to verify that cloning does not occur. Ensure that the setup for this test guarantees only one instance ofXmlTestis created to validate this assertion accurately.src/main/java/org/testng/internal/TestResult.java (1)
- 293-309: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [280-305]
The implementation of
canAttemptCloningand its usage insetParameterscorrectly checks for the@SkipCloningannotation before attempting to clone parameters. This approach aligns with the objective to selectively bypass cloning for certain objects. Ensure that all relevant classes are annotated with@SkipCloningas needed.src/main/java/org/testng/xml/XmlTest.java (2)
- 20-20: The addition of the
@SkipCloningannotation toXmlTestis appropriate and aligns with the objective to prevent unnecessary cloning ofXmlTestobjects.- 512-516: The deprecation of the
clonemethod in favor of using the copy constructor is a good practice. However, ensure that all references toclone()within the codebase are updated to use the new copy constructor to avoid deprecation warnings.
| @@ -1,4 +1,5 @@ | |||
| Current | |||
| Fixed: GITHUB-1994: Prevent duplication of XmlTest objects when its used as a parameter for Native Injection (Krishnan Mahadevan) | |||
There was a problem hiding this comment.
Fixed: Typo in the summary for the addition related to the Assertion class.
- Added a method in the Assertion class to enable downstream TestNG consumers to override the error message (Ryan Laseter)
+ Added a method in the Assertion class to allow downstream TestNG consumers to override the error message (Ryan Laseter)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Fixed: GITHUB-1994: Prevent duplication of XmlTest objects when its used as a parameter for Native Injection (Krishnan Mahadevan) | |
| Fixed: GITHUB-1994: Prevent duplication of XmlTest objects when its used as a parameter for Native Injection (Krishnan Mahadevan) | |
| Added a method in the Assertion class to allow downstream TestNG consumers to override the error message (Ryan Laseter) |
| @BeforeClass | ||
| public void beforeClass(XmlTest xmlTest) { | ||
| } |
There was a problem hiding this comment.
The beforeClass method is empty. If this is intentional, consider adding a comment explaining why; otherwise, implement the necessary logic or remove it if unused.
| @Test | ||
| public void testMethod() { | ||
| } |
There was a problem hiding this comment.
The testMethod is empty. Similar to beforeClass, add a comment if this is intentional, implement required logic, or remove if unused.
| /** | ||
| * This constructor acts as a copy constructor. Please note that it does not automatically | ||
| * associate the copied {@link XmlTest} object with the current {@link XmlSuite} object and | ||
| * requires it to be done explicitly. | ||
| * | ||
| * @param xmlTest - The {@link XmlTest} object to copy from. | ||
| */ | ||
| public XmlTest(XmlTest xmlTest) { | ||
| XmlTest result = new XmlTest(); | ||
| result.setName(getName()); | ||
| result.setIncludedGroups(getIncludedGroups()); | ||
| result.setExcludedGroups(getExcludedGroups()); | ||
| result.setJUnit(isJUnit()); | ||
| result.setParallel(getParallel()); | ||
| result.setVerbose(getVerbose()); | ||
| result.setParameters(getLocalParameters()); | ||
| result.setXmlPackages(getXmlPackages()); | ||
| result.setTimeOut(getTimeOut()); | ||
|
|
||
| Map<String, List<String>> metagroups = getMetaGroups(); | ||
| for (Map.Entry<String, List<String>> group : metagroups.entrySet()) { | ||
| result.addMetaGroup(group.getKey(), group.getValue()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The copy constructor in XmlTest is implemented correctly but contains an unnecessary instantiation of XmlTest (XmlTest result = new XmlTest();) which is not used. Remove this line to clean up the constructor.
- XmlTest result = new XmlTest();Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /** | |
| * This constructor acts as a copy constructor. Please note that it does not automatically | |
| * associate the copied {@link XmlTest} object with the current {@link XmlSuite} object and | |
| * requires it to be done explicitly. | |
| * | |
| * @param xmlTest - The {@link XmlTest} object to copy from. | |
| */ | |
| public XmlTest(XmlTest xmlTest) { | |
| XmlTest result = new XmlTest(); | |
| result.setName(getName()); | |
| result.setIncludedGroups(getIncludedGroups()); | |
| result.setExcludedGroups(getExcludedGroups()); | |
| result.setJUnit(isJUnit()); | |
| result.setParallel(getParallel()); | |
| result.setVerbose(getVerbose()); | |
| result.setParameters(getLocalParameters()); | |
| result.setXmlPackages(getXmlPackages()); | |
| result.setTimeOut(getTimeOut()); | |
| Map<String, List<String>> metagroups = getMetaGroups(); | |
| for (Map.Entry<String, List<String>> group : metagroups.entrySet()) { | |
| result.addMetaGroup(group.getKey(), group.getValue()); | |
| } | |
| } | |
| /** | |
| * This constructor acts as a copy constructor. Please note that it does not automatically | |
| * associate the copied {@link XmlTest} object with the current {@link XmlSuite} object and | |
| * requires it to be done explicitly. | |
| * | |
| * @param xmlTest - The {@link XmlTest} object to copy from. | |
| */ | |
| public XmlTest(XmlTest xmlTest) { | |
| result.setName(getName()); | |
| result.setIncludedGroups(getIncludedGroups()); | |
| result.setExcludedGroups(getExcludedGroups()); | |
| result.setJUnit(isJUnit()); | |
| result.setParallel(getParallel()); | |
| result.setVerbose(getVerbose()); | |
| result.setParameters(getLocalParameters()); | |
| result.setXmlPackages(getXmlPackages()); | |
| result.setTimeOut(getTimeOut()); | |
| Map<String, List<String>> metagroups = getMetaGroups(); | |
| for (Map.Entry<String, List<String>> group : metagroups.entrySet()) { | |
| result.addMetaGroup(group.getKey(), group.getValue()); | |
| } | |
| } |
WalkthroughThe recent updates to TestNG focus on enhancing the framework's reliability and usability. Key changes include the addition of a method to customize error messages in assertions, improvements to prevent the duplication of Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Closes #1994
Currently as part of supporting native injection
TestNG resorts to invoking the “clone()” method
on the parameter being natively injected.
This becomes a problem because XmlTest is the only
internal TestNG object that implements the Cloneable
interface wherein it resorts to something like a deep
copy and adds the cloned instance to the XmlSuite.
This causes a lot of phony XmlTest objects to be
added up to the suite and causes issues as detailed
in the bug.
Fixed this by introducing a marker interface to
indicate to TestNG to skip cloning and use the object
as is. Annotated “XmlTest” with this annotation
so that we dont clone xmltest but instead use it
as is.
Yes this will expose the XmlTest object, but its
already exposed to the end user via various different
means. So I guess we are ok here.
Fixes #1994 .
Did you remember to?
CHANGES.txtSummary by CodeRabbit
@SkipCloningannotation to prevent cloning of objects for dependency injection.XmlTest.XmlTestobjects used as parameters.@AfterGroupswhen group members fail or are skipped.@BeforeGroupsis invoked only when groups are explicitly specified.