Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Manage jacoco-maven-plugin #284

wants to merge 1 commit into from

Conversation

kwin
Copy link
Member

@kwin kwin commented Dec 21, 2024

Add dedicated profile "coverage" to generate separate reports for both UTs and ITs.

This closes #283

@kwin kwin force-pushed the feature/jacoco-profile branch from c5059a1 to 5dc5bc7 Compare December 21, 2024 15:18
@@ -517,6 +560,17 @@ under the License.
</build>
</profile>
<!-- END SNIPPET: release-profile -->
<profile>
<id>coverage</id>
Copy link
Member Author

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
@kwin kwin force-pushed the feature/jacoco-profile branch from 5dc5bc7 to 930f7cd Compare December 21, 2024 15:19
</plugin>
</plugins>
</build>
</profile>
Copy link
Member Author

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)?

Copy link
Member

@ctubbsii ctubbsii left a 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.

@slawekjaranowski
Copy link
Member

I agree with @ctubbsii such configuration should be project specific ... no a global one.

@kwin
Copy link
Member Author

kwin commented Jan 2, 2025

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.

@ctubbsii
Copy link
Member

SonarQube is supported and encouraged by the ASF (https://infra.apache.org/build-supported-services.html#sonarcloud),

Supported, yes. Encouraged... I don't know where you get that. INFRA supports integrations with lots of 3rd party tools. It's up to the individual PMC to decide whether to use them for their projects. INFRA tends to be neutral on such matters.

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.

That is one of many optional analysis services of SonarQube. That doesn't make imply any close relation to ASF projects. It's just an option that SonarQube provides. DockerHub is another similar service that INFRA supports, and there exist maven plugins that help facilitate features of DockerHub as well, but that doesn't establish any stronger relationship.

Even if this plugin is required for that particular feature at SonarQube, it doesn't mean that projects want to use SonarQube, or this plugin, or if they do use SonarQube, that they also want to use that particular feature at SonarQube that requires this plugin.

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.

I started looking to find out, and of the first half dozen I looked at before I stopped, I saw that some had jacoco-m-p configured to be disabled, or used custom configuration that would conflict with configuration in the parent POM, and most of the rest were gradle or sbt that wouldn't be affected by this parent POM.

I stopped looking at the rest, though, because it doesn't really matter. This change adds jacoco to all projects that use this parent POM, not just the subset of projects who use this parent POM who also use SonarQube and who also want to use that particular optional feature of SonarQube and who actually look at those reports. It's up to individual projects whether to make use of SonarQube, or jacoco, and these aren't satisfying some universal use case or ASF-requirement.

Overall, I just don't think the argument to add it is all that convincing. This plugin, and the use cases it's supporting seems too niche, and more likely to cause problems than to solve them. So, I still would strongly advocate against adding this.

However, if it is added, then I'd suggest the following changes:

  1. Manage only the version in the pluginManagement section outside the profile
  2. Put the executions in the profile, so it doesn't automatically apply to users who don't activate the profile
  3. Give the profile a more unique name that is less likely to conflict with user's own profile names

But I still don't think it should be added.

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ctubbsii - don't add ...

@kwin
Copy link
Member Author

kwin commented Jan 10, 2025

This whole PR will not add any execution of jacoco-m-p except if activated via profile. But I see that you don’t like it, therefore closing this.

@kwin kwin closed this Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add profile for coverage reports with JaCoCo
3 participants