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

ISPN-7116 Test methods that use a TestNg dataProvider are duplicated in #4762

Conversation

alanfx
Copy link
Contributor

@alanfx alanfx commented Jan 13, 2017

JUnit reports

https://issues.jboss.org/browse/ISPN-7116

I was unable to find the class that writes the current
"TEST-.xml" file, so I created a new reporter. I know it isn't
ideal to have two files generated, but I need this fix to proceed with
Polarion onboarding. The JIRA can be left open, if this solution isn't
satisfactory.
Also updated TestNGTestListener to output the parameters to the console

@alanfx
Copy link
Contributor Author

alanfx commented Jan 13, 2017

@danberindei can you take a look? Thanks

private String generateFileName(ISuite suite) {
ITestNGMethod[] testMethods = suite.getResults().values().iterator().next().getTestContext().getAllTestMethods();
if (suite.getName().contains("Surefire suite") && testMethods.length > 0) {
return "POLARION-" + testMethods[0].getConstructorOrMethod().getDeclaringClass().getPackage().getName();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to work on the file naming code a little more. Picking the package of the first method doesn't seem to always be a good choice. :-(

@alanfx alanfx force-pushed the ISPN-7116/Test_methods_that_use_a_TestNg_dataProvider_are_duplicated_in_JUnit_reports branch from 7908c75 to 4d6612a Compare January 18, 2017 16:59
@alanfx
Copy link
Contributor Author

alanfx commented Jan 18, 2017

Ok, this is ready for review

@alanfx alanfx force-pushed the ISPN-7116/Test_methods_that_use_a_TestNg_dataProvider_are_duplicated_in_JUnit_reports branch from 4d6612a to a4d9b99 Compare January 18, 2017 21:55
@alanfx alanfx force-pushed the ISPN-7116/Test_methods_that_use_a_TestNg_dataProvider_are_duplicated_in_JUnit_reports branch 3 times, most recently from 8030165 to 998e847 Compare January 19, 2017 16:54
Copy link
Member

@danberindei danberindei left a comment

Choose a reason for hiding this comment

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

@alanfx I was able to debug by setting <forkCount>0</forkCount> and see that surefire uses its own listener for writing the report, org.apache.maven.surefire.testng.ConfigurationAwareTestNGReporter. I'm not sure if you can disable it from the configuration...

int endIndex = path.indexOf(File.separator + "target");
if (endIndex != -1) {
// Use the project name as the filename
if (!System.getenv("MAVEN_PROJECTBASEDIR").equals(System.getProperty("basedir"))) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting a NullPointerException here if I run the build from IntelliJ.

TBH I don't see a reason to treat runs from the project directory and from the module directory differently, as the output directory is already different for each module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this environment variable won't be defined unless you are running it from Maven. So here is what I am trying to handle. If you run the command from the top level of the project and use --project to run tests in a sub-module, everything is ok. MAVEN_PROJECTBASEDIR includes the directory name for the top level of the project, so I can strip that out to name the report file which may include multiple directories above the target directory. However, if you run the maven command from a subdirectory, then MAVEN_PROJECTBASEDIR is set to the sub-module name, so then the code picks the first directory name above target on the output directory. The issue is that multiple modules use a name like remote or embedded that could use the extra context in the results file name. I hope that makes sense.

I could see the org.apache.maven.surefire.testng.ConfigurationAwareTestNGReporter in the list of listeners, but I could not get the debugger to break into it. Thanks for the forkCount tip. I'll try that to see if I can either reuse that code, or at least disable the creation of the TEST-suite.xml files.

@alanfx alanfx force-pushed the ISPN-7116/Test_methods_that_use_a_TestNg_dataProvider_are_duplicated_in_JUnit_reports branch from 998e847 to b76cd55 Compare January 23, 2017 19:40
@alanfx
Copy link
Contributor Author

alanfx commented Jan 23, 2017

@danberindei I pushed a small change to reverse the order of the comparison on line 318 that I think will fix the NullPointerException in IntelliJ. So far, I haven't been able to figure out how to disable ConfigurationAwareTestNGReporter.

@danberindei
Copy link
Member

@alanfx I now get a NPE on line 320 when I run it from IntelliJ.

My point was that it should be enough to store each module's report in a separate basedir, you don't really need a different file name. E.g. surefire generates core/target/surefire-reports/TEST-TestSuite.xml, not core/target/surefire-reports/TEST-core.xml. If you need to move all the reports in a single directory later, you could rename the files to include the module name at that stage.

Regarding debugging, I was able to sort-of debug ConfigurationAwareTestNGReporter by adding this dependency to core:

  <dependency>
     <groupId>org.apache.maven.surefire</groupId>
     <artifactId>surefire-testng</artifactId>
     <version>2.18.1</version>
     <exclusions>
        <exclusion>
           <groupId>org.testng</groupId>
           <artifactId>testng</artifactId>
        </exclusion>
     </exclusions>
  </dependency>

It didn't really work for me, as the downloaded sources didn't match the loaded classes (probably because I tried with 2.19.1 and there were other indirect dependencies on 2.18.1), but at least you'll be able to see some code :)

@alanfx
Copy link
Contributor Author

alanfx commented Jan 24, 2017

Hey @danberindei, yes I forgot to add a null check for System.getenv("MAVEN_PROJECTBASEDIR"). I can push that change as well. QE will be taking these files and importing them into Polarion for each build, so I'd like to avoid the extra step of renaming the files. I was able to step into ConfigurationAwareTestNGReporter with the forkCount change, but in the end I'd still have to create a custom reporter. The test case name in ConfigurationAwareTestNGReporter is generated in a private method, and I have not been able to figure out a way to disable that listener. The POM file already sets usedefaultlisteners=false.

@alanfx alanfx force-pushed the ISPN-7116/Test_methods_that_use_a_TestNg_dataProvider_are_duplicated_in_JUnit_reports branch 2 times, most recently from 6cf2ab2 to b162c00 Compare January 24, 2017 14:56
@alanfx
Copy link
Contributor Author

alanfx commented Jan 24, 2017

@danberindei suggested using the infinispan.module-suffix property to name the results file which works, except for the Integration tests. I'm trying to figure that out now.

@alanfx alanfx force-pushed the ISPN-7116/Test_methods_that_use_a_TestNg_dataProvider_are_duplicated_in_JUnit_reports branch from b162c00 to 490903e Compare January 25, 2017 02:37
@slaskawi
Copy link
Contributor

Hey @alanfx !

Just thinking out laud... wouldn't it be better to create a small script that would scan all directories after the build and push those results into Polarion? Surefire already spits out an xml and a text file (the latter is probably easier to parse).

This way we wouldn't interfere with the build itself and adding yet another step in Jenkins or TeamCity is really easy.

@alanfx alanfx force-pushed the ISPN-7116/Test_methods_that_use_a_TestNg_dataProvider_are_duplicated_in_JUnit_reports branch from 490903e to 6791ae2 Compare January 25, 2017 13:55
@alanfx
Copy link
Contributor Author

alanfx commented Jan 25, 2017

Hey @slaskawi, eventually these steps will need to be automated, but initially this will be a more manual process. It usually takes a few runs of the tests to get a set of results that is worth reporting!

@danberindei I think this is ready to merge now, and I hope you'll be pleased! I got tripped up yesterday, because some of the integration tests do not use TestNg.

JUnit reports

https://issues.jboss.org/browse/ISPN-7116

I was unable to find the class that writes the current
"TEST-<suite>.xml" file, so I created a new reporter. I know it isn't
ideal to have two files generated, but I need this fix to proceed with
Polarion onboarding. The JIRA can be left open, if this solution isn't
satisfactory.
Also updated TestNGTestListener to output the parameters to the console
@alanfx alanfx force-pushed the ISPN-7116/Test_methods_that_use_a_TestNg_dataProvider_are_duplicated_in_JUnit_reports branch from 6791ae2 to 46761be Compare January 25, 2017 20:22
@danberindei
Copy link
Member

Thanks Alan, integrated!

@slaskawi Even if the file name didn't matter, we'd still have the problem of test methods with dataProviders being duplicated...

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