Streamline native injection w.r.t cloning

[krmahadevan]

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?

• Add test case(s)
• Update CHANGES.txt

[juherr]

In fact, the issue is located in the clone method of XmlTest where we have a side effect.
In fact, all XmlXxx classes should not be cloned (we don't have injection tests for them).
But, IMO, the best way to fix the issue is to remove the clone method which has no real value add here.

[krmahadevan]

But, IMO, the best way to fix the issue is to remove the clone method which has no real value add here.

No we can't do that. XmlTest is exposed to users and I dont know how many people are relying on the customized clone() method that XmlTest provides. So I feel that we cannot just like that remove off the behavior.

[krmahadevan]

@juherr - Please let me know if you are fine with my responses and if we can merge this.

[juherr]

Ok for not removing the clone method but I'd prefer that the bug is fixed instead of overengineering.

https://github.com/cbeust/testng/blob/master/src/main/java/org/testng/xml/XmlTest.java#L488

-XmlTest result = new XmlTest(getSuite()); 
+XmlTest result = new XmlTest(getSuite().clone());

Should fix the issue too.

Please, deprecate the clone methods too.

[krmahadevan]

@juherr

+XmlTest result = new XmlTest(getSuite().clone());

This will break backward compatibility in terms of what we are providing as a functionality via the clone() method to all existing users. The current functionality guarantees that when I clone an XmlTest object, it would automatically also get added to the current XmlSuite. With the change you are suggesting, that will be broken.

[krmahadevan]

instead of overengineering.

Where did you feel that the fix was over-engineering? Can you please help call that out? Just curious.

[krmahadevan]

Ping @juherr

[juherr]

Where did you feel that the fix was over-engineering?

Using annotations instead of just fixing the issue.

This will break backward compatibility in terms of what we are providing as a functionality via the clone() method to all existing users. The current functionality guarantees that when I clone an XmlTest object, it would automatically also get added to the current XmlSuite. With the change you are suggesting, that will be broken.

Rigth but I can't imagine someone is using clone() method and the implementation doesn't respect the contract.
BTW, I think making this breaking change in the major version has low risks.

[krmahadevan]

@juherr

Using annotations instead of just fixing the issue.

I felt that was a bit harsh. I was also fixing the issue. Just that I fixed it in a different way and it perhaps didn't align to what you were expecting.

Anyways. I am closing this PR.

[juherr]

Right, it fixed the issue too but thanks to a workaround. I hope I didn't offend you.

I still think we should remove the Clonable here, and that's why I prefer to wait before releasing because it allows us some regressions.

[krmahadevan]

Right, it fixed the issue too but thanks to a workaround.

I didn't consider it as a workaround because my focus was to ensure that backward compatibility is not broken and people don't end up with functionality broken from their side. So in that aspect, I was right. This case wasn't the case of adhering to a contract (there is no interface involved here per se). So we are not at will to change the underlying functionality of a public API or remove a public API at will without going through a deprecation strategy.

I hope I didn't offend you.

There is no scope for emotions when it comes to code. I like to believe that. That's the only way one learns. I still don't know a lot of things when compared to you and @cbeust. So I don't think I am entitled to feel offended :) So none taken Julien. I still get to learn from you. That's more important for me. Thats what brought me into Open source.

I still think we should remove the Cloneable here

Sure we should. But it would need to go through a deprecation strategy. A major version change IMO does mean we are licensed to remove things without providing a proper warning to end-users who consume an API.

I prefer to wait before releasing because it allows us some regressions.

I don't understand that rationale. How is us releasing beta versions and then a final version different from us releasing a major version followed by minor versions as bug fix versions ? To the end-user its still the same. They still need to switch versions in either case to get hold of a new functionality. Just because a version is branded as "beta" doesn't mean that users would not use it. Here its not the case of a regression, its a bug fix. Thats how I look at it. And bugs should be fixed by addressing the problem without breaking backward compatibility whatsoever. Because if we do, then its only us who need to respond to queries in various forums (stackoverflow, testng google groups etc). And I am trying to see what can be done, to keep the churn levels to the minimum.

…

[juherr]

100% agree with you.

But the responsibility is not the same when you use a beta version.
To be honest, I really don't care to have regressions between beta versions.
And a major version is an opportunity to have API changes (which is not the case with minor versions).
I agree too with the deprecation strategy but I'd like to move quicker at the same time. I think ome compromises are possible if we feel I won't break too many things (that's my opinion with clone here).

If you think it is too risky, so we can merge this PR but with comments for the temporary code and without making it public.