chore: remove JaCoCo from the build - #6084
Conversation
JaCoCo was added in apache#163 to publish JVM coverage to Codecov. apache#3381 removed the codecov upload step, which was the only consumer, but left the plugin in place. Since then every build has instrumented every forked test JVM and generated a report that nothing reads: no CI step uploads it, no check rule gates on it, and no path in the repo references it. It was never configured to measure much either. There is no report-aggregate, so each module reports against its own exec file; common has no test sources of its own, so its report is always skipped even though its classes are exercised by the spark module's tests. Also drop "including coverage" from the upload-test-reports description in the java-test action, which has been stale since apache#3381 -- the step it guards uploads surefire .txt reports only.
mbutrovich
left a comment
There was a problem hiding this comment.
I think this PR description could have been 99% shorter.
Thanks @andygrove!
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed 478d57a359943b284e40a2796c755e6dfe1918d1 against 5ca149928f7743bfe7a96feadea5e0f9bed1412f. I found no new or remaining P1/P2 issue.
The root POM previously attached JaCoCo's agent and generated reports during the Maven lifecycle. Removing the version property, managed plugin and active plugin removes those operations. Parsing both POMs and removing exactly those three nodes from the base produces the same remaining XML structure as the head. The test JVM arguments, ScalaTest/Surefire configuration, profiles and module inheritance remain intact. Failsafe retains its separate explicit arguments. No unresolved agent-dependent argLine placeholder remains.
Repository-wide searches found no remaining JaCoCo, Codecov or coverage-file consumer. The upload action still uploads Surefire text reports. Its description now matches its actual behavior. This deliberately removes local Java coverage collection and reports without removing an existing repository coverage threshold or upload gate.
There are no expression, operator, null, overflow, error or fallback changes. The maintained Spark 3.5 and 4.0 branches define SBT test JVM options independently in project/SparkBuild.scala. Comet's Spark SQL workflow continues to build Comet with Maven and run Spark's separate SBT build. Maintained 3.4/4.1 sources were unavailable for this review, so I do not claim source qualification for those versions.
Validation
36 focused source/XML/config checks passed. I inspected the successful Spark 4.1 build, expressions and exec logs. All checked out 21bc98cf1e589daccfa163b08618e63d9cfb65fb, whose parents are the reviewed base and head and whose complete tree equals the head.
The two test jobs completed 1,518 and 974 Scala tests with zero failures, plus successful Java and integration tests. Expressions reported one canceled and 12 ignored Scala tests. Exec reported five ignored. Both uploaded six test-report files. The current check snapshot has 24 successes and 14 skips. Skipped Spark SQL, macOS and other optional jobs are not runtime qualification. No local Maven/SBT build or full suite was run.
Performance
Removing prepare-agent removes JaCoCo's agent attachment step, and removing report removes report-generation work. The inspected CI logs contain no JaCoCo goal executions. This supports the mechanism for lower test overhead, but no matched timing comparison was performed, and the author's local report-size measurement was not independently reproduced. There is no change to a production query execution path.
Design
Deleting the unused integration is a direct solution to the stated problem. Removing both the active plugin and its managed version avoids leaving partial configuration behind. Keeping test selection, result uploads and JVM options intact preserves the existing build contract. I found no required consumer that would justify retaining a separate coverage profile in this PR.
Abstraction & complexity
The change introduces no new abstraction or control flow. It reduces inherited Maven lifecycle configuration and corrects one input description. The surrounding build and CI commands remain unchanged, which keeps the effect easy to trace through the Maven and Spark SBT workflows.
Which issue does this PR close?
Closes #6083.
Rationale for this change
JaCoCo was added in #163 to publish JVM coverage to Codecov. #3381 removed the
codecov/codecov-action@v5step, which was the only consumer of the output, but left the plugin itself in place.So every Maven build still prepends
-javaagent:org.jacoco.agent...toargLine— instrumenting every class loaded in every forked test JVM, both surefire and scalatest — and still runsjacoco:reportin thetestphase for all four modules. Nothing reads the result. It lands in<module>/target/site/jacoco/and<module>/target/jacoco.exec; no CI step uploads it, nojacoco:checkrule gates on it, and no path anywhere in the repo references it.It is not free. On a local run in which zero tests executed, the report was still generated for the
sparkmodule at 59 MB across 2182 files — a 15 MBjacoco.xmlplus the full HTML site — and that happens on every job in the PR matrix.It also was not configured to measure anything useful even for someone who went looking for the files:
report-aggregate, so each module reports against its ownjacoco.exec.commonhas zero test sources, socommon/target/jacoco.execis never written and the goal logsSkipping JaCoCo execution due to missing execution data fileon every build — while the 6 classes undercommon/src/mainare in fact covered by the 248 test files inspark. Coverage ofcommonreads as nothing.The last remaining trace of the original intent is actively misleading: the
upload-test-reportsinput in.github/actions/java-test/action.yamlis still described as "upload test results including coverage to GitHub", but the step it guards globs**/target/surefire-reports/*.txtonly. The one job that opts in (pr_build_linux.yml) has therefore been advertising a coverage upload that has not happened since February.If we want JVM coverage again, it should come back with a consumer attached — an aggregate report plus an upload or a threshold — rather than as a directory nobody opens.
What changes are included in this PR?
jacoco-maven-plugindeclaration from<build><plugins>and its<pluginManagement>entry, and drop the now-unusedjacoco.versionproperty.upload-test-reportsdescription in thejava-testaction, stale since chore: stop uploading code coverage results #3381.No other change to test JVM arguments:
argLinestays-ea -Xmx4g -Xss4m ${extraJavaTestArgs}, just without the agent prepended.How are these changes tested?
Build-only change, verified locally against the default profile:
./mvnw test -Dtest=TestCometS3CredentialProvider -DwildcardSuites=<no match>— reactor reaches thetestphase for all four modules and succeeds, withsurefire:testandscalatest:teststill executing and no jacoco goals bound. Before the change the same command loggedargLine set to -javaagent:...for every module andLoading execution data file .../spark/target/jacoco.exec../mvnw package -DskipTests— succeeds.grep -ri jacocoover the tree (excludingtarget/) returns nothing.dev/ci/check-ci-config.py,check-suites.py,check-benchmark-runner.py,test-iceberg-shards.py.No
run-*label applied: this touches no serde, planner, native operator, Spark shim, Iceberg path, or anything underdev/diffs/, and removing a java agent can only reduce what the test JVMs do.