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.

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

Test/Step name only uses method name instead of test name #147

Closed
gtque opened this issue Sep 23, 2020 · 17 comments
Closed

Test/Step name only uses method name instead of test name #147

gtque opened this issue Sep 23, 2020 · 17 comments
Assignees
Milestone

Comments

@gtque
Copy link

gtque commented Sep 23, 2020

Commit 102b03e87020f7855198770104a99bab350fca1f

refactored the step name into a new method:
protected String createStepName(ITestResult testResult) { return testResult.getMethod().getMethodName(); }

But did not preserve the original logic of checking for test name. This completely breaks the naming between versions for anyone using/setting the testng test name and is preventing us from updating to the latest version.

Here is the original if statement in the buildStartStepRq method:
if (testResult.getTestName() != null) { testStepName = testResult.getTestName(); } else { testStepName = testResult.getMethod().getMethodName(); }

@HardNorth
Copy link
Member

@gtque There was an essential reason to change it. Unfortunately I'm currently on vacation without a laptop to prepare a detailed answer. But nothing prevent you to implement your own "get test step name" method to move forward. This is a part of our official API and won't be changed.

@gtque
Copy link
Author

gtque commented Sep 27, 2020

@HardNorth I'm not entirely sure I understand what you are suggesting.
Are you suggesting that i implement a custom "get test name" method in our test class that implements ITest and TestNG?
Or that I extend TestNGService from this project and then implement my own reportportal listener that uses that instance?

I would be very interested in the reason. I understand moving it to the method, but I am interested in why the behavior was altered.

@gtque
Copy link
Author

gtque commented Oct 1, 2020

@HardNorth ok, but how will just overriding that method work?

ReportPortalTestNGListener always uses your implementation of TestNGService.java.

Also why was the behavior changed?
The change now means if you have a data driven test, then each run in report portal all show as the exact same name and there is no way to distinguish between the runs short of reading through the logs.

@brian30040
Copy link

@HardNorth Any updates for a more detailed answer about why the behavior was changed? This change breaks thousands of our tests!

@HardNorth
Copy link
Member

@gtque @brian30040, Ok I implemented a special test for that case and reverted implementation on a previous one for a moment. Here what I got:
Screen Shot 2020-12-07 at 6 13 39 PM

Well, what I see here is that parent test and both step names are the same. And how that behavior helps to solve the issue you mentioned here (about parameterized tests)? Actually you can distinguish one test from another if you go inside a step and open "Item Details" section. In the section you will find parameter list and their values.

Actually it was literally a long-standing bug which ended in to a situation when people start messing tests and test steps. And it was fixed in version 5. Because the view on the screenshot above is a step level view (with method types). And test level is a level above. Test steps and their test should have different names because they are different entities on different levels. Moreover we need to keep naming logic similar for similar frameworks, so current behavior conforms with behavior of JUnit 4 and JUnit 5 agents.

Again, you are free to customize your naming convention by overriding createStepName method. You can use our example for that: https://github.com/reportportal/examples-java/blob/master/example-testng-logback/src/main/java/com/epam/reportportal/example/testng/logback/extension/ParametersAsAttributesTest.java

I think the discussion is over here.

@gtque
Copy link
Author

gtque commented Dec 7, 2020

@HardNorth
Your assumption then is that the "test method" is a step, but whether you call it a step or a test doesn't matter. I don't want to have to go into the individual tests/steps to identify it. For the person implementing the test that is not a problem, but from the perspective of someone reading and trying to understand the report, that is not acceptable.

Going into "item details" is also not helpful as we pass a collection of objects, making the identification of that particular run difficult. Which is why we use the TestNG test name to inject a more detailed name into the "test" name. Whether you call it a step or test is simply a matter of terminology depending on the framework you are using. In either case, we run the same method (whether you call it a test or a step is irrelevant) and modify the test name.

When reviewing the JUNIT report produced, the test/step name is as we expect. It is only in report portal where it is not correct.

This is the TestNG agent, not the JUNIT4 or JUNIT 5 agent. As such it seems you should be supporting TestNG.
I am inclined to continually create a new issue for this issue as this agent-java-testng does not properly support TestNG. I understand your stance of "you can implement your own createStepName", and we have, but this causes a point of possible breakage in the future if you change the underlying behavior again.

And no, I don't think the discussion is over.

@gtque
Copy link
Author

gtque commented Dec 7, 2020

@HardNorth
I think I understand where you got your results from. You simply overrode the test name, probably with a hard coded value. But we override the testname with a parameter that comes from the test data, thus making the test name dynamic instead of static.

@gtque
Copy link
Author

gtque commented Dec 7, 2020

@HardNorth
I implement the ITest interface and override the getTestName() method.
So what should be done, is check to see if the class implements the ITest class, if it does, use the getTestName() method.

@HardNorth
Copy link
Member

HardNorth commented Dec 8, 2020

@gtque I would not recommend you keep posting duplicate issues, because in that case I'll just block you for the organization.

As I said createStepName method was introduced for everyone who wants to customize their step names. You can find the same method in every Java agent since version 5. But you are trying to convince me that your custom logic, which calculates test names is the only right way. But RP do take it into account and reports it according to its hierarchy. And if I revert the change back then we get the same names on two different levels. I believe I'll get another batch of angry users in this case, since such behavior will look broken for them.

So my bet here is to keep it as is. Anyway it's open source, you are free to implement your own agent.
And BTW, few lines from our license:

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

@gtque
Copy link
Author

gtque commented Dec 8, 2020

@HardNorth
I am not trying to convince you that my custom logic is the only right way.
I am trying to convince you that your agent-java-testNG should support the functionality provided by TestNG, in this case the getTestName() method from the ITest interface. Whether the name is dynamically generated in that method or not is irrelevant. I only mentioned that to address the results you saw in your testing.

The problem with having to implement my own agent is that it creates a point of potential future conflict. As you mentioned in your license, you are free to implement any changes you all see fit and I use it "as is".

In order to support your suggestion, I not only have to implement a TestNGService but I also now have to implement and maintain an implementation of BaseTestNGListener to load it properly.
I understand that frameworks change and evolve over time, and you are free to make the changes as you see fit, but in this case you changed underlying functionality that was actually closer to fully supporting features from TestNG in favor of removing it to fix a "bug" to support JUNIT4 and 5 and keeping the naming similar. However, the junit report generated by TestNG supports getTestName() but you no longer do.

The bug is that you have a specific TestNG agent that no longer supports the test/step name provided from testng and instead just use the method name.

@HardNorth
Copy link
Member

@gtque There is one simple rule for discussions, called "burden of proof". That means a statement maker is also responsible for providing proofs of the statement. So please provide proofs of your words, for example, a test project.

@gtque
Copy link
Author

gtque commented Dec 9, 2020

@HardNorth
Here is your proof: https://github.com/gtque/rptestngtestname

There is a custom service and listener implemented. If the test class is in the com.thegoate.rp.custom package it uses testResult.getName(). Otherwise it uses the default createStepName() from TetstNGService. The extra logger messages are to provide some a look into the behavior of getName() versus getTestName().

From what I have found, you can just use testResult.getName() and TestNG will resolve to either getTestName() if implemented or to the method name if getTestName() returns null.

Here is the junit xml output. Notice the testnames, if overridden either with the test annotation or the getTestName method, appear as expected in the xml report, but not in report portal.
test.zip

some screen shots from report portal:
image
image
image

@HardNorth HardNorth reopened this Dec 17, 2020
@HardNorth HardNorth self-assigned this Jan 27, 2021
@HardNorth HardNorth added this to the 6.0 milestone Jan 27, 2021
@HardNorth
Copy link
Member

The fix will be not earlier that in 6th version of RP and agents, since it breaks retry and re-run logic.

@sergio67174
Copy link

When will this be fixed it has been almost 3 years from this and I am having the same issue report is coming with test method name instead of test name given?

@reportportal reportportal deleted a comment from alkachawda Dec 5, 2023
@HardNorth
Copy link
Member

@alkachawda Please don't spam any issue you can find with irrelevant comments. TestNG has nothing to do with pytest and vice versa. I already deleted one from you and I'm going to delete this one too. If you post another one I'll just block you for the whole project.

@reportportal reportportal deleted a comment from alkachawda Dec 6, 2023
HardNorth added a commit that referenced this issue Jan 22, 2024
@HardNorth
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants