Uploaded image for project: 'Maven Surefire'
  1. Maven Surefire
  2. SUREFIRE-2039

Skipped test classes are getting into the non-skipped test classes reports

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Duplicate
    • 3.0.0-M5
    • None
    • None
    • None

    Description

      The setup: maven-surefire-plugin 3.0.0-M5 and junit 5.8.2.

      Consider a simple scenario, two test classes, ATest and BTest, one Disabled and one not respectively, each has three test methods. After running mvn surefire:test the report will indicate that 4 tests were run and one of them was skipped:

      [INFO]  T E S T S
      [INFO] -------------------------------------------------------
      [INFO] Running org.example.test.BTest
      [WARNING] Tests run: 4, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.062 s - in org.example.test.BTest
      [INFO] 
      [INFO] Results:
      [INFO] 
      [WARNING] Tests run: 4, Failures: 0, Errors: 0, Skipped: 1
      

      And if we enable ATest and disable BTest, then the report will only mention running three tests, none skipped:

      [INFO]  T E S T S
      [INFO] -------------------------------------------------------
      [INFO] Running org.example.test.ATest
      [INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.079 s - in org.example.test.ATest
      [INFO] 
      [INFO] Results:
      [INFO] 
      [INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0
      

      This behavior is the result of the way how org.apache.maven.plugin.surefire.report.TestSetRunListener handles skipped tests. It adds them to the detailsForThis and only prints after the test set is finished which happens when a non-disabled test set is finished thus adding skipped test results to the statistics of a non-skipped test set. And, of course, if the disabled test is ran after the non-disabled test, it won't be reported at all.

      The reproducer can be found here: https://github.com/SammyVimesFiledIssues/SUREFIRE-2039

      Attachments

        Issue Links

          Activity

            sdanilov Semyon Danilov added a comment -

            I have a naive approach patch for solving this issue: https://github.com/apache/maven-surefire/pull/493 but, as I've written in the description for the PR, I don't really like it and some feedback from the maintainers can help me improve it.

            sdanilov Semyon Danilov added a comment - I have a naive approach patch for solving this issue: https://github.com/apache/maven-surefire/pull/493 but, as I've written in the description for the PR, I don't really like it and some feedback from the maintainers can help me improve it.
            tibordigana Tibor Digana added a comment - - edited

            Hey Sammy, no no, this has nothing to do with you. I had only a problem with the reproducible project because I have not noticed it exists and the code in TestSetRunListener. Pls see the pull requests on Github in Surefire, you will see that there are several PRs which touch the class TestSetRunListener and StatelessXmlReporter. 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.

            tibordigana Tibor Digana added a comment - - edited Hey Sammy, no no, this has nothing to do with you. I had only a problem with the reproducible project because I have not noticed it exists and the code in TestSetRunListener. Pls see the pull requests on Github in Surefire, you will see that there are several PRs which touch the class TestSetRunListener and StatelessXmlReporter. 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.
            sdanilov Semyon Danilov added a comment -

            Thank you, got it! I will get to the writing of integration tests right away. Thank you for the guidance!

            sdanilov Semyon Danilov added a comment - Thank you, got it! I will get to the writing of integration tests right away. Thank you for the guidance!
            tibordigana Tibor Digana added a comment -

            Thank you as well. We will do our best!

            tibordigana Tibor Digana added a comment - Thank you as well. We will do our best!
            andpab Andreas Pabst added a comment -

            This is essentially a duplicate of SUREFIRE-2032. It was fixed in 3.0.0-M8 by gnodet.

            The output is now correct:

            [INFO] --- surefire:3.0.0-M8:test (default-cli) @ surefire-bug-poc ---
            [INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
            [INFO]
            [INFO] -------------------------------------------------------
            [INFO]  T E S T S
            [INFO] -------------------------------------------------------
            [INFO] Running org.example.test.BTest
            [WARNING] Tests run: 3, Failures: 0, Errors: 0, Skipped: 3, Time elapsed: 0.006 s - in org.example.test.BTest
            [INFO] Running org.example.test.ATest
            [INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.03 s - in org.example.test.ATest
            [INFO]
            [INFO] Results:
            [INFO]
            [WARNING] Tests run: 6, Failures: 0, Errors: 0, Skipped: 3
            [INFO]
            [INFO] ------------------------------------------------------------------------
            [INFO] BUILD SUCCESS 
            

            gnodet You could close this as fixed in 3.0.0-M8. And also decline the corresponding pull request which is obsolete now: #493

            andpab Andreas Pabst added a comment - This is essentially a duplicate of SUREFIRE-2032 . It was fixed in 3.0.0-M8 by gnodet . The output is now correct: [INFO] --- surefire:3.0.0-M8:test (default-cli) @ surefire-bug-poc --- [INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider [INFO] [INFO] ------------------------------------------------------- [INFO]  T E S T S [INFO] ------------------------------------------------------- [INFO] Running org.example.test.BTest [WARNING] Tests run: 3, Failures: 0, Errors: 0, Skipped: 3, Time elapsed: 0.006 s - in org.example.test.BTest [INFO] Running org.example.test.ATest [INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.03 s - in org.example.test.ATest [INFO] [INFO] Results: [INFO] [WARNING] Tests run: 6, Failures: 0, Errors: 0, Skipped: 3 [INFO] [INFO] ------------------------------------------------------------------------ [INFO] BUILD SUCCESS gnodet You could close this as fixed in 3.0.0-M8. And also decline the corresponding pull request which is obsolete now: #493

            People

              Unassigned Unassigned
              sdanilov Semyon Danilov
              Votes:
              2 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: