Mojo's Cobertura Maven Plugin
  1. Mojo's Cobertura Maven Plugin
  2. MCOBERTURA-83

Can't run install and cobertura:cobertura without running test phase twice

    Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: 2.2
    • Fix Version/s: None
    • Labels:
      None
    • Number of attachments :
      3

      Description

      I want run as part of my CI process the goals:

      mvn install cobertura:cobertura

      The problem is that this causes the test phase to be ran twice.
      AFAIK, there's no way to do this and it's a major issue when doing CI.

      1. patch
        3 kB
        Ryan Stewart
      2. report-only.patch
        2 kB
        Charles Honton
      3. reportOnlyAndInstrumentOutputDirPatch.patch
        4 kB
        Shant Stepanian

        Issue Links

          Activity

          Hide
          Rafael Mahnovetsky added a comment -

          I have the same issue but with running mvn install site. It is a real pain as our CI server teamcity will show that we have run twice as many tests and display duplicate entries of all the tests run. It also take the build longer to run, and gets worse with more tests we write.

          From my investigation into solving this issue some people prefer to run the tests twice as instrumentation can cause issues with run certain types of tests. So maybe a config flag would be nice?

          Show
          Rafael Mahnovetsky added a comment - I have the same issue but with running mvn install site. It is a real pain as our CI server teamcity will show that we have run twice as many tests and display duplicate entries of all the tests run. It also take the build longer to run, and gets worse with more tests we write. From my investigation into solving this issue some people prefer to run the tests twice as instrumentation can cause issues with run certain types of tests. So maybe a config flag would be nice?
          Hide
          Ricky Hazelwood added a comment -

          Surefire report plugin has a goal which creates reports only without running tests. Cobertura plugin should be able to do the same.

          Show
          Ricky Hazelwood added a comment - Surefire report plugin has a goal which creates reports only without running tests. Cobertura plugin should be able to do the same.
          Hide
          Stephen Connolly added a comment -

          Be aware, running the tests twice is actually a good thing.

          I have had tests that pass when run with cobertura even though the underlying code is broken... i.e. running the tests normally and they fail!

          I have had tests that fail when run with cobertura and pass without...

          In both cases there is usually a bug that I would not have found if I had only run the tests once.

          Please reconsider trying to run them only once.... in reality you should try and run them five or six times (Sun JVM, IBM JVM, BEA JVM, both with and without coverage... and consider using both cobertura and emma)

          If there is a discrepancy between any of these runs then likely you have badly written code and a bug just waiting to be found.

          Show
          Stephen Connolly added a comment - Be aware, running the tests twice is actually a good thing. I have had tests that pass when run with cobertura even though the underlying code is broken... i.e. running the tests normally and they fail! I have had tests that fail when run with cobertura and pass without... In both cases there is usually a bug that I would not have found if I had only run the tests once. Please reconsider trying to run them only once.... in reality you should try and run them five or six times (Sun JVM, IBM JVM, BEA JVM, both with and without coverage... and consider using both cobertura and emma) If there is a discrepancy between any of these runs then likely you have badly written code and a bug just waiting to be found.
          Hide
          Hugo Palma added a comment -

          Stephen, i agree that tests ideally should be run several times in different conditions. But let's not stray from this issue.

          The problem at hands is that in this use case cobertura forces the exact same tests under the same conditions to be run twice, this both causes heavier load and also erratic reports in some CI servers as Rafael as pointed out.

          Show
          Hugo Palma added a comment - Stephen, i agree that tests ideally should be run several times in different conditions. But let's not stray from this issue. The problem at hands is that in this use case cobertura forces the exact same tests under the same conditions to be run twice, this both causes heavier load and also erratic reports in some CI servers as Rafael as pointed out.
          Hide
          Rafael Mahnovetsky added a comment -

          I'm actually run the tests 3 times now. One for surefire, another for cobertura to fail the build if the coverage isn't met and another for the cobertura report.

          Show
          Rafael Mahnovetsky added a comment - I'm actually run the tests 3 times now. One for surefire, another for cobertura to fail the build if the coverage isn't met and another for the cobertura report.
          Hide
          Guillaume Jeudy added a comment -

          Given Stephen's comment, running tests twice is a feature? Even though there might be good reasons to pass tests twice I think users should have the freedom of overriding this default behavior and run tests only once.

          Show
          Guillaume Jeudy added a comment - Given Stephen's comment, running tests twice is a feature? Even though there might be good reasons to pass tests twice I think users should have the freedom of overriding this default behavior and run tests only once.
          Hide
          Ha Ryon added a comment -

          I doubt that every application are writing tests that tests thread sync.
          While most application tests execution takes at least a few minutes.
          It sounds to me that systematically doubling the execution time just because there might be a test that cobertura will mess up, is kinda overkill.

          Make it an option to run the tests twice (for example by setting to executions of surefire at different times).

          Make it that I can run in a few seconds my unit tests, verify coverage of the classes I modified, and let me run a way longer nightly build where I can run integration-tests as well as instrumented/non-intrumented validations.

          You might add a large warning in cobertura plugin documentations of all the consequences of using coverage upon the reliance of our tests.

          Show
          Ha Ryon added a comment - I doubt that every application are writing tests that tests thread sync. While most application tests execution takes at least a few minutes. It sounds to me that systematically doubling the execution time just because there might be a test that cobertura will mess up, is kinda overkill. Make it an option to run the tests twice (for example by setting to executions of surefire at different times). Make it that I can run in a few seconds my unit tests, verify coverage of the classes I modified, and let me run a way longer nightly build where I can run integration-tests as well as instrumented/non-intrumented validations. You might add a large warning in cobertura plugin documentations of all the consequences of using coverage upon the reliance of our tests.
          Hide
          Ryan Stewart added a comment - - edited

          I'm attaching a patch file with a new class, CoberturaReportOnlyMojo, that mostly duplicates code from the CoberturaReportMojo and allows you to run a cobertura report without triggering the test phase in a parallel lifecycle. It implements a new goal named "report". To use it in its current form, do three things:

          1) Configure the plugin to specify output formats and bind the instrument goal to the process-test-classes phase, like so:

           
          <plugin>
              <groupId>org.codehaus.mojo</groupId>
              <artifactId>cobertura-maven-plugin</artifactId>
              <configuration>
                  <formats>
                      <format>xml</format>
                      <format>html</format>
                  </formats>
              </configuration>
              <executions>
                  <execution>
                      <id>cobertura-instrument</id>
                      <phase>process-test-classes</phase>
                      <goals>
                          <goal>instrument</goal>
                      </goals>
                  </execution>
              </executions>
          </plugin>
          

          2) Modify the surefire plugin to point to the instrumented classes instead of the default target/classes. By default, the plugin places the instrumented classes at target/generated-classes/cobertura:

           
          <plugin>
              <artifactId>maven-surefire-plugin</artifactId>
              <configuration>
                  <classesDirectory>${project.build.directory}/generated-classes/cobertura</classesDirectory>
              </configuration>
          </plugin>
          

          3) Run "mvn clean test cobertura:report", and your reports will, by default, be in $

          {project.build.directory}

          /cobertura. Naturally, you could bind the report goal to a lifecycle phase if you wanted to.

          Show
          Ryan Stewart added a comment - - edited I'm attaching a patch file with a new class, CoberturaReportOnlyMojo, that mostly duplicates code from the CoberturaReportMojo and allows you to run a cobertura report without triggering the test phase in a parallel lifecycle. It implements a new goal named "report". To use it in its current form, do three things: 1) Configure the plugin to specify output formats and bind the instrument goal to the process-test-classes phase, like so: <plugin> <groupId> org.codehaus.mojo </groupId> <artifactId> cobertura-maven-plugin </artifactId> <configuration> <formats> <format> xml </format> <format> html </format> </formats> </configuration> <executions> <execution> <id> cobertura-instrument </id> <phase> process-test-classes </phase> <goals> <goal> instrument </goal> </goals> </execution> </executions> </plugin> 2) Modify the surefire plugin to point to the instrumented classes instead of the default target/classes. By default, the plugin places the instrumented classes at target/generated-classes/cobertura: <plugin> <artifactId> maven-surefire-plugin </artifactId> <configuration> <classesDirectory> ${project.build.directory}/generated-classes/cobertura </classesDirectory> </configuration> </plugin> 3) Run "mvn clean test cobertura:report", and your reports will, by default, be in $ {project.build.directory} /cobertura. Naturally, you could bind the report goal to a lifecycle phase if you wanted to.
          Hide
          Alaa Mohssen Nassef added a comment - - edited

          So, is there any news about if this is going to be fixed any time soon, especially that it's one of the really popular issues?

          Show
          Alaa Mohssen Nassef added a comment - - edited So, is there any news about if this is going to be fixed any time soon, especially that it's one of the really popular issues?
          Hide
          Brian Wawok added a comment -

          This would be very useful for us also

          Show
          Brian Wawok added a comment - This would be very useful for us also
          Hide
          Charles Honton added a comment -

          Surefire had similar problem and was fixed with a Report-only mojo

          Show
          Charles Honton added a comment - Surefire had similar problem and was fixed with a Report-only mojo
          Hide
          Charles Honton added a comment -

          Patch to create report-only goal

          Show
          Charles Honton added a comment - Patch to create report-only goal
          Hide
          Brian Wawok added a comment -

          The patch above may make cobertura:report behave.. but cobertura:verify will still trigger a new build. So helpful, not not perfect for our use case...

          Show
          Brian Wawok added a comment - The patch above may make cobertura:report behave.. but cobertura:verify will still trigger a new build. So helpful, not not perfect for our use case...
          Hide
          Christoph Kutzinski added a comment - - edited

          Any news on this?

          BTW http://nayidisha.com/techblog/an-over-eager-code-coverage-tool also links to a patch which is reported to work fine.

          Show
          Christoph Kutzinski added a comment - - edited Any news on this? BTW http://nayidisha.com/techblog/an-over-eager-code-coverage-tool also links to a patch which is reported to work fine.
          Hide
          Shant Stepanian added a comment -

          Hi,
          Is there an update on this? We ran into this issue as well.

          I have attached a patch (cobertura-report-only-patch-20120201.patch) that defines a "report" goal that just generates the report and does nothing else. The existing functionality remains.

          This seems like something reasonable to add, as we already have a corresponding "instrument" task, so why not a standalone report task?

          Is it possible to get this into the next release?

          I'd be willing to coordinate w/ someone on this to get this in

          Thanks,
          Shant

          Show
          Shant Stepanian added a comment - Hi, Is there an update on this? We ran into this issue as well. I have attached a patch (cobertura-report-only-patch-20120201.patch) that defines a "report" goal that just generates the report and does nothing else. The existing functionality remains. This seems like something reasonable to add, as we already have a corresponding "instrument" task, so why not a standalone report task? Is it possible to get this into the next release? I'd be willing to coordinate w/ someone on this to get this in Thanks, Shant
          Hide
          Robert Scholte added a comment -

          Let me just quote Stephen again:

          Be aware, running the tests twice is actually a good thing.

          I have had tests that pass when run with cobertura even though the underlying code is broken... i.e. running the tests normally and they fail!

          I have had tests that fail when run with cobertura and pass without...

          In both cases there is usually a bug that I would not have found if I had only run the tests once.

          Please reconsider trying to run them only once.... in reality you should try and run them five or six times (Sun JVM, IBM JVM, BEA JVM, both with and without coverage... and consider using both cobertura and emma)

          If there is a discrepancy between any of these runs then likely you have badly written code and a bug just waiting to be found.

          Maybe it should be surefire which should detect that it is actually running exactly the same test-run again...

          Show
          Robert Scholte added a comment - Let me just quote Stephen again: Be aware, running the tests twice is actually a good thing. I have had tests that pass when run with cobertura even though the underlying code is broken... i.e. running the tests normally and they fail! I have had tests that fail when run with cobertura and pass without... In both cases there is usually a bug that I would not have found if I had only run the tests once. Please reconsider trying to run them only once.... in reality you should try and run them five or six times (Sun JVM, IBM JVM, BEA JVM, both with and without coverage... and consider using both cobertura and emma) If there is a discrepancy between any of these runs then likely you have badly written code and a bug just waiting to be found. Maybe it should be surefire which should detect that it is actually running exactly the same test-run again...
          Hide
          Christoph Kutzinski added a comment -

          Be aware, running the tests twice is actually a good thing.

          Others have said this before and I agree: yes, running tests under different conditions is a maybe a good thing, but certainly no tool should force this behaviour upon me.
          E.g. It would also be a good idea to run tests multiple times in different run orders to prevent inadverted dependencies between tests. Still I wouldn't want surefire to do this when I wouldn't request it to do so.

          Show
          Christoph Kutzinski added a comment - Be aware, running the tests twice is actually a good thing. Others have said this before and I agree: yes, running tests under different conditions is a maybe a good thing, but certainly no tool should force this behaviour upon me. E.g. It would also be a good idea to run tests multiple times in different run orders to prevent inadverted dependencies between tests. Still I wouldn't want surefire to do this when I wouldn't request it to do so.
          Hide
          Shant Stepanian added a comment -

          Agreed w/ Christoph on this - various clients may have different use cases, and so the plugin should not force this behavior on everyone. There are many requests on this JIRA ticket and others (not to mention that this JIRA now references patches from 4 people who tried to solve this issue), so it seems like it should be addressed in the next version and it would benefit a lot of people.

          In addition, I noticed that the "instrument" goal was changing the build output directory to the instrumented directory. This had negative consequences on our project as the jar plugin was now packaging the instrumented files. I've replaced my previous patch w/ a new one that configures the output directory change as a configuration parameter.

          In general, it seems like there should be two usages of this plugin:

          • via the cobertura goal, which essentially does everything for you
          • via specific usage of the instrument and "report" goals, which allows finer grained control, including the ability to not run your tests twice. The last patch I had on not changing the outputDirectory goes along with this

          Please let us know when these changes can be added. I'd be willing to work w/ someone on this

          Thanks,
          Shant

          Show
          Shant Stepanian added a comment - Agreed w/ Christoph on this - various clients may have different use cases, and so the plugin should not force this behavior on everyone. There are many requests on this JIRA ticket and others (not to mention that this JIRA now references patches from 4 people who tried to solve this issue), so it seems like it should be addressed in the next version and it would benefit a lot of people. In addition, I noticed that the "instrument" goal was changing the build output directory to the instrumented directory. This had negative consequences on our project as the jar plugin was now packaging the instrumented files. I've replaced my previous patch w/ a new one that configures the output directory change as a configuration parameter. In general, it seems like there should be two usages of this plugin: via the cobertura goal, which essentially does everything for you via specific usage of the instrument and "report" goals, which allows finer grained control, including the ability to not run your tests twice. The last patch I had on not changing the outputDirectory goes along with this Please let us know when these changes can be added. I'd be willing to work w/ someone on this Thanks, Shant
          Hide
          Robert Scholte added a comment -

          If the tests are run on two different folders, namely on target/classes (the original sources) and on target/generated-sources/cobertura (the instrumented sources) then that's the expected behavior. If we change this and something goes wrong everybody will point at us.
          If the instrumented classes end up in the jar, then that's a bug.

          Exactly the same test-runs shouldn't run twice since SUREFIRE-257

          Show
          Robert Scholte added a comment - If the tests are run on two different folders, namely on target/classes (the original sources) and on target/generated-sources/cobertura (the instrumented sources) then that's the expected behavior. If we change this and something goes wrong everybody will point at us. If the instrumented classes end up in the jar, then that's a bug. Exactly the same test-runs shouldn't run twice since SUREFIRE-257
          Hide
          Ian Robertson added a comment -

          What about making this change in behavior configurable, but not the default option? Then you don't have to worry about finger pointing, but still give people the ability to speed up their builds if they are willing to accept a mild increase in risk.

          Show
          Ian Robertson added a comment - What about making this change in behavior configurable, but not the default option? Then you don't have to worry about finger pointing, but still give people the ability to speed up their builds if they are willing to accept a mild increase in risk.
          Hide
          Robert Scholte added a comment -

          See the attributes on http://mojo.codehaus.org/cobertura-maven-plugin/cobertura-mojo.html

          • Invokes the execution of the lifecycle phase test prior to executing itself.
          • Executes in its own lifecycle: cobertura.

          You can't change attributes like you can with parameters, so optional is not an option.

          Show
          Robert Scholte added a comment - See the attributes on http://mojo.codehaus.org/cobertura-maven-plugin/cobertura-mojo.html Invokes the execution of the lifecycle phase test prior to executing itself. Executes in its own lifecycle: cobertura. You can't change attributes like you can with parameters, so optional is not an option.
          Hide
          Shant Stepanian added a comment -

          Possibly the intent of my fix was not clear earlier, given the subject-line of this JIRA.

          What I'd like to propose (and this is basically what Ryan Stewart had proposed earlier in this JIRA) is to leave the cobertura goal alone, and instead add support for a report-only goal. A similar "report-only" goal looks like was proposed in the SUREFIRE-257 jira that you referenced for surefire

          What I think people are wanting is just a way to not have the tests execute twice. It seems to be difficult to do that via the cobertura task or via some configuration in the surefire plugin. So what we'd like is just a basic report-only goal, which is already available in many other similar plugins.

          Hence, if people wanted to just have run the tests once, they would access individual goals, similar to Ryan Stewart's proposal, i.e. 1) call the instrument goal 2) have the surefire plugin point to the instrumented folder 3) call the report-only goal to generate the report.

          The reportOnlyAndInstrumentOutputDirPatch.patch has a patch for the report-only goal, as well as a configuration in the instrument goal to not switch the output folder, as otherwise having access to the instrument goal itself causes more trouble than we needed (as only the surefire plugin really needs the instrumented classes - all other plugins like code analysis [say findbugs] and jar/packaging need the actual classes).

          This should be a reasonable compromise where 1) all behaviors for existing goals and configurations remain as is, including the cobertura task 2) people can still have better access to specific goals to handle executions the way that they want, if they choose that route. Short of having a better integration between surefire and cobertura, this seems like a best way to go, and would solve problems

          Show
          Shant Stepanian added a comment - Possibly the intent of my fix was not clear earlier, given the subject-line of this JIRA. What I'd like to propose (and this is basically what Ryan Stewart had proposed earlier in this JIRA) is to leave the cobertura goal alone, and instead add support for a report-only goal. A similar "report-only" goal looks like was proposed in the SUREFIRE-257 jira that you referenced for surefire What I think people are wanting is just a way to not have the tests execute twice. It seems to be difficult to do that via the cobertura task or via some configuration in the surefire plugin. So what we'd like is just a basic report-only goal, which is already available in many other similar plugins. Hence, if people wanted to just have run the tests once, they would access individual goals, similar to Ryan Stewart's proposal, i.e. 1) call the instrument goal 2) have the surefire plugin point to the instrumented folder 3) call the report-only goal to generate the report. The reportOnlyAndInstrumentOutputDirPatch.patch has a patch for the report-only goal, as well as a configuration in the instrument goal to not switch the output folder, as otherwise having access to the instrument goal itself causes more trouble than we needed (as only the surefire plugin really needs the instrumented classes - all other plugins like code analysis [say findbugs] and jar/packaging need the actual classes). This should be a reasonable compromise where 1) all behaviors for existing goals and configurations remain as is, including the cobertura task 2) people can still have better access to specific goals to handle executions the way that they want, if they choose that route. Short of having a better integration between surefire and cobertura, this seems like a best way to go, and would solve problems
          Hide
          Shant Stepanian added a comment -

          Another option is to explicitly embed skipTests=false in the lifecycle.xml file, as mentioned at this link
          http://nayidisha.com/techblog/an-over-eager-code-coverage-tool Thus, developers can configure their surefire plugin as they want, and add <skipTests>true</skipTests> in their pom. Thus, when the regular test execution goes through, it will get skipped; but it will get fired on the cobertura plugin. From seeing the feedback on that link, it seems like it had success for people

          Either approach would seem fine, whether improving the standalone goals to work as described in my previous post, or to embed some property in the cobertura lifecycle that can toggle test execution in that lifecycle while disabling it in the regular execution. If you are uncomfortable w/ the variable being called "skipTests", then possibly you can coordinate w/ the surefire folks to see if they can add some variable like "runOnlyForCoverage" tests, which could be useful for other code coverage plugins too like EMMA or Clover

          Show
          Shant Stepanian added a comment - Another option is to explicitly embed skipTests=false in the lifecycle.xml file, as mentioned at this link http://nayidisha.com/techblog/an-over-eager-code-coverage-tool Thus, developers can configure their surefire plugin as they want, and add <skipTests>true</skipTests> in their pom. Thus, when the regular test execution goes through, it will get skipped; but it will get fired on the cobertura plugin. From seeing the feedback on that link, it seems like it had success for people Either approach would seem fine, whether improving the standalone goals to work as described in my previous post, or to embed some property in the cobertura lifecycle that can toggle test execution in that lifecycle while disabling it in the regular execution. If you are uncomfortable w/ the variable being called "skipTests", then possibly you can coordinate w/ the surefire folks to see if they can add some variable like "runOnlyForCoverage" tests, which could be useful for other code coverage plugins too like EMMA or Clover

            People

            • Assignee:
              Unassigned
              Reporter:
              Hugo Palma
            • Votes:
              34 Vote for this issue
              Watchers:
              29 Start watching this issue

              Dates

              • Created:
                Updated: