New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregate Report Maven Goal #388

Merged
merged 27 commits into from Apr 19, 2016

Conversation

Projects
None yet
9 participants
@marchof
Member

marchof commented Mar 22, 2016

To finally offer a solution for multi module Maven projects (#18) I did a detailed analysis of the issue together with @jwloka. We documented the results here:

https://github.com/jacoco/jacoco/wiki/MavenMultiModule

As part of the analysis we also reviewed the existing PR #97. We came up with a different solution which should cover the described use cases and should be more robust in terms of build configuration and execution.

Please feel free to add comments regarding the general approach as well as the implementation

marchof added some commits Mar 13, 2016

Experimental multi-module support
Initial test setup and stub implementation for multi-module projects,
developed with @jwloka.
@velo

This comment has been minimized.

Show comment
Hide comment
@velo

velo Mar 22, 2016

I would say that having a "report" module is not feasible....

To have a report module, this modules must depend on ALL other modules.

IMHO, there should be an option that would make the reports to be written on the top level project.
OR
Something like deployAtEnd... https://maven.apache.org/plugins/maven-deploy-plugin/deploy-mojo.html#deployAtEnd

BTW, I can help with deployAtEnd'ish behavior if you want me to

velo commented Mar 22, 2016

I would say that having a "report" module is not feasible....

To have a report module, this modules must depend on ALL other modules.

IMHO, there should be an option that would make the reports to be written on the top level project.
OR
Something like deployAtEnd... https://maven.apache.org/plugins/maven-deploy-plugin/deploy-mojo.html#deployAtEnd

BTW, I can help with deployAtEnd'ish behavior if you want me to

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 22, 2016

Member

@velo The report has to be written after all modules have been built and all tests have been executed. If you know how this can be properly implemented in a Maven goal of the top level project please let us know. From our understanding the goal is executed before the sub-modules. See linked document for a detailed discussion.
And no, we don't want extra goals specified on the command line.

Member

marchof commented Mar 22, 2016

@velo The report has to be written after all modules have been built and all tests have been executed. If you know how this can be properly implemented in a Maven goal of the top level project please let us know. From our understanding the goal is executed before the sub-modules. See linked document for a detailed discussion.
And no, we don't want extra goals specified on the command line.

@velo

This comment has been minimized.

Show comment
Hide comment
@velo

velo Mar 22, 2016

not if you enable threads in order to have a speeding build... in that case, modules order are ignored.

velo commented Mar 22, 2016

not if you enable threads in order to have a speeding build... in that case, modules order are ignored.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 22, 2016

Member

@velo Ok, then how do you implement deferred execution of a top level project's goal in a single threaded build?

Member

marchof commented Mar 22, 2016

@velo Ok, then how do you implement deferred execution of a top level project's goal in a single threaded build?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 22, 2016

Member

@velo WTF?!? Do we want to maintain such a hack...? Anyways, many thanks for that reference!

Member

marchof commented Mar 22, 2016

@velo WTF?!? Do we want to maintain such a hack...? Anyways, many thanks for that reference!

@velo

This comment has been minimized.

Show comment
Hide comment
@velo

velo Mar 22, 2016

well, I'm my opinion assuming that projects will be executed at right order is a big problem.

Specially when you add mvn -T to the mix.

velo commented Mar 22, 2016

well, I'm my opinion assuming that projects will be executed at right order is a big problem.

Specially when you add mvn -T to the mix.

@velo

This comment has been minimized.

Show comment
Hide comment
@velo

velo commented Mar 22, 2016

I made this to show what I mean:
https://github.com/jacoco/jacoco/pull/389/files

Merge pull request #389 from velo/patch-1
Test the fix on multi thread environment
<dependencies>
<dependency>
<groupId>jacoco</groupId>
<artifactId>child1</artifactId>

This comment has been minimized.

@velo

velo Mar 22, 2016

this is why multiple threads did not fail....

now, if someone create a child3, child3 may or may not appear on the reports...

unless the same someone remember to add child3 into this dependency list...

@velo

velo Mar 22, 2016

this is why multiple threads did not fail....

now, if someone create a child3, child3 may or may not appear on the reports...

unless the same someone remember to add child3 into this dependency list...

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 22, 2016

Member

@velo Absolutely, this is the advantage and disadvantage of the approach:

  • + explicit dependencies allows to control what should be in the report
  • + proper execution with the Maven live cycle (also for multi-threading)
  • - manual, redundant maintenance of the report scope
Member

marchof commented Mar 22, 2016

@velo Absolutely, this is the advantage and disadvantage of the approach:

  • + explicit dependencies allows to control what should be in the report
  • + proper execution with the Maven live cycle (also for multi-threading)
  • - manual, redundant maintenance of the report scope
@velo

This comment has been minimized.

Show comment
Hide comment
@velo

velo Mar 22, 2016

Also

  • - need for a report module

velo commented Mar 22, 2016

Also

  • - need for a report module
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 22, 2016

Member

child3 may or may not appear on the reports...

This is not true: The report-aggregate goal only adds modules to the reportwhich are declared as a dependency. So if a module is not declared as a dependency it will never show up. If it is declared as a dependency Maven should ensure a proper execution sequence even with multiple threads.

Member

marchof commented Mar 22, 2016

child3 may or may not appear on the reports...

This is not true: The report-aggregate goal only adds modules to the reportwhich are declared as a dependency. So if a module is not declared as a dependency it will never show up. If it is declared as a dependency Maven should ensure a proper execution sequence even with multiple threads.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 23, 2016

Member

@marchof I didn't looked deeper yet, but it seems that this covers case of JaCoCo itself only partially, am I right?

Member

Godin commented Mar 23, 2016

@marchof I didn't looked deeper yet, but it seems that this covers case of JaCoCo itself only partially, am I right?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 23, 2016

Member

@Godin Sure? I think the goal could be directly used in the doc module (or a new one). I'll try to use report-aggregate within our build.

Member

marchof commented Mar 23, 2016

@Godin Sure? I think the goal could be directly used in the doc module (or a new one). I'll try to use report-aggregate within our build.

@velo

This comment has been minimized.

Show comment
Hide comment
@velo

velo Mar 23, 2016

So, if I created something similar to deployAtEnd ( run coverage report on the last module inside reactor and record results onto reactor project would you guys be willing to consider it?

velo commented Mar 23, 2016

So, if I created something similar to deployAtEnd ( run coverage report on the last module inside reactor and record results onto reactor project would you guys be willing to consider it?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 23, 2016

Member

Absolutely! We need to make sure "something" is robust enough to work in most scenarios. For example usage of static tracker in "deployAtEnd" seems very unstable (Maven embedded in other JVM, Multiple aggregate Reports in a more complex module structure etc.)
Maybe someone can get in touch with Maven developers and ask for best practices.

Member

marchof commented Mar 23, 2016

Absolutely! We need to make sure "something" is robust enough to work in most scenarios. For example usage of static tracker in "deployAtEnd" seems very unstable (Maven embedded in other JVM, Multiple aggregate Reports in a more complex module structure etc.)
Maybe someone can get in touch with Maven developers and ask for best practices.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 23, 2016

Member

@marchof I had a talk with @rfscholte at FOSDEM about deployAtEnd and as far as I understood as of today there is no way to do better. Don't know how it plays in case of embedding of Maven.

Member

Godin commented Mar 23, 2016

@marchof I had a talk with @rfscholte at FOSDEM about deployAtEnd and as far as I understood as of today there is no way to do better. Don't know how it plays in case of embedding of Maven.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Mar 23, 2016

Member

@marchof about usage within our build - I made a wrong guess, because didn't found such a change 😉

Member

Godin commented Mar 23, 2016

@marchof about usage within our build - I made a wrong guess, because didn't found such a change 😉

@rfscholte

This comment has been minimized.

Show comment
Hide comment
@rfscholte

rfscholte Mar 23, 2016

I have several proprosals on how to improve the lifecycle handling in Maven. Once https://issues.apache.org/jira/browse/MNG-5666 there's no need for installAtEnd/deployAtEnd anymore and it will work for all Maven projects. installAtEnd/deployAtEnd works for about 90%+ of the projects, it fails for custom packaging types and lifecycle forks because it Maven creates a new classloader, which makes it impossible for the maven-deploy-plugin to detect when the build is really finished. So the real solution means improving Maven core.

rfscholte commented Mar 23, 2016

I have several proprosals on how to improve the lifecycle handling in Maven. Once https://issues.apache.org/jira/browse/MNG-5666 there's no need for installAtEnd/deployAtEnd anymore and it will work for all Maven projects. installAtEnd/deployAtEnd works for about 90%+ of the projects, it fails for custom packaging types and lifecycle forks because it Maven creates a new classloader, which makes it impossible for the maven-deploy-plugin to detect when the build is really finished. So the real solution means improving Maven core.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Mar 23, 2016

Member

works for about 90%+ of the projects

This doesn't sound like we should build a solution on this approach. The 10% of projects will cause lots of support requests here.

Member

marchof commented Mar 23, 2016

works for about 90%+ of the projects

This doesn't sound like we should build a solution on this approach. The 10% of projects will cause lots of support requests here.

public void setReportOutputDirectory(final File reportOutputDirectory) {
if (reportOutputDirectory != null
&& !reportOutputDirectory.getAbsolutePath().endsWith(
"jacoco-aggregate")) {

This comment has been minimized.

@Godin

Godin Apr 17, 2016

Member

@marchof this condition looks weird / hackish for me.

@Godin

Godin Apr 17, 2016

Member

@marchof this condition looks weird / hackish for me.

This comment has been minimized.

@marchof

marchof Apr 17, 2016

Member

@Godin Same for me, I simply took the pattern from the existing report goal.

@marchof

marchof Apr 17, 2016

Member

@Godin Same for me, I simply took the pattern from the existing report goal.

/**
* Footer text used in HTML report pages.
*
* @parameter

This comment has been minimized.

@Godin

Godin Apr 17, 2016

Member

I think would be nice to allow definition of values for title and footer when report mojo executed from command-line.

@Godin

Godin Apr 17, 2016

Member

I think would be nice to allow definition of values for title and footer when report mojo executed from command-line.

This comment has been minimized.

@marchof

marchof Apr 18, 2016

Member

@Godin So you're thinking of something like this?

@parameter property="jacoco.reportTitle"
@parameter property="jacoco.reportFooter"
@marchof

marchof Apr 18, 2016

Member

@Godin So you're thinking of something like this?

@parameter property="jacoco.reportTitle"
@parameter property="jacoco.reportFooter"

This comment has been minimized.

@Godin

Godin Apr 18, 2016

Member

@marchof I withdraw this request for now - let's deal with this separately, since some other important properties are not available for customization from command-line and those properties are not that important.

@Godin

Godin Apr 18, 2016

Member

@marchof I withdraw this request for now - let's deal with this separately, since some other important properties are not available for customization from command-line and those properties are not that important.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Apr 17, 2016

Member

@marchof I'm wondering about following scenario:

  • module A contains class A, but no tests for it
  • module B contains class B, which depends on class A, and test for B indirectly covers A

it is IMO questionable whether aggregate report should show class A as covered or not.

Member

Godin commented Apr 17, 2016

@marchof I'm wondering about following scenario:

  • module A contains class A, but no tests for it
  • module B contains class B, which depends on class A, and test for B indirectly covers A

it is IMO questionable whether aggregate report should show class A as covered or not.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Apr 18, 2016

Member

@Godin This question of "cross module coverage" probably depends on the scenario: If B are integration tests users probably want to see coverage on A. If B is plain unit tests I would prefer to only show code coverage on B. Actually our own build implements this by limiting the instrumentation scope of the agent. This could also be implemented in reporting by loading execution data separately for each bundle. I propose to discuss this requirement in future in a separate issue.

Member

marchof commented Apr 18, 2016

@Godin This question of "cross module coverage" probably depends on the scenario: If B are integration tests users probably want to see coverage on A. If B is plain unit tests I would prefer to only show code coverage on B. Actually our own build implements this by limiting the instrumentation scope of the agent. This could also be implemented in reporting by loading execution data separately for each bundle. I propose to discuss this requirement in future in a separate issue.

* <li><code>test</code>: Only execution data is considered for the report.</li>
* </ul>
*
* @goal report-aggregate

This comment has been minimized.

@Godin

Godin Apr 18, 2016

Member

Question to myself - do we need @requiresDependencyResolution test or @requiresDependencyCollection test or none here?

@Godin

Godin Apr 18, 2016

Member

Question to myself - do we need @requiresDependencyResolution test or @requiresDependencyCollection test or none here?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Apr 18, 2016

Member

@marchof I had some time to carefully study some official Apache Maven reporting plugins, which have aggregation feature (maven-surefire-report-plugin, maven-pmd-plugin, maven-javadoc-plugin), as well as non official plugins (cobertura-maven-plugin, taglist-maven-plugin) - http://blog.the-future-group.com/2014/01/28/maven-aggregate-reports-multi-module-projects/

And seems that finally understood principle - key point is "Inheritance v. Aggregation" ( https://maven.apache.org/pom.html#Inheritance_v ) and in respect to reporting it is pretty well described in http://stackoverflow.com/a/21590120/244993 : if aggregator project is not parent, then it will be the last project in the build, and mentioned above plugins take advantage of this to guarantee availability of required materials for report generation. So that what is described on page https://github.com/jacoco/jacoco/wiki/MavenMultiModule#strategy-aggregator seems to be about situation, when aggregator project is used also as parent project. And indeed in such case his goals should be executed before others to build it, since parent is required for other projects.

To me this looks as complementary feature/solution to the one implemented here, which seems able to address needs and concerns mentioned earlier on this subject by @velo , @johnoliver and others (generation of report for whole project without need to maintain list of dependencies). Maybe @rfscholte , @jwloka , @mfriedenhagen can correct me, if I misunderstood something or if there are some pitfalls (other than combining aggregator and parent) in this approach?

And in any case, if you don't have objections @marchof , I'm going to squash-merge this PR into master since it looks good and meets our needs.

Member

Godin commented Apr 18, 2016

@marchof I had some time to carefully study some official Apache Maven reporting plugins, which have aggregation feature (maven-surefire-report-plugin, maven-pmd-plugin, maven-javadoc-plugin), as well as non official plugins (cobertura-maven-plugin, taglist-maven-plugin) - http://blog.the-future-group.com/2014/01/28/maven-aggregate-reports-multi-module-projects/

And seems that finally understood principle - key point is "Inheritance v. Aggregation" ( https://maven.apache.org/pom.html#Inheritance_v ) and in respect to reporting it is pretty well described in http://stackoverflow.com/a/21590120/244993 : if aggregator project is not parent, then it will be the last project in the build, and mentioned above plugins take advantage of this to guarantee availability of required materials for report generation. So that what is described on page https://github.com/jacoco/jacoco/wiki/MavenMultiModule#strategy-aggregator seems to be about situation, when aggregator project is used also as parent project. And indeed in such case his goals should be executed before others to build it, since parent is required for other projects.

To me this looks as complementary feature/solution to the one implemented here, which seems able to address needs and concerns mentioned earlier on this subject by @velo , @johnoliver and others (generation of report for whole project without need to maintain list of dependencies). Maybe @rfscholte , @jwloka , @mfriedenhagen can correct me, if I misunderstood something or if there are some pitfalls (other than combining aggregator and parent) in this approach?

And in any case, if you don't have objections @marchof , I'm going to squash-merge this PR into master since it looks good and meets our needs.

@Godin Godin assigned Godin and unassigned marchof Apr 18, 2016

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Apr 18, 2016

Member

@Godin Thx for the pointers! I added them to the wiki page.
For now I would prefer to have the report-aggregate goal available. In future we can think about enhancing the report goal.

+1 for squash-merge

Member

marchof commented Apr 18, 2016

@Godin Thx for the pointers! I added them to the wiki page.
For now I would prefer to have the report-aggregate goal available. In future we can think about enhancing the report goal.

+1 for squash-merge

@Godin Godin merged commit c181f60 into master Apr 19, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Godin Godin deleted the issue-388 branch Apr 19, 2016

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Apr 19, 2016

Member

@Godin Thanks!

Member

marchof commented Apr 19, 2016

@Godin Thanks!

@mblub

This comment has been minimized.

Show comment
Hide comment
@mblub

mblub Apr 20, 2016

@marchof @Godin

This is fantastic, worked great for me!

Used 0.7.7-SNAPSHOT as of today (nexus snapshot version 0.7.7-20160419.203207-13, which presumably includes commit c181f60).

Thanks for adding this valuable item. When do you plan to produce a JaCoCo release that includes this commit?

mblub commented Apr 20, 2016

@marchof @Godin

This is fantastic, worked great for me!

Used 0.7.7-SNAPSHOT as of today (nexus snapshot version 0.7.7-20160419.203207-13, which presumably includes commit c181f60).

Thanks for adding this valuable item. When do you plan to produce a JaCoCo release that includes this commit?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Apr 20, 2016

Member

@mblub We do some cleanup and will probably release end of this month.

Member

marchof commented Apr 20, 2016

@mblub We do some cleanup and will probably release end of this month.

@scottcarey

This comment has been minimized.

Show comment
Hide comment
@scottcarey

scottcarey Aug 10, 2016

"If B are integration tests users probably want to see coverage on A. If B is plain unit tests I would prefer to only show code coverage on B."

Note that there are cases where a unit test MUST be in a different module, due to corner case interactions between test scope and non-test scope transitive dependencies.

scottcarey commented Aug 10, 2016

"If B are integration tests users probably want to see coverage on A. If B is plain unit tests I would prefer to only show code coverage on B."

Note that there are cases where a unit test MUST be in a different module, due to corner case interactions between test scope and non-test scope transitive dependencies.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Aug 10, 2016

Member

@scottcarey as you can see in GitHub Web UI on the right hand-side - this ticket is closed and released as part of version 0.7.7. So please direct questions or suggestions about this subject, if there are any, to our mailing list ( https://groups.google.com/forum/#!forum/jacoco ) instead of commenting here.

Thank you for understanding.

Member

Godin commented Aug 10, 2016

@scottcarey as you can see in GitHub Web UI on the right hand-side - this ticket is closed and released as part of version 0.7.7. So please direct questions or suggestions about this subject, if there are any, to our mailing list ( https://groups.google.com/forum/#!forum/jacoco ) instead of commenting here.

Thank you for understanding.

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.