Add code coverage measurement for unit and integration tests (3.x) - #1018
Draft
nikagra wants to merge 6 commits into
Draft
Add code coverage measurement for unit and integration tests (3.x)#1018nikagra wants to merge 6 commits into
nikagra wants to merge 6 commits into
Conversation
Introduce a `coverage` profile that attaches the JaCoCo agent to the forked
test JVMs. It is opt-in because the agent slows every fork down and all
existing test lanes have to stay able to run without it.
The agent argument goes into a dedicated `jacoco.argline` property rather than
surefire's conventional `argLine`, for two reasons:
- the surefire argLine already carries -Djdk.attach.allowAttachSelf=true, so
prepare-agent would have to preserve it;
- a dedicated property can be declared empty in <properties>, which guarantees
that surefire's late-replaced @{jacoco.argline} always resolves. Without a
declared default, any build that skips prepare-agent -- no profile at all, or
-Djacoco.skip=true -- passes the literal token to the forked JVM, which then
dies with "Unrecognized option".
Because 3.x runs its integration tests as TestNG `short`-group tests through
surefire, this single argLine change instruments the unit lane and every
integration lane.
Failsafe is deliberately left alone. It only runs the tests under
driver-tests/** (OSGi, shading), which are outside the scope of the report; the
OSGi ones additionally load the driver inside their own Pax Exam container,
where an agent on the Failsafe JVM would see little of it.
Refs: DRIVER-892
jacoco:report-aggregate reports on the reactor dependencies of the module it runs in, so producing one report for the whole driver needs a module whose dependencies are exactly the modules worth measuring: driver-core, driver-mapping and driver-extras. driver-examples, driver-tests/** and driver-dist are deliberately out of scope. The module publishes nothing, so it takes the same skip set as driver-tests/pom.xml (clirr, source, javadoc, gpg, install, deploy) and joins the central-publishing excludeArtifacts list. report-aggregate is bound inside the `coverage` profile rather than in the default build. The module is part of the default reactor, so an unconditional binding to `verify` would render a report during every `mvn install`, before any test had run. Refs: DRIVER-892
`COVERAGE=true` on any of the test-* targets turns the profile on, rather than duplicating each recipe into a -coverage variant. It also pulls in a step that removes jacoco.exec first: JaCoCo appends by default, which is what lets one lane accumulate coverage across several forks, but it also means data from an earlier run survives a recompile and classes that changed in between get reported as uncovered because their checksum no longer matches. Only jacoco.exec is removed, so data deliberately renamed out of the way to hold one lane's results while another runs is kept; `make clean-coverage` is the one that removes everything. `make coverage-report` aggregates whatever execution data is on disk with tests skipped, so the same target serves one local lane and execution data collected from several CI jobs. Three things it refuses to do quietly: - it deletes the previous report first, so a failed Maven invocation cannot leave a stale report to be read as this run's result; - it stops before Maven when there is no execution data at all, instead of rendering a report that says 0.00%, which is what JaCoCo does when it loads nothing; - it fails if the finished report has no covered lines, which is the shape a checksum mismatch takes. The percentage comes from the LINE counter in jacoco.xml, not from positional fields of the unquoted CSV. Verified locally on JDK 8 and 11: `make test-unit COVERAGE=true` passes 976 unit tests with the agent attached and `make coverage-report` reports 9033/22602 lines (39.97%) with no checksum mismatches. Running the two-lane recipe from CONTRIBUTING.md gives 1.62% for the second lane alone and 1.85% for both, so the execution data is merged rather than one file winning. Refs: DRIVER-892
The unit lane and all four integration lanes already run the tests worth measuring, so they collect the execution data and one small job aggregates it. Re-running a suite inside a dedicated coverage job would add roughly ninety minutes per pull request and make a known integration flake fail a second job. Each lane flattens its data to one file per module before uploading, because upload-artifact derives the artifact root from the common ancestor of the files it matched: a lane where only one module produced data would otherwise upload a differently shaped artifact. The aggregate job renames each file after the lane it came from and drops it in the module's target directory, which is where jacoco:report-aggregate looks. The job is continue-on-error and runs on !cancelled(): coverage is a metric, and a flaky integration test must not turn it into a second red mark on the pull request. It also overrides MVNCMD to drop -X, which would otherwise produce a log measured in hundreds of megabytes for a build that has nothing to debug. actions/download-artifact is new to this repository, so its pinned SHA has to be added to the Actions allowlist (see CONTRIBUTING.md) before the job can run. Refs: DRIVER-892
Added to the "Running the tests" section of CONTRIBUTING.md, which is where the test instructions already live, rather than to README-dev.md, which is about building the docs. Covers the two things that are not obvious from the commands: what the report's scope is, and that JaCoCo matches execution data to classes by checksum, which is what makes stale data show up as uncovered code. Refs: DRIVER-892
testing/bin/coverage is a Python 2 script (it still uses print statements) that drives `mvn cobertura:cobertura` and rsyncs the result to a server whose address was never filled in. The Cobertura plugin is not configured anywhere in the build, so the script could not have worked for years. Its README duplicates what CONTRIBUTING.md already says about CCM and loopback aliases, and the directory was in .gitignore, so nothing new could be added to it anyway. Removing both leaves one answer to "how do I get coverage". Refs: DRIVER-892
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds opt-in JaCoCo coverage collection and aggregation across unit and integration test lanes.
Changes:
- Adds JaCoCo Maven configuration and an aggregate report module.
- Extends Make and CI test lanes to collect and merge coverage.
- Replaces obsolete Cobertura tooling and documentation.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pom.xml |
Configures JaCoCo and registers the report module. |
driver-coverage-report/pom.xml |
Generates aggregate HTML, XML, and CSV reports. |
Makefile |
Adds coverage-enabled test and reporting targets. |
.github/workflows/tests@v1.yml |
Collects, merges, and publishes CI coverage. |
CONTRIBUTING.md |
Documents the coverage workflow. |
testing/README.md |
Removes obsolete Cobertura documentation. |
testing/bin/coverage |
Removes the legacy Cobertura utility. |
.gitignore |
Removes obsolete testing exclusions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| ``` | ||
| make test-unit COVERAGE=true | ||
| mv driver-core/target/jacoco.exec driver-core/target/jacoco-unit.exec |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds JaCoCo code coverage to the 3.x driver: an opt-in Maven profile, an aggregate module, a
COVERAGE=trueknob on the Make test targets, and a CI job that turns the execution data theexisting lanes produce into a single number.
Sibling of #1005 (DRIVER-891, 4.x). It deliberately does not copy that PR's structure, because
three things in it would misbehave here:
@{argLine}with no declared default, so-Djacoco.skip=truepasses a literal token to the forked JVMjacoco.arglineproperty, declared empty in<properties>, so@{jacoco.argline}always resolvesreport-aggregatebound toverifyin a default-reactor module, rendering a report on everymvn installcoverageprofile; with the profile off the module is an inert pomWhat 3.x makes easier
3.x has no unit/IT split by plugin: integration tests are TestNG
short-group tests run bySurefire, and Failsafe's default
**/*IT.javaincludes match nothing indriver-core. So thesingle
argLinechange in the parent pom instruments the unit lane and all four integrationlanes. There is one
argLinein the whole tree to touch.Failsafe is left uninstrumented on purpose: it only runs the tests under
driver-tests/**(OSGi,shading), which are outside the report's scope, and the OSGi ones load the driver inside their own
Pax Exam container.
Scope of the report
driver-core,driver-mapping,driver-extras— the dependencies of the newdriver-coverage-reportmodule.driver-examples,driver-tests/**anddriver-distare out.The module publishes nothing: same skip set as
driver-tests/pom.xml, plus theexcludeArtifactslist.Verification
Run locally on both JDK 8 and JDK 11 (the versions the lanes use):
make test-unit COVERAGE=truepasses 976 unit tests with the agent attached;make coverage-reportreports 9033/22602 lines (39.97%) with no checksum mismatches.mvn test -pl driver-core -Djacoco.skip=truestarts its test JVM — the Add code coverage measurement for unit and integration tests #1005 failure mode,reproduced as a negative test.
mvn verify -DskipTestson the default reactor renders no report and logs zero jacoco goals;make checkpasses on 8 and 11.so execution data is merged rather than one file winning.
make coverage-reportwith no execution data present fails instead of reporting 0.00%, whichis what JaCoCo renders when it loads nothing.
CI evidence (run 32779140252)
Every lane green, and the aggregate job reports 18728/22509 lines (83.20%) with zero
checksum mismatches, so execution data recorded on one runner matches classes compiled on another.
All 15 execution data files (5 lanes x 3 modules) were loaded. The aggregate job itself takes 44s.
Cost of the agent, against the uninstrumented
scylla-3.xpush run 32052641398:timeout-minutesThe integration lanes finish at roughly a third of their budget, so no timeout needs raising. The
Cassandra lane came in faster than baseline: runner variance dominates the agent's cost there.
actions/download-artifactturned out to need no allowlist entry: the download step succeeded onthis run.
Deliberately not in scope
jacoco:checkthreshold: the epic asks for a metric, and a ratchet on abranch that is being deprecated (DRIVER-483) would mostly produce false failures.
timeout-minutes: 90. If this run shows one landing close to the cap, raising it should be itsown commit with the measured numbers.
tests@v1.ymlhas noconcurrency:group, so a superseded push keeps running a full ITsuite. Pre-existing, affects every lane, belongs in its own PR. Same on 4.x.
tests-reports@v1.ymllistens forworkflows: ['Tests']while this workflow is namedTests (Driver 3.x), so it looks like it never fires here. Unrelated, worth its own issue.driver-tests/shading'sNettyUtilITnever runs: Failsafe picks it up atverify, but it isannotated
groups = "unit"andverifyis only ever invoked with-Pshort. Noticed whilescoping the report; not touched.
Refs: DRIVER-892