Skip to content

[fix][ci] Fix Jacoco code coverage to report classes in dependent projects - #19264

Merged
lhotari merged 6 commits into
apache:masterfrom
lhotari:lh-fix-jacoco-coverage-multi-project
Jan 20, 2023
Merged

[fix][ci] Fix Jacoco code coverage to report classes in dependent projects#19264
lhotari merged 6 commits into
apache:masterfrom
lhotari:lh-fix-jacoco-coverage-multi-project

Conversation

@lhotari

@lhotari lhotari commented Jan 17, 2023

Copy link
Copy Markdown
Member

Motivation

Currently, jacoco code coverage doesn't get reported for classes that reside in a project other than the test class.

Additional context

Jacoco includes another goal "report-aggregate" which correctly handles multi-project builds.
Usage example: https://github.com/jacoco/jacoco/blob/master/jacoco-maven-plugin.test/it/it-report-aggregate-customization/report/pom.xml

The first attempt was to use the jacoco:report-aggregate goal:
It has 2 issues:
- 0.8.8 version doesn't yet support the required "includeCurrentProject" feature.
- the dependent projects must be built as part of the same mvn execution and belong to the same maven "reactor"
- this isn't compatible with the way how Pulsar CI builds in "Build and License check" job and reuses
the build results to run unit tests.

In addition, there's an issue with the stability of Codecov backend. It frequently fails with errors related to GitHub API quota limits. Therefore, coverage should be collected, aggregated and published to Codecov in a separate build job that can be retried.

An additional benefit of creating a single aggregate report is the reduced amount of coverage data published to Codecov. When all source code is considered, that coverage file is 22MB for the Pulsar repository. Instead of sending for all unit tests separately, 9x22MB, it will send only once up to about 22MB for all unit tests.

Modifications

  • add bash scripts that:
    • collect all required files needed for aggregate reporting in a separate build phase
      • files are uploaded as build artifacts to be used in the later build phase
    • use the Jacoco command line tool to do the aggregate reporting in the correct way.
  • make GitHub Actions workflow changes for the separate build phase for Coverage upload for unit tests, integration tests and system tests.
  • Publish Jacoco xml, csv and html report as build artifact

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari

lhotari commented Jan 17, 2023

Copy link
Copy Markdown
Member Author

@yaalsn Please review this PR.

@lhotari lhotari changed the title [fix][ci] Fix Jacoco code coverage to report classes in dependent project [fix][ci] Fix Jacoco code coverage to report classes in dependent projects Jan 17, 2023
@lhotari
lhotari requested a review from Technoboy- January 17, 2023 18:03
@lhotari
lhotari marked this pull request as draft January 17, 2023 21:11
@lhotari

lhotari commented Jan 17, 2023

Copy link
Copy Markdown
Member Author

I made this a draft until I have properly tested this change and fixed the remaining issue.

@lhotari

lhotari commented Jan 18, 2023

Copy link
Copy Markdown
Member Author

It turns out that the default jacoco maven plugin resolves only dependent projects that are part of the "Maven reactor".

https://github.com/jacoco/jacoco/blob/v0.8.8/jacoco-maven-plugin/src/org/jacoco/maven/ReportAggregateMojo.java#L164-L177

The dependent projects aren't part of the "Maven reactor" in the current way how we build projects once in the "Build and License check" job and then run only the tests.
And someone claims that Maven is easier than Gradle. These are the times when I just wish we were using Gradle.

Another issue is that the "includeCurrentProject" feature in the Jacoco maven plugin hasn't been released yet. It will be part of 0.8.9 version, jacoco/jacoco#1007 . A document annotation was missing and that caused it to show 0.7.7 version in the docs as the version where this feature is supported.

Since the Jacoco maven plugin won't be able to create a proper report of the coverage, I'll solve this by skipping the Jacoco report generation using the Maven plugin altogether and write a bash script to handle this case.

@lhotari
lhotari force-pushed the lh-fix-jacoco-coverage-multi-project branch from 8c1d3fe to 040c26c Compare January 18, 2023 10:06
@lhotari
lhotari force-pushed the lh-fix-jacoco-coverage-multi-project branch 7 times, most recently from bc52169 to a562b94 Compare January 19, 2023 06:16
@lhotari
lhotari marked this pull request as ready for review January 19, 2023 08:16
@lhotari

lhotari commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great stuff !
I have never been able to make it work

+1

@lhotari
lhotari force-pushed the lh-fix-jacoco-coverage-multi-project branch 3 times, most recently from 51c3909 to f57fb5d Compare January 19, 2023 10:25
@lhotari

lhotari commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

@yaalsn This is a PR which will significantly improve the reliability of Codecov metrics. It solves the coverage reporting for dependent projects, but also moves uploading of coverage to Codecov to a separate build job which can be retried. It is also more likely that the master branch coverage will get consistently reported with this solution.
In addition, the Jacoco HTML report will be available. This can help validate Codecov results. There will be separate HTML reports for unit tests, integration tests and system tests.

@Jason918 Jason918 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work!

@lhotari
lhotari force-pushed the lh-fix-jacoco-coverage-multi-project branch 3 times, most recently from 41f8507 to 8c5c71f Compare January 19, 2023 12:40
@lhotari
lhotari force-pushed the lh-fix-jacoco-coverage-multi-project branch from 8c5c71f to eb2db5b Compare January 19, 2023 12:41
Error: Exception in thread "main" java.io.IOException: Error while analyzing /home/runner/.m2/repository/org/apache/pulsar/pulsar-io-kafka/2.12.0-SNAPSHOT/pulsar-io-kafka-2.12.0-SNAPSHOT.jar@META-INF/bundled-dependencies/bcprov-ext-jdk15on-1.69.jar@META-INF/versions/15/org/bouncycastle/jcajce/provider/asymmetric/edec/KeyFactorySpi$Ed25519.class with JaCoCo 0.8.8.202204050719/5dcf34a.
Caused by: java.lang.IllegalStateException: Can't add different class with same name: org/bouncycastle/jcajce/provider/asymmetric/edec/KeyFactorySpi$Ed25519
- fail faster when the backend doesn't respond
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants