Aggregate Report Maven Goal

[marchof]

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

[velo]

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]

@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]

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

[marchof]

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

[velo]

Something like this:
https://github.com/apache/maven-plugins/blob/trunk/maven-deploy-plugin/src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java#L155

[marchof]

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

[velo]

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]

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

[velo]

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]

@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]

Also

• - need for a report module

[marchof]

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]

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

[marchof]

@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]

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]

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]

@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]

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

[rfscholte]

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]

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.

[Godin]

@marchof this condition looks weird / hackish for me.

[marchof]

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

[Godin]

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

[marchof]

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

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

[Godin]

@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]

@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]

@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.

[Godin]

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

[Godin]

@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.

[marchof]

@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

[marchof]

@Godin Thanks!

[mblub]

@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]

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

[scottcarey]

"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]

@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.