Skip to content

Merge JaCoCo coverage reports the "right" way#3650

Merged
3flex merged 2 commits intomainfrom
jacoco-again
May 21, 2021
Merged

Merge JaCoCo coverage reports the "right" way#3650
3flex merged 2 commits intomainfrom
jacoco-again

Conversation

@3flex
Copy link
Copy Markdown
Member

@3flex 3flex commented Apr 3, 2021

Advantages over the current config are that there's no cross project configuration using subprojects (which is discouraged), and that the generated HTML report now renders properly from the merged JaCoCo task outputs.

Although more advanced/technical than typical Gradle build scripts this technique is actually the recommended way to share outputs between projects.

@3flex 3flex marked this pull request as draft April 3, 2021 13:35
Copy link
Copy Markdown
Member

@picklebento picklebento left a comment

Choose a reason for hiding this comment

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

Advantages over the current config are that there's no cross project configuration using subprojects (which is discouraged), and that the generated HTML report now renders properly.

To paraphrase: If module B's test code covers module A's code, then module A's code would be counted as "covered" in jacoco. Is that correct?

Coverage report on my machine seems slightly improved with this config as well.

Would you mind sharing the jacoco output? (I assume our detekt-gradle-plugin code cannot be correctly calculated still)

Comment thread build.gradle.kts Outdated
jacocoTestReport {
executionData.setFrom(fileTree(project.rootDir.absolutePath).include("**/build/jacoco/*.exec"))
dependencies {
implementation(project("custom-checks"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using the implementation configuration feels weird: Would that create unnecessary tasks by the plugins applied in this build.gradle?

I am wondering if we could use a different configuration, or move the jacoco to a separate module.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moving jacoco generation to a separate project/module makes sense to me. I believe a different configuration would work instead of implementation too. I can make those changes.

Would that create unnecessary tasks by the plugins applied in this build.gradle?

I don't believe so. But if there were unnecessary tasks, and they're not created during the configuration phase, there'll be no penalty to build performance. This is a jacocoTestReport build scan and a codeCoverageReport build scan for comparison. The only tasks created immediately or during configuration are from nexus-staging, binary-compatibility-validator, gradle.plugin-publish, github-release and sonarqube plugins.

The benefit of using configurations like this is it's more declarative so manually setting up task dependencies can be avoided, and only the tasks that need to be run are run. The disadvantage is the boilerplate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can the following block of build script be removed


        subprojects
            .filterNot { it.name in examplesOrTestUtils }
            .forEach {
                this@jacocoTestReport.sourceSets(it.sourceSets.main.get())
                this@jacocoTestReport.dependsOn(it.tasks.test)
            }

I don't know jacoco enough to realize the actual benefit. The boilderplate might be fine since we do not add/change gradle modules often.

@3flex 3flex force-pushed the jacoco-again branch 2 times, most recently from f4f4473 to b0fc185 Compare April 4, 2021 01:16
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 4, 2021

Codecov Report

Merging #3650 (f2ce3a7) into main (4d80329) will decrease coverage by 4.72%.
The diff coverage is n/a.

❗ Current head f2ce3a7 differs from pull request most recent head 0251e66. Consider uploading reports for the commit 0251e66 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3650      +/-   ##
============================================
- Coverage     83.48%   78.75%   -4.73%     
+ Complexity     2914     2900      -14     
============================================
  Files           452      473      +21     
  Lines          8766     9336     +570     
  Branches       1664     1722      +58     
============================================
+ Hits           7318     7353      +35     
- Misses          544     1075     +531     
- Partials        904      908       +4     
Impacted Files Coverage Δ Complexity Δ
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <0.00%> (ø) 3.00% <0.00%> (ø%)
...rbosch/detekt/rules/style/ObjectLiteralToLambda.kt
...kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt 6.25% <0.00%> (ø) 0.00% <0.00%> (?%)
...o/gitlab/arturbosch/detekt/internal/SharedTasks.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...lab/arturbosch/detekt/internal/ClassLoaderCache.kt 85.71% <0.00%> (ø) 0.00% <0.00%> (?%)
.../io/gitlab/arturbosch/detekt/internal/DetektJvm.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...o/gitlab/arturbosch/detekt/invoke/DetektInvoker.kt 40.62% <0.00%> (ø) 0.00% <0.00%> (?%)
.../main/kotlin/io/gitlab/arturbosch/detekt/Detekt.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...itlab/arturbosch/detekt/extensions/DetektReport.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...gitlab/arturbosch/detekt/report/XmlReportMerger.kt 90.90% <0.00%> (ø) 5.00% <0.00%> (?%)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d80329...0251e66. Read the comment docs.

@3flex
Copy link
Copy Markdown
Member Author

3flex commented Apr 4, 2021

To paraphrase: If module B's test code covers module A's code, then module A's code would be counted as "covered" in jacoco. Is that correct?

I believe that's already happening - this change is just about getting the data from each subproject's JaCoCo output, source files and compiled classes in the "right" way. So instead of the merge task reaching into directories manually, the relevant files are packaged by the new configurations (like coverageDataElements) in each project then that configuration is consumed by the new configurations in the root Gradle project. This helps decouple the builds.

Would you mind sharing the jacoco output?

I'm not sure if the line coverage differential in the JaCoCo HTML report run on my machine is actually consequential - now the report is picked up by Codecov you can see there's no difference in coverage reported when using the new task.

@3flex 3flex force-pushed the jacoco-again branch 3 times, most recently from fd38f08 to 1baa3b4 Compare May 10, 2021 08:57
@3flex 3flex marked this pull request as ready for review May 10, 2021 08:57
@3flex 3flex requested a review from picklebento May 10, 2021 22:44
@cortinico cortinico added this to the 1.18.0 milestone May 21, 2021
@cortinico cortinico added the housekeeping Marker for housekeeping tasks and refactorings label May 21, 2021
@3flex 3flex merged commit f5fe1d1 into main May 21, 2021
@3flex 3flex deleted the jacoco-again branch May 21, 2021 10:29
@3flex 3flex mentioned this pull request Oct 2, 2021
@3flex 3flex mentioned this pull request Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

housekeeping Marker for housekeeping tasks and refactorings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants