Skip to content

chore: remove JaCoCo from the build - #6084

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:remove-jacoco
Sep 21, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
andygrove:remove-jacoco

Conversation

@andygrove

Copy link
Copy Markdown
Member

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@v5 step, 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... to argLine — instrumenting every class loaded in every forked test JVM, both surefire and scalatest — and still runs jacoco:report in the test phase 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, no jacoco:check rule 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 spark module at 59 MB across 2182 files — a 15 MB jacoco.xml plus 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:

  • There is no report-aggregate, so each module reports against its own jacoco.exec. common has zero test sources, so common/target/jacoco.exec is never written and the goal logs Skipping JaCoCo execution due to missing execution data file on every build — while the 6 classes under common/src/main are in fact covered by the 248 test files in spark. Coverage of common reads as nothing.
  • Native Rust code is out of scope for JaCoCo entirely, so even a correct JVM number would describe a minority of the codebase.

The last remaining trace of the original intent is actively misleading: the upload-test-reports input in .github/actions/java-test/action.yaml is still described as "upload test results including coverage to GitHub", but the step it guards globs **/target/surefire-reports/*.txt only. 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?

  • Remove the jacoco-maven-plugin declaration from <build><plugins> and its <pluginManagement> entry, and drop the now-unused jacoco.version property.
  • Drop "including coverage" from the upload-test-reports description in the java-test action, stale since chore: stop uploading code coverage results #3381.

No other change to test JVM arguments: argLine stays -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 the test phase for all four modules and succeeds, with surefire:test and scalatest:test still executing and no jacoco goals bound. Before the change the same command logged argLine set to -javaagent:... for every module and Loading execution data file .../spark/target/jacoco.exec.
  • ./mvnw package -DskipTests — succeeds.
  • grep -ri jacoco over the tree (excluding target/) returns nothing.
  • The CI preflight checks pass: 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 under dev/diffs/, and removing a java agent can only reduce what the test JVMs do.

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.
@github-actions github-actions Bot added the enhancement New feature or request label Sep 21, 2026
@andygrove
andygrove requested a review from comphead September 21, 2026 18:15

@mbutrovich mbutrovich 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.

I think this PR description could have been 99% shorter.

Thanks @andygrove!

@sunchao sunchao left a comment

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.

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.

@andygrove
andygrove added this pull request to the merge queue Sep 21, 2026
Merged via the queue into apache:main with commit ccdff0a Sep 21, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove JaCoCo from the build

3 participants