Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support params in Factory in ITestResult #1911

Merged
merged 1 commit into from
Sep 4, 2018
Merged

Support params in Factory in ITestResult #1911

merged 1 commit into from
Sep 4, 2018

Conversation

krmahadevan
Copy link
Member

Closes #1041

Fixes #1041 .

Did you remember to?

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

We encourage pull requests that:

  • Add new features to TestNG (or)
  • Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

juherr
juherr reviewed Aug 31, 2018
View reviewed changes
Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't catch why you use IParameterInfo.embeddedInstance everywhere instead of directly dealing with IParameterInfo.

src/main/java/org/testng/ITestNGMethod.java Outdated
@@ -211,4 +211,8 @@
default boolean isDataDriven() {
return false;
}

default Object getRawInstance() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some javadoc is missing. What's the difference between raw instance and instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this for better readability

src/main/java/org/testng/internal/BaseTestMethod.java Outdated
@@ -715,4 +724,8 @@ public ConstructorOrMethod getConstructorOrMethod() {
public String getQualifiedName() {
return getRealClass().getName() + "." + getMethodName();
}

public Object getRawInstance() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override is missing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

src/main/java/org/testng/internal/TestNGClassFinder.java Outdated
@@ -34,7 +34,7 @@

private static final String PREFIX = "[TestNGClassFinder]";
private final ITestContext m_testContext;
private final Map<Class<?>, List<Object>> m_instanceMap = Maps.newHashMap();
private final Map<Class<?>, List<Object>> m_instanceMap = Maps.newHashMap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space added

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@krmahadevan
Copy link
Member Author

@juherr

I don't catch why you use IParameterInfo.embeddedInstance everywhere instead of directly dealing with IParameterInfo.

The reason behind doing that is because a user has the ability to retrieve the instance associated with a ITestNGMethod object. If we were to replace the underlying object with a IParameterInfo then we will affect backward compatibility.

The other option for this would be to deprecate the getInstance() method and introduce a new one that returns a IParameterInfo object. Should we go that route ? I chose not to, because there's not many people who are looking for this feature and I didn't want to introduce refactoring (Even if its a very minimal one wherein people will have to replace the call to getInstance() with a call to the new method that returns a IParameterInfo object).

juherr
juherr approved these changes Sep 4, 2018
View reviewed changes
src/main/java/org/testng/ITestNGMethod.java
* @return - A {@link IParameterInfo} object that represents details about the parameters
* associated with the factory method.
*/
default IParameterInfo getFactoryMethodParamsInfo() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not have the method on a IFactoryMethod which extends ITestNGMethod?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will need a bit more involved changes. I will take it up later in a separate PR once I am back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, create an issue as reminder

@krmahadevan
Copy link
Member Author

Merging this PR since @juherr has approved it.

@krmahadevan krmahadevan merged commit bbeafd1 into testng-team:master Sep 4, 2018
@krmahadevan krmahadevan deleted the bugfix-1041 branch September 4, 2018 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juherr juherr juherr approved these changes

@cbeust cbeust Awaiting requested review from cbeust

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Factory dataprovider parameters not displayed in testresult
2 participants
@krmahadevan @juherr