[SUREFIRE-2039] Fix skipped test classes reporting

[SammyVimes]

https://issues.apache.org/jira/browse/SUREFIRE-2039

Why: Skipped test classes reports are being added into the non-skipped test classes reports (see attached JIRA issue for detailed information). The reproducer can be found here: https://github.com/SammyVimesFiledIssues/SUREFIRE-2039

What: A simple fix that tests if the report entry of a skipped test is a skipped test class report, not a skipped test method report, and reports it. This fix is not ideal because it relies on a side effect (empty test name which indicates that the report is about a test class, not a test method), I don't really like this approach because I think there should be a separate BOOTERCODE for the skipped test class, but at the same time, I think this issue and the solution approach should be reviewed by maintainers because I don't have a deep knowledge of the source code yet. I will happily rewrite the patch according to suggestions on the matter. I also wonder where should I put a test for this scenario because as far as I can see, no test is relying on the specific implementation of the TestReportListener class, so it probably should be a new test class.

Following this checklist to help us incorporate your contribution quickly and easily:

• [x] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
• [x] Each commit in the pull request should have a meaningful subject line and body.
• [x] Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles, where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice is to use the JIRA issue title in the pull request title and in the first line of the commit message.
• [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• [x] Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
• [x] You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.

• [x] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• [x] In any other case, please file an Apache Individual Contributor License Agreement.

[Tibor17]

@SammyVimes We fixed some issue regarding skipped tests in SUREFIRE-1556 but I am not sure it is the same. Sorry, but I do not trust this JIRA report because i do not know what test framework you use, how you "skip" the test and I am not able to reproduce this problem upon the JIRA description. How can I prove that this is a general problem and not individual problem and then why should I accept a fix which is not the right one. If we accepted all patches like this without a deep understanding and debugging the problem, we would have a serious problem which would be bigger than the reported one in JIRA.

So sometimes, it is better to make a small step and precisely write JIRA ticket. Of course if you have debugged the code and helped us with the findings around the root cause, it would help and maybe we would make the fix.

[SammyVimes]

@Tibor17 JIRA report has a reproducer attached to it, it's really small (https://github.com/SammyVimesFiledIssues/SUREFIRE-2039). I fully understand that this patch at this moment is not the right one (it doesn't even have tests) and, of course, I do not expect it to be merged right away. On the contrary, as a fellow Apache Committer, I'm willing to work on this issue iff you deem it worthy

[Tibor17]

Hey @SammyVimes ,

I would rather see your help in the reimplementation of TestSetRunListener and StatelessXmlReporter. The patches don't make sense because they would have short life anyway and it would very consufe the development. These implementations are not in perfect state and adding workarounds would mean that we would not be sure during the refactoring. Anyway, I appreciate your efforts and I would like to invite you into the refactoring. Meanwhile pls close this ticket and write integration test which fails in current master and open a new PR with the test. This would really help! Pls see my notices from JIRA:

It really does not make sense to report any pull requests against these two classes with small workarounds or patches or fixes for three reasons:

1. they are problematic and they exist for many years
2. we have changes or prerequisite for (3)
3. we have to refactor both classes but as you may have noticed the implementation relies on ordering of classes and methods, and the report is finished in one point of time when the test set has finished and not when the Provider has finished. Now we have problem with re-run feature, this issue, natural ordering, parallel tests executions. We have this refactoring in our roadmap and we have to do one thing, rework these two classes and employ the {{testRunId}} and {{RunMode}} and not to accept patches because these would definitely overload us. We have to smesh down the current implementation and then verify your requirements are met by the new implementation. So this effort you have made is pushing us but unfortunately it won't help, even if you want to help us, but the turn is on our side. So nowadays we should release M5, and then we should rework the implementation and we can do it together with your ITs in a new PR. What would definitely help us is to provide us with integration tests which fail now in master, and you can do it in a PR even if it would have a broken CI, that's not a problem, and after we have got these classes reimplemented, we would make an agreement to adopt these ITs and push the entire work to the master as one complete solution. Currently accepting the patches into an archaic reporter would be problematic and cannot be accepted, otherwise we as maintainers would blow up the problem to bigger and bigger one, and of course this has to be stopped.

[andpab]

This pull request is obsolete. The referenced issue is a duplicate of SUREFIRE-2032 which was fixed in 3.0.0-M8 with pull request #564.

[slawekjaranowski]

@SammyVimes thanks for your effort