[SUREFIRE-1057] Surefire doesn't detect TestNG test method which fails b...

[mbocek]

Surefire doesn't detect TestNG test method which fails because of exception in @dataProvider method.
Also TestNG SkipException wasn't properly populated to the report.

[Tibor17]

@mbocek LGTM
I have only one problem with other ITs. Can you find out all ITs using TestNG with skipped tests? We need to check their assertions statements and reports before/after this change.

[mbocek]

@Tibor17
I have test this change in this flow:

• created IT
• mvn clean verify - my IT fails
• applying fix - IT finished with succeed

I tried to find testcases which contains SkipException what is programmatical way how to signalizing skip to TestNG but without luck.

Please let me know if you would like to validate some specific testcase or extend it.

[Tibor17]

@agudian
@krosenvold
Can we proceed further with this PR?

[Tibor17]

@mbocek
Your data provider should return double array Object[][].

What release version of surefire plugin had this bug?
In JIRA the bug was found in surefire 2.16. Currently we have 2.18 and it seems that bug has gone in 2.18 or 2.19-SNAPSHOT with other changes.

The problem is that I downloaded your sources, reverted the TestNGReporter.java, commented out DataProviderExceptionReportTest#testSkip(). So I have only one test method testDataProvider() in DataProviderExceptionReportTest.
In my code the expected values are .assertTestSuiteResults( 1, 0, 1, 0 ); in IT, and the
TEST-testng.DataProviderExceptionReportTest.xml does not contain "skip" because the single test is marked with failure in surefire report.

Am i doing something wrong or have i misunderstood?

Pls keep this PR open, we can still utilize your effort.
I can see that this IT can be still pushed to the project.

[mbocek]

@Tibor17
I have improved IT.

I'll try to explain where is the problem. When i was digging around i have found difference between behavior in TestNG between versions < 5.9 and 6.0 < . After version 5.9 exception in data provider is no more reported to TestNG listener as failure but as a skip. That's the reason why i have changed onTestSkipped in the listener.

With this fix we will get also more information when test is skipped by regular way in TestNG (like SkipException).

What was changed in IT:

• please correct me if i'm wrong, but integration tests for TestNG are running with more then 7 years old version of TestNG like 5.7, so i have changed dependency to last stable - 6.8.8 for this particular test case.
• i have changed assertion from simple string comparison to xml parsing and evaluating results from xml document.
  So test should be more reliable.

[Tibor17]

Thx for your update. I'll have a look in the weekend.

[Tibor17]

Ok I understand. It sounds like TestNG bug.

I am afraid that you should report a bug for TestNG, very similar progress with
https://jira.codehaus.org/browse/SUREFIRE-1113
Try to do it now because TestNG seems to make the release soon.

Our release due date isn't planned yet but I can say that we may have chance to do it in the EO January. Your IT looks very good. If the TestNG could fix this issue in February, we wouldn't need the change in TestNGReporter.java but the IT can be pushed with TestNG fixed version, right?

[Tibor17]

@mbocek
I have found the bug reported in TestNG testng-team/testng#401.
What about to commit only IT using TestNG 5.x and I will close the critical bug https://jira.codehaus.org/browse/SUREFIRE-1057 ?
Then I will open new improvement issue for this IT in order to replace TestNG version with > 6.8.8 and we will listen to the bug reported in TestNG.

[mbocek]

@Tibor17
Bug looks like right one. But it is reported more then year ago. I'm afraid that it is not bug but it is a feature. You know it's long time from testng version 6.0.

If you think it is best what we can do then i can modify the testcase for testng version 5.7.

Maybe one point which can be discussed: At the moment surefire not handling any skip information for testng. For junit there is ignore message populated to the skip report element. This fix improve skip information as well.

[Tibor17]

But it is reported more then year ago.
Do they have a mailinglist in TestNG? Perhaps we should force them to have a look.

You know it's long time from testng version 6.0.
I understand.

If you think it is best what we can do then i can modify the testcase for testng version 5.7.
yes at the moment

Maybe one point which can be discussed: At the moment surefire not handling any skip information for testng. For junit there is ignore message populated to the skip report element. This fix improve skip information as well.

Let me check it. Maybe we would have to open another JIRA issue as an improvement not related to this one.

[mbocek]

Test was modified to run against different versions of testng and you should be able to simply use it without surefire patch.

I have sent a message to testng dev mailing list with query for clarification: https://groups.google.com/forum/#!topic/testng-dev/oOhjfASvyKw