WIP for #3111

[juherr]

FYI

Current status: tests are failing

[juherr]

@krmahadevan I'm trying to replace Object instance by IInstanceInfo<?> instance everywhere possible without an api signature change.
As the return type of getInstance() will change, maybe breaking the api won't be an huge step.

I think the tests are currently failing because test methods are not always found (an instance.getInstance() is certainly missing somewhere).

[krmahadevan]

@juherr - Replacing getInstancce() with IInstanceInfo seems wierd. I say this because IInstanceInfo has a specific use case (i.e., it comes into play only when the user's @Factory method returns back this interface instance) and in all other usecases it's not relevant. Please see here for more context

So why would we want to try and unify this functionality?

We have also tried to alter this interface without adding default implementation (not a major concern and easily fixable), but doing this will cause backward compatibility loss.

We are also always assuming that getParameters() will be non-null. I think this assumption will not be true, when the factory method has no parameters.

[juherr]

@krmahadevan Thanks for your feedbacks.

Replacing getInstancce() with IInstanceInfo seems wierd. I say this because IInstanceInfo has a specific use case (i.e., it comes into play only when the user's @factory method returns back this interface instance) and in all other usecases it's not relevant. Please see testng-team/testng-team.github.io#70 for more context

Agree. I was looking for a structure that hosts the meta-data of an instance. I didn't remember that IInstanceInfo has a specific usage. In the other hand, IInstanceInfo could be the low-level object where the factory method may provide sugar syntax. BTW, it won't be a big deal to change it be another name.

So why would we want to try and unify this functionality?

It was not an objective. I need to think deeper since you pointed me to the previous discussion.

We have also tried to alter this interface without adding default implementation (not a major concern and easily fixable), but doing this will cause backward compatibility loss.

Ok.

We are also always assuming that getParameters() will be non-null. I think this assumption will not be true, when the factory method has no parameters.

I disagree with that. "no parameters" means the collection of parameters is empty.

[juherr]

IInstanceInfo could be the low-level object where the factory method may provide sugar syntax.

In fact, I've already said something close the last time: testng-team/testng-team.github.io#70 (comment)

[krmahadevan]

I disagree with that. "no parameters" means the collection of parameters is empty.

This guarantee is being provided by the implementation and not by the interface.

[juherr]

Yes, agree. But using Optional will not assure that implementation won't return null ;)

[juherr]

BTW, if you have some free time, I'm open to help because I haven't found yet where is the issue into my branch :(

[krmahadevan]

BTW, if you have some free time, I'm open to help because I haven't found yet where is the issue into my branch :(

I will spend sometime tomorrow and find out what is going on in this branch.

[juherr]

I think the issues come from missing getInstance where instances are not considered as IInstanceInfo and/or bad management of uuid/hash.

The main goal is not to merge the changes but to have a clear view of what should be done.

About the conceptual model, my current point of view is that every instance should be created by a factory (maybe virtual or implicit) that has parameters from a dataprovider (maybe virtual or implicit).
A dataprovider may be a merge from another dataprovider and extra parameters.

Possible alternative that is maybe closer to the current implementation:
a factory has parameters from Parameters and parameters are a merge from dataprovider and extra parameters

[krmahadevan]

I think the issues come from missing getInstance where instances are not considered as IInstanceInfo and/or bad management of uuid/hash.

Are you talking about the existing changes that exist in master|my_feature_branch branch, or are you talking about them missing in your PR? I didnt quite ustand.

Either ways, I think we are trying to skin the cat in one shot which perhaps doesnt work. I say this based on my past experiences in terms of refactoring within TestNG (Getting rid of the GraphThreadPoolExecutor and paving the way to working with plain vanilla thread pool executors and also allowing users to work with shared thread pool is IMO a big example of this).

We should do it in phases. The immediate problem/concern at hand is how do we expose the required stuff to the end users without having to change the functionality/signature/user experience (whatever we want to call it).

The second thing is, so should I try and spend time on figuring out what the failures are, or are you saying that this is to be used as a reference implementation to give an idea on where we want to be in the near future.

I am a bit confused.

[juherr]

Sorry. I'm talking about my changes in the current PR.
It is working except for some tests.
I'd like to make it work first and rework changes step by step after.

The way to not break the current interface will not be a big deal because we can easily add a new method instead of using the actual getInstance and add new interface instead of using the actual IInstanceInfo.

My latest comment was about my current understanding of the model that should be implemented. I'm pretty sure the code is already there but not located at the best place.
About the model itself, I think something like testMethod.getInstance().getFactory().getDataProvider().getParameters() should be possible later.

[krmahadevan]

@juherr - I spent sometime looking at the changeset. Here's what I feel

• We need to deprecate getInstance() and add a default implementation of getInstanceInfo() which will be implemented and called everywhere. Right now the code is trying to do a lot of things by doing instanceof checks and casts and I think that's where it is going wrong (I see you have added a comment which is a TODO for this)
• The tests all should be refactored such that there's nowhere a call to getInstance() and everywhere we are invoking only getInstanceInfo() to get the instance details.

Once both these are done, I think the tests would start passing. I also felt that perhaps this changeset can be in a new branch (since I don't see it share much with what I had in my PR except for the interface that I introduced).

[krmahadevan]

@krmahadevan I'm trying to replace Object instance by IInstanceInfo<?> instance everywhere possible without an api signature change. As the return type of getInstance() will change, maybe breaking the api won't be an huge step.

I think the tests are currently failing because test methods are not always found (an instance.getInstance() is certainly missing somewhere).

I strongly feel that we should deprecate getInstance() and introduce a fresh default implementation for getInstanceInfo() which will be called by users. The entire codebase should be swept to start calling getInstanceInfo() internally and getInstance() should ONLY be left to be called by our end users (once they see the deprecation, they will move away from it, and we can perhaps leave it for a release or two before getting rid of it).

camouflaging the IInstanceInfo behind the getInstance() is perhaps is what is breaking the tests everywhere.

[juherr]

Ok. I will cleanup

[juherr]

Done.

Current status: FAILURE 328,3sec, 8200 completed, 1007 failed, 8 skipped, Gradle Test Run :testng-core:test

[juherr]

I think issues are here because equals/hashcode are maybe not well implemented

[krmahadevan]

@juherr - I think that the changes you have introduced are a bit invasive (they are subtle and so they were hard to spot). I have spent time reverting the changes (the blast radius in my opinion is too large).

I have reduced the failures to 12288 tests completed, 24 failed, 6 skipped

IInstanceInfo IMO should ONLY be used in places where we were using the IdentifiableObject.

I think its better you start this again, but this time keep the changes to literally only a find and replace (apart from adopting your proposed model). I think that would be a lot more easier.