andygrove opened a new pull request, #6084:
URL: https://github.com/apache/datafusion-comet/pull/6084
## 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 #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
