Maven Surefire
  1. Maven Surefire
  2. SUREFIRE-446

Surefire fails to capture TestNG results when forkMode=always

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.9
    • Component/s: process forking
    • Labels:
      None
    • Environment:
      Maven 2.0.8, JDK 1.5.0_12, WinXP
    • Complexity:
      Intermediate
    • Testcase included:
      yes
    • Number of attachments :
      1

      Description

      The interplay between surefire.Surefire and testng.TestNGDirectoryTestSuite does not properly notify the ReportManager such that it reports 0 executed tests after the end of the day. IT to reproduce attached.

      Also confirmed against 2.5-SNAPSHOT.

        Activity

        Hide
        Dan Fabulich added a comment -

        Repro'd.

        Show
        Dan Fabulich added a comment - Repro'd.
        Dan Fabulich made changes -
        Field Original Value New Value
        Fix Version/s 2.4.2 [ 14062 ]
        Hide
        Dan Fabulich added a comment -

        This is trickier than it looks... I'm not 100% certain that it even makes sense to run TestNG with forkMode=always; it isn't necessarily safe to do so (especially when tests depend on tests in other classes). [This certainly explains how Benjamin found this bug... ;-)]

        Ideally, TestNG would provide its own support for forkMode=always... Short of that, it's hard to see how we can hack it into Surefire on top of the framework that's already in place.

        Show
        Dan Fabulich added a comment - This is trickier than it looks... I'm not 100% certain that it even makes sense to run TestNG with forkMode=always; it isn't necessarily safe to do so (especially when tests depend on tests in other classes). [This certainly explains how Benjamin found this bug... ;-)] Ideally, TestNG would provide its own support for forkMode=always... Short of that, it's hard to see how we can hack it into Surefire on top of the framework that's already in place.
        Dan Fabulich made changes -
        Fix Version/s 2.4.2 [ 14062 ]
        Fix Version/s 2.x [ 13647 ]
        Hide
        Benjamin Bentmann added a comment -

        This certainly explains how Benjamin found this bug...

        What should I say, I am just evil minded

        it isn't necessarily safe to do so

        The same is true with running tests in parallel. If your test suite contains non-thread-safe method calls, well, multi-threaded tests are a bad idea, too. So, is Surefire going to drop its parameter "parallel" because an incompotent user could make the tests errorneously fail? I wouldn't feel quite comfortable with a build tool that restricts me from doing something just because it might not work although - for a special use-case - it will work.

        Short of that, it's hard to see how we can hack it into Surefire on top of the framework that's already in place.

        Naively spoken, I would expect that forkMode=always behaves as if TestNGDirectoryTestSuite found only a single test class.

        Show
        Benjamin Bentmann added a comment - This certainly explains how Benjamin found this bug... What should I say, I am just evil minded it isn't necessarily safe to do so The same is true with running tests in parallel. If your test suite contains non-thread-safe method calls, well, multi-threaded tests are a bad idea, too. So, is Surefire going to drop its parameter "parallel" because an incompotent user could make the tests errorneously fail? I wouldn't feel quite comfortable with a build tool that restricts me from doing something just because it might not work although - for a special use-case - it will work. Short of that, it's hard to see how we can hack it into Surefire on top of the framework that's already in place. Naively spoken, I would expect that forkMode=always behaves as if TestNGDirectoryTestSuite found only a single test class.
        Hide
        Andreas Andreou added a comment -

        The 2.4-collab-SNAPSHOT version does not suffer from this.

        Show
        Andreas Andreou added a comment - The 2.4-collab-SNAPSHOT version does not suffer from this.
        Hide
        Joel Wiegman added a comment -

        I'm suffering from this as well. The 2.4-collab-SNAPSHOT has plenty of unfixed bugs and I unfortunately cannot use this version.

        As for forkMode=always, this is a relatively common practice in unit tests. Consider the following scenarios:

        1. I have a crucial static variable and would like it reset to a known state for each test / test class
        2. I bootstrap my unit tests with some data by using an in-memory database that's created on the fly and would like it to be reset to a known state for each test/test class as well

        In either of these scenarios, the only way to return to a known state in a unit test is to ensure you're starting with a fresh JVM.

        I get that TestNG offers some testing functionality that is "outside" the unit testing paradigm (@BeforeSuite and @AfterSuite), but I agree with Benjamin... let's let the developer use whatever configuration he/she wants and let them make their own mistakes.

        Show
        Joel Wiegman added a comment - I'm suffering from this as well. The 2.4-collab-SNAPSHOT has plenty of unfixed bugs and I unfortunately cannot use this version. As for forkMode=always, this is a relatively common practice in unit tests. Consider the following scenarios: 1. I have a crucial static variable and would like it reset to a known state for each test / test class 2. I bootstrap my unit tests with some data by using an in-memory database that's created on the fly and would like it to be reset to a known state for each test/test class as well In either of these scenarios, the only way to return to a known state in a unit test is to ensure you're starting with a fresh JVM. I get that TestNG offers some testing functionality that is "outside" the unit testing paradigm (@BeforeSuite and @AfterSuite), but I agree with Benjamin... let's let the developer use whatever configuration he/she wants and let them make their own mistakes.
        Hide
        Andreas Andreou added a comment -

        Are there any plans to somehow resolve this?

        I'd like to help resolve this, or at least include a patch here for those that would also be interested - any guidance on which part of the code I should start looking at?

        Show
        Andreas Andreou added a comment - Are there any plans to somehow resolve this? I'd like to help resolve this, or at least include a patch here for those that would also be interested - any guidance on which part of the code I should start looking at?
        Hide
        Marvin Froeder added a comment -

        I made a small patch that doesn't solve the problem of the testNG html report, but, it makes this issue more manageable, specially for those running testNG + maven + forked=always on CI servers

        Index: surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGDirectoryTestSuite.java
        ===================================================================
        --- surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGDirectoryTestSuite.java	(revision 1033415)
        +++ surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGDirectoryTestSuite.java	(working copy)
        @@ -98,6 +98,8 @@
                     throw new TestSetFailedException( "Unable to find test set '" + testSetName + "' in suite" );
                 }
         
        +        this.options.put( "suitename", testSetName );
        +
                 ReporterManager reporterManager = reporterManagerFactory.createReporterManager();
                 startTestSuite( reporterManager, this );
         
        

        This will create on report.xml per test. This eliminates the problem on CI builds and makes it much more useful for people running it locally, since now it is possible to see the test failure result.

        VELO

        Show
        Marvin Froeder added a comment - I made a small patch that doesn't solve the problem of the testNG html report, but, it makes this issue more manageable, specially for those running testNG + maven + forked=always on CI servers Index: surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGDirectoryTestSuite.java =================================================================== --- surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGDirectoryTestSuite.java (revision 1033415) +++ surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGDirectoryTestSuite.java (working copy) @@ -98,6 +98,8 @@ throw new TestSetFailedException( "Unable to find test set '" + testSetName + "' in suite" ); } + this .options.put( "suitename" , testSetName ); + ReporterManager reporterManager = reporterManagerFactory.createReporterManager(); startTestSuite( reporterManager, this ); This will create on report.xml per test. This eliminates the problem on CI builds and makes it much more useful for people running it locally, since now it is possible to see the test failure result. VELO
        Hide
        Brian Demers added a comment -

        I applied the previous patch.

        Show
        Brian Demers added a comment - I applied the previous patch.
        Hide
        Peter Lynch added a comment - - edited

        velo's patch was applied and included in the 2.9 release.

        http://svn.apache.org/viewvc?view=revision&revision=1133426

        Didn't see another reason to keep this issue open - if there is, feel free to reopen it.

        Show
        Peter Lynch added a comment - - edited velo's patch was applied and included in the 2.9 release. http://svn.apache.org/viewvc?view=revision&revision=1133426 Didn't see another reason to keep this issue open - if there is, feel free to reopen it.
        Peter Lynch made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Peter Lynch made changes -
        Fix Version/s 2.9 [ 17312 ]
        Fix Version/s Backlog [ 13647 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Benjamin Bentmann
          • Votes:
            10 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: