-
Notifications
You must be signed in to change notification settings - Fork 23
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
Manage jacoco-maven-plugin #284
base: master
Are you sure you want to change the base?
Conversation
c5059a1
to
5dc5bc7
Compare
@@ -517,6 +560,17 @@ under the License. | |||
</build> | |||
</profile> | |||
<!-- END SNIPPET: release-profile --> | |||
<profile> | |||
<id>coverage</id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: allow to activate via property?
Add dedicated profile "coverage" to generate separate reports for both UTs and ITs. This closes #283
5dc5bc7
to
930f7cd
Compare
</plugin> | ||
</plugins> | ||
</build> | ||
</profile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add another profile for merged report (for both UTs and ITs)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really prefer to avoid setting a precedent for adding in 3rd party plugins into various profiles into the apache parent POM. It seems like there'd be no end to it. There's lots of plugins that are useful, or potentially useful, but aren't included in the parent POM... but that doesn't mean they all should be. I myself help maintain a Java code formatter plugin and an import sort order plugin, among a few others, that are quite useful, but I don't think should be included in the Apache parent POM. The line has to be drawn somewhere in order to avoid bloat. I'd prefer to leave this one out. It's easy enough to add this profile to one's own ~/.m2/settings.xml
or to a project's parent POM instead.
I also don't don't think it will be particularly useful to add it to the parent POM, especially since doing so makes it difficult to override a lot of its configuration in order to customize anything and for this plugin, it's quite common to customize it's settings, and not all of its options can be customized with properties. So, adding it here won't actually save any time or effort for many of the people who really depend on this plugin.
Also, the proposed profile name is also generic enough that it could collide with others and from what I've seen, the people who care about this report are people who have already added it to their POM.
On a personal note, jacoco is the only plugin I've removed from projects (because it was broken, usually) where not a single developer has noticed for years.... it's useful in theory, but from what I can tell, almost no developer pays attention to its reports or even notices when they are missing. Maybe that's an argument for making it more ubiquitous, to encourage more developers to consider testing coverage and code quality (testability), but I think there's also a reasonable argument that it's utility is low enough that it doesn't make for a good candidate for the apache parent POM.
I agree with @ctubbsii such configuration should be project specific ... no a global one. |
SonarQube is supported and encouraged by the ASF (https://infra.apache.org/build-supported-services.html#sonarcloud), and part of it is exposing coverage metrics. This part requires jacoco-m-p, so this doesn't make it an arbitrary 3rd party plugin, but one with a close relation to ASF projects. Looking at https://sonarcloud.io/organizations/apache/projects exposes 340 projects (not sure how many leverage Maven and the ASF parent POM), but for me this number qualifies to make this part of the ASF parent POM. |
Add dedicated profile "coverage" to generate separate reports for both UTs and ITs.
This closes #283