From f332af7d2bfa14f76d5199bd08ac9c9e9dfbc0ca Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 20:15:11 +0000 Subject: [PATCH 1/6] build: add opt-in JaCoCo coverage measurement jacoco-maven-plugin was declared in the parent pom but produced nothing usable, for two reasons. First, coverage was silently not collected for core or integration-tests. Both set to just their own JVM flags (${mockitoopens.argline}, ${blockhound.argline}), replacing rather than combining with the value jacoco:prepare-agent injects into that same property, so the -javaagent flag never reached the forked test JVM. They now combine both with Maven's deferred-property syntax, @{argLine} being required over ${argLine} because prepare-agent sets the property at build-execution time. Both argLine and blockhound.argline are declared empty in the root pom: a composite value referencing an undeclared property keeps the literal "@{...}" text, which the forked JVM rejects as an option, and blockhound.argline is only set from JDK 14 onwards. Second, nothing merged the per-module execution data into a cross-module view. Coverage that core gets *through* the integration suite was never attributed back to core's own source, because each module's own report only knows its own classes. A new coverage-report module aggregates over core, query-builder, the mapper and metrics modules and integration-tests. scope=compile is set explicitly on four of those because the root pom's dependencyManagement pins them to scope=test, and report-aggregate only aggregates compile/runtime-scoped reactor dependencies. Instrumentation is opt-in through a "coverage" profile rather than bound unconditionally: the agent slows every forked test JVM down, and the existing test lanes have to stay able to run without it. report-aggregate is bound inside that profile too, so the default reactor renders nothing during a plain `mvn install`. Pass COVERAGE=true to any test-* Make target to enable it, then `make coverage-report` to aggregate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NxW9fzmwwpSLvzcHEr5MRa --- Makefile | 82 +++++++++++++++++-- core/pom.xml | 2 +- coverage-report/pom.xml | 164 ++++++++++++++++++++++++++++++++++++++ integration-tests/pom.xml | 6 +- pom.xml | 84 ++++++++++++++----- 5 files changed, 309 insertions(+), 29 deletions(-) create mode 100644 coverage-report/pom.xml diff --git a/Makefile b/Makefile index b41799423d6..0d187c4e839 100644 --- a/Makefile +++ b/Makefile @@ -24,6 +24,20 @@ MAVEN_OPTS ?= RELEASE_SKIP_TESTS ?= +# Set COVERAGE=true on any of the test-* targets to attach the JaCoCo agent to +# the forked test JVMs; `make coverage-report` then aggregates whatever +# execution data is on disk. Off by default: the agent slows every fork down, +# and the existing test lanes have to stay able to run without it. +COVERAGE ?= false +ifeq ($(filter true 1,$(COVERAGE)),) + MVN_COVERAGE := + COVERAGE_PREREQ := +else + MVN_COVERAGE := -Pcoverage + COVERAGE_PREREQ := .clean-coverage-data +endif +COVERAGE_REPORT_DIR := coverage-report/target/site/jacoco-aggregate + ifeq (${CCM_CONFIG_DIR},) CCM_CONFIG_DIR = ~/.ccm endif @@ -33,6 +47,18 @@ export SCYLLA_EXT_OPTS export SCYLLA_VERSION export PATH := $(MAKEFILE_PATH)/bin:$(PATH) +# JaCoCo appends to its execution data by default, which is what lets one lane +# accumulate coverage across several forks (integration-tests alone runs three). +# The flip side is that data from an earlier run survives a recompile, and a +# class that changed in between is then reported uncovered because its checksum +# no longer matches. Truncating before a run is the fix. +# +# Only jacoco.exec is removed -- the file the agent is about to write. Data +# renamed out of the way to keep one lane's results while another runs (as the +# CI coverage job does) is left alone. +.clean-coverage-data: + @find . -name 'jacoco.exec' -delete + .install-guava-shaded: $(MVNCMD) install -pl guava-shaded @@ -290,10 +316,10 @@ check: fix: $(MVNCMD) fmt:format xml-format:xml-format -test-unit: .install-guava-shaded - $(MVNCMD) test -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true +test-unit: .install-guava-shaded $(COVERAGE_PREREQ) + $(MVNCMD) test $(MVN_COVERAGE) -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true -test-integration-scylla: .install-all-modules .prepare-scylla-ccm resolve-scylla-version .prepare-environment-update-aio-max-nr +test-integration-scylla: .install-all-modules .prepare-scylla-ccm resolve-scylla-version .prepare-environment-update-aio-max-nr $(COVERAGE_PREREQ) @if [[ -z "$${SCYLLA_VERSION_RESOLVED}" ]]; then SCYLLA_VERSION_RESOLVED=`cat '${SCYLLA_VERSION_FILE}'` fi @@ -301,9 +327,9 @@ test-integration-scylla: .install-all-modules .prepare-scylla-ccm resolve-scylla echo "ScyllaDB version ${SCYLLA_VERSION} was not resolved" exit 1 fi - mvn -B -e verify -pl integration-tests -Dccm.version=$${SCYLLA_VERSION_RESOLVED} -Dccm.distribution=scylla -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true $(MAVEN_EXTRA_ARGS) + mvn -B -e verify $(MVN_COVERAGE) -pl integration-tests -Dccm.version=$${SCYLLA_VERSION_RESOLVED} -Dccm.distribution=scylla -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true $(MAVEN_EXTRA_ARGS) -test-integration-cassandra: .install-all-modules .prepare-scylla-ccm resolve-cassandra-version +test-integration-cassandra: .install-all-modules .prepare-scylla-ccm resolve-cassandra-version $(COVERAGE_PREREQ) @if [[ -z "$${CASSANDRA_VERSION_RESOLVED}" ]]; then CASSANDRA_VERSION_RESOLVED=`cat '${CASSANDRA_VERSION_FILE}'` fi @@ -311,7 +337,51 @@ test-integration-cassandra: .install-all-modules .prepare-scylla-ccm resolve-cas echo "Cassandra version ${CASSANDRA_VERSION} was not resolved" exit 1 fi - mvn -B -e verify -pl integration-tests -Dccm.version=$${CASSANDRA_VERSION_RESOLVED} -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true $(MAVEN_EXTRA_ARGS) + mvn -B -e verify $(MVN_COVERAGE) -pl integration-tests -Dccm.version=$${CASSANDRA_VERSION_RESOLVED} -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true $(MAVEN_EXTRA_ARGS) + +# Aggregates the execution data left behind by any COVERAGE=true test run into +# a single cross-module report -- most importantly attributing the coverage +# core gets *through* the integration suite back to core's own source, which +# each module's own report cannot see. Tests are skipped here on purpose: this +# only reads what is already on disk, so the same target serves one local lane +# and execution data collected from several CI jobs. +# +# report-aggregate is bound to `verify` inside the "coverage" profile (see +# coverage-report/pom.xml) rather than requested as a bare CLI goal: a CLI goal +# runs against every project the -am reactor pulls in, which rendered a stray +# report in all eleven of them (one of those over guava-shaded's relocated +# classes), and it never sees the execution's own configuration. Keeping the +# binding inside the profile still leaves a plain `mvn verify`/`mvn install` +# rendering nothing. +# +# .PHONY here (unlike the rest of this file) because these target names +# collide with real paths -- coverage-report/ is the module's own directory -- +# so make would otherwise treat the target as already up to date and skip it. +.PHONY: coverage-report clean-coverage +coverage-report: .install-guava-shaded + @if [[ -z "$$(find . -name 'jacoco*.exec' -not -path './coverage-report/*' -print -quit)" ]]; then + echo 'No JaCoCo execution data found.' + echo "Run the tests with COVERAGE=true first, e.g. 'make test-unit COVERAGE=true'." + exit 1 + fi + rm -rf '${COVERAGE_REPORT_DIR}' + $(MVNCMD) verify -Pcoverage -pl coverage-report -am -DskipTests -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true + if [[ ! -f '${COVERAGE_REPORT_DIR}/jacoco.xml' ]]; then + echo 'Maven produced no report at ${COVERAGE_REPORT_DIR}/jacoco.xml.' + exit 1 + fi + echo 'HTML report: ${COVERAGE_REPORT_DIR}/index.html' + # Read the report-level LINE counter rather than the sibling csv, whose + # fields are unquoted and so shift on any class name containing a comma. + # Zero covered lines means the execution data did not match these classes + # (look for a checksum mismatch in the log), which is worth failing on: + # the alternative is a confident-looking 0%. + python3 -c 'import sys, xml.etree.ElementTree as ET; r = ET.parse(sys.argv[1]).getroot(); c = next(x for x in r.findall("counter") if x.get("type") == "LINE"); missed, covered = int(c.get("missed")), int(c.get("covered")); total = missed + covered; print("Line coverage: {}/{} ({:.2f}%)".format(covered, total, 100.0 * covered / total if total else 0.0)); sys.exit("No lines are recorded as covered: the execution data is either missing or does not match these classes. Look for a checksum mismatch warning in the Maven log.") if covered == 0 else None' '${COVERAGE_REPORT_DIR}/jacoco.xml' | tee -a "$${GITHUB_STEP_SUMMARY:-/dev/null}" + +clean-coverage: + find . -name 'jacoco*.exec' -delete + find . -type d -path '*/target/site/jacoco*' -exec rm -rf {} + + rm -rf coverage-report/target/site check-no-compile-warnings: @$(MAKE) compile-all | grep WARNING >/tmp/all-compile-warnings.log || true diff --git a/core/pom.xml b/core/pom.xml index 45f2ee64cf6..136fcba0420 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -261,7 +261,7 @@ maven-surefire-plugin ${testing.jvm}/bin/java - ${mockitoopens.argline} + @{argLine} ${mockitoopens.argline} 1 diff --git a/coverage-report/pom.xml b/coverage-report/pom.xml new file mode 100644 index 00000000000..5fc8e79fb75 --- /dev/null +++ b/coverage-report/pom.xml @@ -0,0 +1,164 @@ + + + + + 4.0.0 + + com.scylladb + java-driver-parent + 4.19.2.2-SNAPSHOT + + java-driver-coverage-report + pom + Java driver for Scylla and Apache Cassandra(R) - coverage report + + + + com.scylladb + java-driver-core + + + com.scylladb + java-driver-query-builder + + + com.scylladb + java-driver-mapper-runtime + compile + + + com.scylladb + java-driver-mapper-processor + compile + + + com.scylladb + java-driver-metrics-micrometer + compile + + + com.scylladb + java-driver-metrics-microprofile + compile + + + com.scylladb + java-driver-integration-tests + ${project.version} + + + + + + org.jacoco + jacoco-maven-plugin + + + + default + none + + + report + none + + + + + maven-install-plugin + + true + + + + maven-deploy-plugin + + true + + + + + + + + coverage + + + + org.jacoco + jacoco-maven-plugin + + + report-aggregate + verify + + report-aggregate + + + Java Driver for Scylla and Apache Cassandra 4.x + + HTML + XML + CSV + + ${project.build.directory}/site/jacoco-aggregate + + + + + + + + + diff --git a/integration-tests/pom.xml b/integration-tests/pom.xml index 7f481242fce..dd05e343872 100644 --- a/integration-tests/pom.xml +++ b/integration-tests/pom.xml @@ -283,7 +283,7 @@ ${test.parallel.threads} ${project.build.directory}/failsafe-reports/failsafe-summary-parallelized.xml ${skipParallelizableITs} - ${blockhound.argline} + @{argLine} ${blockhound.argline} ${testing.jvm}/bin/java @@ -296,7 +296,7 @@ com.datastax.oss.driver.categories.ParallelizableTests, com.datastax.oss.driver.categories.IsolatedTests ${project.build.directory}/failsafe-reports/failsafe-summary-serial.xml ${skipSerialITs} - ${blockhound.argline} + @{argLine} ${blockhound.argline} ${testing.jvm}/bin/java @@ -312,7 +312,7 @@ false ${project.build.directory}/failsafe-reports/failsafe-summary-isolated.xml ${skipIsolatedITs} - ${blockhound.argline} + @{argLine} ${blockhound.argline} ${testing.jvm}/bin/java diff --git a/pom.xml b/pom.xml index 76caea447cc..ca2471cf70e 100644 --- a/pom.xml +++ b/pom.xml @@ -50,10 +50,20 @@ distribution-tests examples bom + coverage-report UTF-8 UTF-8 + + 1.4.8 2.2.2 @@ -101,6 +111,17 @@ false false + + false @@ -712,7 +733,7 @@ true central - java-driver-distribution-source,java-driver-distribution-tests,java-driver-distribution,java-driver-examples,java-driver-integration-tests,java-driver-osgi-tests + java-driver-distribution-source,java-driver-distribution-tests,java-driver-distribution,java-driver-examples,java-driver-integration-tests,java-driver-osgi-tests,java-driver-coverage-report ${release.autopublish} validated @@ -770,24 +791,16 @@ - - org.jacoco - jacoco-maven-plugin - - - - prepare-agent - - - - report - prepare-package - - report - - - - + maven-surefire-plugin @@ -1040,6 +1053,39 @@ height="0" width="0" style="display:none;visibility:hidden"> + + + coverage + + + + org.jacoco + jacoco-maven-plugin + + + + prepare-agent + + + + report + prepare-package + + report + + + + + + + fast From a39ca8047de659186e5fa5e05186ac9824a02708 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 20:23:03 +0000 Subject: [PATCH 2/6] ci: report code coverage from the existing test lanes Measures coverage without running any suite twice: the unit and integration lanes that already run the tests do so with COVERAGE=true and upload their execution data, and one short job aggregates it. A dedicated workflow would have re-run the Scylla LATEST/17 suite that the "Scylla ITs" lanes already cover, and would have given a known integration flake a second job to redden. Execution data is flattened to one file per module on upload, so the artifact layout does not depend on which modules produced data, and is placed back under a per-lane name on download, since report-aggregate picks up every *.exec in a module's target directory. The aggregating job is continue-on-error and runs under !cancelled(): a failing lane still uploads whatever it recorded, so partial coverage is reported rather than lost, and the metric never becomes a second failure on the pull request. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NxW9fzmwwpSLvzcHEr5MRa --- .github/workflows/tests@v1.yml | 142 +++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/.github/workflows/tests@v1.yml b/.github/workflows/tests@v1.yml index 68da13e9823..dd3edfd292e 100644 --- a/.github/workflows/tests@v1.yml +++ b/.github/workflows/tests@v1.yml @@ -133,8 +133,32 @@ jobs: key: ${{ runner.os }}-${{ matrix.java-version }}-maven-${{ hashFiles('**/pom.xml') }} - name: Run unit tests + env: + COVERAGE: "true" run: make test-unit + # Flattened to one file per module so the artifact layout does not depend + # on how many modules happened to produce data, and named per lane so the + # aggregating job can keep each lane's contribution apart. + - name: Collect coverage execution data + if: ${{ !cancelled() }} + run: | + shopt -s nullglob + mkdir -p coverage-exec + for exec_file in */target/jacoco.exec metrics/*/target/jacoco.exec; do + module="${exec_file%/target/jacoco.exec}" + cp "$exec_file" "coverage-exec/${module//\//-}.exec" + done + ls -l coverage-exec + + - name: Upload coverage execution data + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + if: ${{ !cancelled() }} + with: + name: coverage-exec-unit-${{ matrix.java-version }} + path: coverage-exec/ + if-no-files-found: warn + - name: Upload test results uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 if: always() @@ -256,8 +280,28 @@ jobs: GET_VERSION_VERSION: 0.4.5 GH_TOKEN: ${{ github.token }} MAVEN_EXTRA_ARGS: ${{ steps.test-skip-args.outputs.value }} + COVERAGE: "true" run: make test-integration-cassandra + - name: Collect coverage execution data + if: ${{ !cancelled() }} + run: | + shopt -s nullglob + mkdir -p coverage-exec + for exec_file in */target/jacoco.exec metrics/*/target/jacoco.exec; do + module="${exec_file%/target/jacoco.exec}" + cp "$exec_file" "coverage-exec/${module//\//-}.exec" + done + ls -l coverage-exec + + - name: Upload coverage execution data + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + if: ${{ !cancelled() }} + with: + name: coverage-exec-cassandra-${{ matrix.cassandra-version }}-${{ matrix.java-version }}-${{ matrix.test-group }} + path: coverage-exec/ + if-no-files-found: warn + - name: Upload test results if: failure() && steps.run-integration-tests.outcome == 'failure' uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 @@ -369,8 +413,28 @@ jobs: env: SCYLLA_VERSION_RESOLVED: ${{ steps.scylla-version.outputs.value }} MAVEN_EXTRA_ARGS: ${{ steps.test-skip-args.outputs.value }} + COVERAGE: "true" run: make test-integration-scylla + - name: Collect coverage execution data + if: ${{ !cancelled() }} + run: | + shopt -s nullglob + mkdir -p coverage-exec + for exec_file in */target/jacoco.exec metrics/*/target/jacoco.exec; do + module="${exec_file%/target/jacoco.exec}" + cp "$exec_file" "coverage-exec/${module//\//-}.exec" + done + ls -l coverage-exec + + - name: Upload coverage execution data + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + if: ${{ !cancelled() }} + with: + name: coverage-exec-scylla-${{ matrix.scylla-version }}-${{ matrix.java-version }}-${{ matrix.test-group }} + path: coverage-exec/ + if-no-files-found: warn + - name: Upload test results uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 if: failure() && steps.run-integration-tests.outcome == 'failure' @@ -396,3 +460,81 @@ jobs: detailed_summary: true updateComment: false skip_annotations: true + + coverage-report: + name: Coverage report + runs-on: ubuntu-latest + needs: [unit-tests, cassandra-integration-tests, scylla-integration-tests] + # Runs even when a test lane failed: partial coverage data is still worth + # reporting, and continue-on-error keeps a flaky integration test from + # turning this metric into a second failure on the pull request. + if: ${{ !cancelled() }} + continue-on-error: true + timeout-minutes: 20 + + # Only needs to read the checkout; same-run artifacts are handled by the + # Actions runtime token rather than GITHUB_TOKEN. Scoped on this job alone + # so the existing lanes keep the token permissions their reporting steps + # rely on. + permissions: + contents: read + + env: + # Overrides the Makefile default of `mvn -B -X -ntp`; this job has nothing + # to debug and -X buys a log measured in hundreds of megabytes. + MVNCMD: mvn -B -ntp + + steps: + - name: Checkout source + uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 + with: + persist-credentials: false + + - name: Set up JDK 17 + uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 + with: + java-version: 17 + distribution: 'temurin' + + - name: Restore maven repository cache + uses: actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 + with: + path: ~/.m2/repository + key: ${{ runner.os }}-17-maven-${{ hashFiles('**/pom.xml') }} + + - name: Download coverage execution data + uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 + with: + pattern: coverage-exec-* + path: coverage-exec + + # jacoco:report-aggregate picks up every *.exec in a module's target + # directory, so each lane's data only has to land there under a name of + # its own. The module name was flattened on upload (metrics/micrometer -> + # metrics-micrometer), so undo that to find the directory again. + - name: Place execution data next to the classes it was recorded against + run: | + shopt -s nullglob + for lane in coverage-exec/*/; do + lane_name="$(basename "$lane")" + for exec_file in "$lane"*.exec; do + module="$(basename "$exec_file" .exec)" + if [[ ! -d "$module" && -d "${module/-//}" ]]; then + module="${module/-//}" + fi + mkdir -p "$module/target" + cp "$exec_file" "$module/target/jacoco-${lane_name#coverage-exec-}.exec" + done + done + find . -name 'jacoco-*.exec' -printf '%p\t%s bytes\n' + + - name: Aggregate coverage + run: make coverage-report + + - name: Upload coverage report + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + if: ${{ !cancelled() }} + with: + name: coverage-report + path: coverage-report/target/site/jacoco-aggregate + if-no-files-found: error From 12bb4a9053c212934421ee289e673af11230feda Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 20:23:03 +0000 Subject: [PATCH 3/6] docs: describe how to measure code coverage Documents the COVERAGE=true opt-in, how to combine several lanes into one number, where the report lands, and how to recognise the checksum mismatch that stale execution data produces. Added to README-dev.md, which already documents this fork's Makefile-based workflow; the upstream CONTRIBUTING.md predates it and is left alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NxW9fzmwwpSLvzcHEr5MRa --- README-dev.md | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/README-dev.md b/README-dev.md index 07a4673f8a2..2d96cda699d 100644 --- a/README-dev.md +++ b/README-dev.md @@ -27,4 +27,57 @@ Most day-to-day tasks are wrapped in the top-level `Makefile` so you do not have - `make fix` executes `mvn fmt:format` to format the code. - `make clean` removes Maven targets, shaded artifacts, and release backups to reset the tree. +### Measuring code coverage + +Coverage is measured with [JaCoCo](https://www.jacoco.org/jacoco/) and is off by default: the agent +slows every forked test JVM down, so it is opt-in through the `coverage` Maven profile. Pass +`COVERAGE=true` to any of the `test-*` Make targets to enable it, then aggregate: + +``` +make test-unit COVERAGE=true +make coverage-report +``` + +`make coverage-report` reads whatever execution data is already on disk, so several lanes can be +combined into one number -- which is the point of the separate `coverage-report` module: it +attributes the coverage `core` gets *through* the integration suite back to `core`'s own source, +which each module's own report cannot see. A `COVERAGE=true` run truncates `jacoco.exec` before it +starts, so rename the previous lane's data out of the way to keep it: + +``` +make test-unit COVERAGE=true +find . -name jacoco.exec -execdir mv jacoco.exec jacoco-unit.exec \; +make test-integration-scylla COVERAGE=true +make coverage-report +``` + +The report lands in `coverage-report/target/site/jacoco-aggregate` (HTML, XML and CSV), and +`make clean-coverage` removes it along with the execution data. `make coverage-report` fails rather +than rendering a confident-looking but empty report if it finds no execution data, or if the data +matches none of the classes. + +In CI, the unit and integration jobs in `tests@v1.yml` run with `COVERAGE=true` and upload their +execution data; the "Coverage report" job aggregates it, prints the percentage to its job summary +and attaches the HTML report as an artifact. That job is `continue-on-error`, so a flaky +integration test costs the metric some data rather than adding a second failure to the pull +request. Collecting from the existing lanes rather than a dedicated workflow keeps the Scylla suite +from being run twice. + +JaCoCo matches execution data to classes by checksum, so the data has to come from the same build +of the classes the report is rendered against. If a report shows code you know was exercised as +uncovered, look for `Execution data for class ... does not match` in the Maven log; the usual cause +is stale execution data from before a recompile, which `make clean-coverage` clears. + +Note: the surefire/failsafe configs in `core` and `integration-tests` previously set `` to +just their own JVM flags (e.g. `${mockitoopens.argline}`), which silently discarded the +`-javaagent` flag `jacoco:prepare-agent` injects into the `argLine` property -- coverage was being +collected for every *other* module, but not these two. They now combine both via Maven's +deferred-property syntax: `@{argLine} ${mockitoopens.argline}` (`@{...}` is +necessary rather than `${...}` because `jacoco:prepare-agent` sets `argLine` at build-execution +time, after the POM's own `${...}` references would already have been resolved). `argLine` itself +is declared, empty, as a root `pom.xml` property so that combination resolves to something even +outside the `coverage` profile, where `jacoco:prepare-agent` never runs to give it a real value. +(`distribution-tests` has no `src` of its own, so surefire never forks there either way; it was +left out of this.) + The Makefile automatically installs the shaded Guava dependency and, for integration tests, bootstraps the appropriate CCM toolchain and raises kernel `aio-max-nr` when required. If a target fails because the toolchain is missing, rerun after installing the prerequisites highlighted in the target output. From c5c0bba3afb085b3a150be407be34d030d875fac Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 21:52:38 +0000 Subject: [PATCH 4/6] fix: make the coverage report fail when it cannot be trusted `make coverage-report` ended in a pipe to tee, so the recipe's exit status was tee's: the python check for a report with zero covered lines printed its complaint and passed the target anyway, as did a missing python3. `set -o pipefail` makes the pipeline's status the recipe's. Also fail on the checksum mismatch itself. JaCoCo only warns when execution data does not match the classes it is rendered against, drops that class's data and renders a report that reads low with nothing to show why, which `covered == 0` catches only in the total-wipeout case. The target now tees its Maven log and greps it. The CI job lists the lanes it actually received data from in its summary, since `if: !cancelled()` lets it aggregate with lanes failed or skipped. COVERAGE is normalised, so COVERAGE=TRUE no longer runs silently without the agent, and an unrecognised value is an error rather than an "off" you find out about later. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NxW9fzmwwpSLvzcHEr5MRa --- .github/workflows/tests@v1.yml | 23 +++++++++++++++++++- Makefile | 38 ++++++++++++++++++++++++++++------ README-dev.md | 21 ++++++++++++------- 3 files changed, 67 insertions(+), 15 deletions(-) diff --git a/.github/workflows/tests@v1.yml b/.github/workflows/tests@v1.yml index dd3edfd292e..1152963ce62 100644 --- a/.github/workflows/tests@v1.yml +++ b/.github/workflows/tests@v1.yml @@ -512,19 +512,40 @@ jobs: # directory, so each lane's data only has to land there under a name of # its own. The module name was flattened on upload (metrics/micrometer -> # metrics-micrometer), so undo that to find the directory again. + # + # The lanes are listed to the job summary because "if: !cancelled()" + # lets this job run with some of them failed or skipped, and a lane + # missing its data costs the percentage silently: the report is simply + # lower, with nothing in it recording what it was built from. - name: Place execution data next to the classes it was recorded against run: | + set -o pipefail shopt -s nullglob - for lane in coverage-exec/*/; do + lanes=(coverage-exec/*/) + if [[ ${#lanes[@]} -eq 0 ]]; then + echo '::error::No coverage execution data was downloaded from any test lane.' + exit 1 + fi + { + echo '### Coverage execution data' + echo + echo "| Lane | Modules |" + echo "| --- | --- |" + } | tee -a "${GITHUB_STEP_SUMMARY:-/dev/null}" + for lane in "${lanes[@]}"; do lane_name="$(basename "$lane")" + modules=() for exec_file in "$lane"*.exec; do module="$(basename "$exec_file" .exec)" if [[ ! -d "$module" && -d "${module/-//}" ]]; then module="${module/-//}" fi + modules+=("$module") mkdir -p "$module/target" cp "$exec_file" "$module/target/jacoco-${lane_name#coverage-exec-}.exec" done + echo "| ${lane_name#coverage-exec-} | ${#modules[@]}: ${modules[*]} |" \ + | tee -a "${GITHUB_STEP_SUMMARY:-/dev/null}" done find . -name 'jacoco-*.exec' -printf '%p\t%s bytes\n' diff --git a/Makefile b/Makefile index aa0567ce155..a59f8cfa5bd 100644 --- a/Makefile +++ b/Makefile @@ -29,14 +29,25 @@ RELEASE_SKIP_TESTS ?= # execution data is on disk. Off by default: the agent slows every fork down, # and the existing test lanes have to stay able to run without it. COVERAGE ?= false -ifeq ($(filter true 1,$(COVERAGE)),) +# Spellings are normalised, and anything outside the two lists below is an +# error rather than a silent "off": COVERAGE=TRUE used to run without the +# agent, and the miss only surfaced much later, when `make coverage-report` +# said to run with COVERAGE=true -- the thing you thought you had just done. +_COVERAGE_NORM := $(or $(shell printf '%s' '$(COVERAGE)' | tr '[:upper:]' '[:lower:]'),false) +ifneq ($(filter $(_COVERAGE_NORM),true 1 yes on),) + MVN_COVERAGE := -Pcoverage + COVERAGE_PREREQ := .clean-coverage-data +else ifneq ($(filter $(_COVERAGE_NORM),false 0 no off),) MVN_COVERAGE := COVERAGE_PREREQ := else - MVN_COVERAGE := -Pcoverage - COVERAGE_PREREQ := .clean-coverage-data +# Not tab-indented, unlike the assignments above: make reads a tab-indented +# line that is not an assignment as a recipe line. +$(error COVERAGE must be one of true/1/yes/on or false/0/no/off, got '$(COVERAGE)') endif COVERAGE_REPORT_DIR := coverage-report/target/site/jacoco-aggregate +COVERAGE_MAVEN_LOG_DIR := coverage-report/target +COVERAGE_MAVEN_LOG := ${COVERAGE_MAVEN_LOG_DIR}/coverage-maven.log ifeq (${CCM_CONFIG_DIR},) CCM_CONFIG_DIR = ~/.ccm @@ -438,13 +449,28 @@ test-integration-cassandra: .install-all-modules .prepare-scylla-ccm resolve-cas # so make would otherwise treat the target as already up to date and skip it. .PHONY: coverage-report clean-coverage coverage-report: .install-guava-shaded - @if [[ -z "$$(find . -name 'jacoco*.exec' -not -path './coverage-report/*' -print -quit)" ]]; then + @# Without this the recipe's exit status is that of the tee on its last + @# line, not of the python that feeds it, so the empty-report check would + @# print its complaint and let the target pass anyway. + set -o pipefail + if [[ -z "$$(find . -name 'jacoco*.exec' -not -path './coverage-report/*' -print -quit)" ]]; then echo 'No JaCoCo execution data found.' echo "Run the tests with COVERAGE=true first, e.g. 'make test-unit COVERAGE=true'." exit 1 fi rm -rf '${COVERAGE_REPORT_DIR}' - $(MVNCMD) verify -Pcoverage -pl coverage-report -am -DskipTests -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true + mkdir -p '${COVERAGE_MAVEN_LOG_DIR}' + $(MVNCMD) verify -Pcoverage -pl coverage-report -am -DskipTests -Dfmt.skip=true -Dclirr.skip=true -Danimal.sniffer.skip=true 2>&1 | tee '${COVERAGE_MAVEN_LOG}' + # JaCoCo matches execution data to classes by checksum and only warns when + # it does not match, dropping that class's data; the report still renders, + # just quietly short. Nothing else in this target can see that -- the + # percentage is simply lower -- so fail on the warning itself. + if grep -q 'Execution data for class .* does not match' '${COVERAGE_MAVEN_LOG}'; then + echo 'Execution data does not match the compiled classes, so the report below understates coverage.' + echo 'The data has to come from the same build of the classes the report is rendered against.' + grep 'Execution data for class .* does not match' '${COVERAGE_MAVEN_LOG}' | sort -u + exit 1 + fi if [[ ! -f '${COVERAGE_REPORT_DIR}/jacoco.xml' ]]; then echo 'Maven produced no report at ${COVERAGE_REPORT_DIR}/jacoco.xml.' exit 1 @@ -460,7 +486,7 @@ coverage-report: .install-guava-shaded clean-coverage: find . -name 'jacoco*.exec' -delete find . -type d -path '*/target/site/jacoco*' -exec rm -rf {} + - rm -rf coverage-report/target/site + rm -rf coverage-report/target/site '${COVERAGE_MAVEN_LOG}' check-no-compile-warnings: @$(MAKE) compile-all | grep WARNING >/tmp/all-compile-warnings.log || true diff --git a/README-dev.md b/README-dev.md index 2d96cda699d..309565f0621 100644 --- a/README-dev.md +++ b/README-dev.md @@ -56,17 +56,22 @@ The report lands in `coverage-report/target/site/jacoco-aggregate` (HTML, XML an than rendering a confident-looking but empty report if it finds no execution data, or if the data matches none of the classes. +`COVERAGE` accepts `true`/`1`/`yes`/`on` and `false`/`0`/`no`/`off`, in any case; anything else is +an error rather than a silent "off", because a run that quietly skipped the agent only shows up +much later, when `make coverage-report` finds nothing to aggregate. + In CI, the unit and integration jobs in `tests@v1.yml` run with `COVERAGE=true` and upload their -execution data; the "Coverage report" job aggregates it, prints the percentage to its job summary -and attaches the HTML report as an artifact. That job is `continue-on-error`, so a flaky -integration test costs the metric some data rather than adding a second failure to the pull -request. Collecting from the existing lanes rather than a dedicated workflow keeps the Scylla suite -from being run twice. +execution data; the "Coverage report" job aggregates it and writes both the lanes it actually +received data from and the resulting percentage to its job summary, then attaches the HTML report +as an artifact. That job is `continue-on-error`, so a flaky integration test costs the metric some +data rather than adding a second failure to the pull request. Collecting from the existing lanes +rather than a dedicated workflow keeps the Scylla suite from being run twice. JaCoCo matches execution data to classes by checksum, so the data has to come from the same build -of the classes the report is rendered against. If a report shows code you know was exercised as -uncovered, look for `Execution data for class ... does not match` in the Maven log; the usual cause -is stale execution data from before a recompile, which `make clean-coverage` clears. +of the classes the report is rendered against. When they diverge it only warns and drops that +class's data, leaving a report that renders happily and reads low, so `make coverage-report` greps +its own Maven log for `Execution data for class ... does not match` and fails on it. The usual +cause is stale execution data from before a recompile, which `make clean-coverage` clears. Note: the surefire/failsafe configs in `core` and `integration-tests` previously set `` to just their own JVM flags (e.g. `${mockitoopens.argline}`), which silently discarded the From 8dfc86c7ccd6fa43563a08c6b0fd2527fff9a442 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 21:52:38 +0000 Subject: [PATCH 5/6] docs: correct the coverage profile's rationale in the parent pom Two comments described a state the branch no longer has. The one above surefire justified the profile by @{argLine} reaching a fork unresolved under -Djacoco.skip=true. With argLine declared empty in that reference resolves either way, as the Full verify lanes show, so jacoco.skip would in fact work; the reason to keep the profile is that opting out per job is the wrong default, not that opting out is broken. The one on the profile itself still pointed at a test-unit-coverage target and a coverage.yml workflow, neither of which exists: coverage rides the existing lanes in tests@v1.yml via COVERAGE=true. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NxW9fzmwwpSLvzcHEr5MRa --- pom.xml | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/pom.xml b/pom.xml index 22ad65737cd..b0512d64506 100644 --- a/pom.xml +++ b/pom.xml @@ -793,13 +793,11 @@ maven-surefire-plugin @@ -1055,12 +1053,13 @@ height="0" width="0" style="display:none;visibility:hidden"> coverage From 5e7d52135a204829bcb3b794cc286d74eabbde9c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 21:58:36 +0000 Subject: [PATCH 6/6] fix: only instrument the lanes the report can actually read JaCoCo matches execution data to classes by an id derived from the compiled bytes. javac 11 and javac 17 do not emit the same bytes for the same source even under --release 11: they order the constant pool differently, so the ids differ and a report rendered against one compiler's classes cannot see the other's data. Ten of the seventeen instrumented lanes ran on JDK 11 while the aggregating job compiles on 17, so their data was being loaded and then dropped. Not loudly: JaCoCo warns only when it finds no data at all under a class's id, and here the JDK 17 lanes supply a matching one, so the mismatched data loses without a word. Zero "does not match" warnings is what this looks like, not evidence against it. Instrument only the lanes on COVERAGE_JAVA_VERSION, which the aggregating job also compiles on. The number should not move, since those lanes are the ones it was already made of; the JDK 11 lanes stop paying for an agent whose output nothing read. Each lane now records the JDK it ran on next to its execution data and the aggregating job refuses a lane that does not match, so a matrix change cannot quietly bring the mismatch back. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NxW9fzmwwpSLvzcHEr5MRa --- .github/workflows/tests@v1.yml | 51 ++++++++++++++++++++++++++-------- README-dev.md | 21 +++++++++++--- 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/.github/workflows/tests@v1.yml b/.github/workflows/tests@v1.yml index 1152963ce62..7fefb725707 100644 --- a/.github/workflows/tests@v1.yml +++ b/.github/workflows/tests@v1.yml @@ -27,6 +27,19 @@ on: - ".gitignore" workflow_dispatch: +# JaCoCo matches execution data to classes by an id derived from the compiled +# bytes, and javac 11 and javac 17 do not emit the same bytes for the same +# source even under --release 11 (they order the constant pool differently). +# Execution data recorded on one of them is therefore invisible to a report +# rendered against classes compiled by the other, and invisible is literal: +# JaCoCo only warns about a name it has no id for at all, so data that loses +# to a matching id from another lane is dropped without a word. Only the lanes +# on this JDK are instrumented, and the aggregating job compiles on it and +# rejects data from any other, so the number stays one the lanes actually +# produced. +env: + COVERAGE_JAVA_VERSION: "17" + jobs: build: name: Build @@ -134,17 +147,20 @@ jobs: - name: Run unit tests env: - COVERAGE: "true" + COVERAGE: ${{ matrix.java-version == env.COVERAGE_JAVA_VERSION }} run: make test-unit # Flattened to one file per module so the artifact layout does not depend # on how many modules happened to produce data, and named per lane so the # aggregating job can keep each lane's contribution apart. - name: Collect coverage execution data - if: ${{ !cancelled() }} + if: ${{ !cancelled() && matrix.java-version == env.COVERAGE_JAVA_VERSION }} run: | shopt -s nullglob mkdir -p coverage-exec + # Read back by the aggregating job, which refuses data compiled by a + # different javac than the one it renders the report against. + echo '${{ matrix.java-version }}' > coverage-exec/jdk.txt for exec_file in */target/jacoco.exec metrics/*/target/jacoco.exec; do module="${exec_file%/target/jacoco.exec}" cp "$exec_file" "coverage-exec/${module//\//-}.exec" @@ -153,7 +169,7 @@ jobs: - name: Upload coverage execution data uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 - if: ${{ !cancelled() }} + if: ${{ !cancelled() && matrix.java-version == env.COVERAGE_JAVA_VERSION }} with: name: coverage-exec-unit-${{ matrix.java-version }} path: coverage-exec/ @@ -280,14 +296,17 @@ jobs: GET_VERSION_VERSION: 0.4.5 GH_TOKEN: ${{ github.token }} MAVEN_EXTRA_ARGS: ${{ steps.test-skip-args.outputs.value }} - COVERAGE: "true" + COVERAGE: ${{ matrix.java-version == env.COVERAGE_JAVA_VERSION }} run: make test-integration-cassandra - name: Collect coverage execution data - if: ${{ !cancelled() }} + if: ${{ !cancelled() && matrix.java-version == env.COVERAGE_JAVA_VERSION }} run: | shopt -s nullglob mkdir -p coverage-exec + # Read back by the aggregating job, which refuses data compiled by a + # different javac than the one it renders the report against. + echo '${{ matrix.java-version }}' > coverage-exec/jdk.txt for exec_file in */target/jacoco.exec metrics/*/target/jacoco.exec; do module="${exec_file%/target/jacoco.exec}" cp "$exec_file" "coverage-exec/${module//\//-}.exec" @@ -296,7 +315,7 @@ jobs: - name: Upload coverage execution data uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 - if: ${{ !cancelled() }} + if: ${{ !cancelled() && matrix.java-version == env.COVERAGE_JAVA_VERSION }} with: name: coverage-exec-cassandra-${{ matrix.cassandra-version }}-${{ matrix.java-version }}-${{ matrix.test-group }} path: coverage-exec/ @@ -413,14 +432,17 @@ jobs: env: SCYLLA_VERSION_RESOLVED: ${{ steps.scylla-version.outputs.value }} MAVEN_EXTRA_ARGS: ${{ steps.test-skip-args.outputs.value }} - COVERAGE: "true" + COVERAGE: ${{ matrix.java-version == env.COVERAGE_JAVA_VERSION }} run: make test-integration-scylla - name: Collect coverage execution data - if: ${{ !cancelled() }} + if: ${{ !cancelled() && matrix.java-version == env.COVERAGE_JAVA_VERSION }} run: | shopt -s nullglob mkdir -p coverage-exec + # Read back by the aggregating job, which refuses data compiled by a + # different javac than the one it renders the report against. + echo '${{ matrix.java-version }}' > coverage-exec/jdk.txt for exec_file in */target/jacoco.exec metrics/*/target/jacoco.exec; do module="${exec_file%/target/jacoco.exec}" cp "$exec_file" "coverage-exec/${module//\//-}.exec" @@ -429,7 +451,7 @@ jobs: - name: Upload coverage execution data uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 - if: ${{ !cancelled() }} + if: ${{ !cancelled() && matrix.java-version == env.COVERAGE_JAVA_VERSION }} with: name: coverage-exec-scylla-${{ matrix.scylla-version }}-${{ matrix.java-version }}-${{ matrix.test-group }} path: coverage-exec/ @@ -490,17 +512,17 @@ jobs: with: persist-credentials: false - - name: Set up JDK 17 + - name: Set up JDK ${{ env.COVERAGE_JAVA_VERSION }} uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 with: - java-version: 17 + java-version: ${{ env.COVERAGE_JAVA_VERSION }} distribution: 'temurin' - name: Restore maven repository cache uses: actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 with: path: ~/.m2/repository - key: ${{ runner.os }}-17-maven-${{ hashFiles('**/pom.xml') }} + key: ${{ runner.os }}-${{ env.COVERAGE_JAVA_VERSION }}-maven-${{ hashFiles('**/pom.xml') }} - name: Download coverage execution data uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 @@ -534,6 +556,11 @@ jobs: } | tee -a "${GITHUB_STEP_SUMMARY:-/dev/null}" for lane in "${lanes[@]}"; do lane_name="$(basename "$lane")" + lane_jdk="$(cat "$lane/jdk.txt" 2>/dev/null || true)" + if [[ "$lane_jdk" != "$COVERAGE_JAVA_VERSION" ]]; then + echo "::error::${lane_name} recorded its execution data on JDK ${lane_jdk:-unknown}, but this job renders the report against classes compiled by JDK ${COVERAGE_JAVA_VERSION}. JaCoCo would drop that lane's data without a warning, so fail here instead." + exit 1 + fi modules=() for exec_file in "$lane"*.exec; do module="$(basename "$exec_file" .exec)" diff --git a/README-dev.md b/README-dev.md index 309565f0621..a209f7e2827 100644 --- a/README-dev.md +++ b/README-dev.md @@ -60,19 +60,32 @@ matches none of the classes. an error rather than a silent "off", because a run that quietly skipped the agent only shows up much later, when `make coverage-report` finds nothing to aggregate. -In CI, the unit and integration jobs in `tests@v1.yml` run with `COVERAGE=true` and upload their +In CI, the unit and integration lanes in `tests@v1.yml` run with `COVERAGE=true` and upload their execution data; the "Coverage report" job aggregates it and writes both the lanes it actually received data from and the resulting percentage to its job summary, then attaches the HTML report as an artifact. That job is `continue-on-error`, so a flaky integration test costs the metric some data rather than adding a second failure to the pull request. Collecting from the existing lanes rather than a dedicated workflow keeps the Scylla suite from being run twice. -JaCoCo matches execution data to classes by checksum, so the data has to come from the same build -of the classes the report is rendered against. When they diverge it only warns and drops that -class's data, leaving a report that renders happily and reads low, so `make coverage-report` greps +Only the lanes on `COVERAGE_JAVA_VERSION` (a workflow-level variable, JDK 17) are instrumented. +javac 11 and javac 17 do not emit the same bytes for the same source even under `--release 11` -- +they order the constant pool differently -- so the class ids differ, and a report rendered against +one compiler's classes cannot see the other's execution data. Instrumenting the JDK 11 lanes would +not have added anything to the number; it would only have slowed them down. Each lane records the +JDK it ran on alongside its execution data and the aggregating job refuses anything that does not +match its own, so a future matrix change cannot quietly reintroduce the mismatch. + +JaCoCo matches execution data to classes by an id derived from the compiled bytes, so the data has +to come from the same build of the classes the report is rendered against. When they diverge it +drops that class's data and the report renders happily, just short, so `make coverage-report` greps its own Maven log for `Execution data for class ... does not match` and fails on it. The usual cause is stale execution data from before a recompile, which `make clean-coverage` clears. +That warning only covers the case where JaCoCo finds no data at all under a class's id. When two +builds of the same class are represented -- one matching, one not -- the matching one wins and the +other is dropped in silence, which is why the JDK the data was recorded on is checked separately +rather than left to the warning. + Note: the surefire/failsafe configs in `core` and `integration-tests` previously set `` to just their own JVM flags (e.g. `${mockitoopens.argline}`), which silently discarded the `-javaagent` flag `jacoco:prepare-agent` injects into the `argLine` property -- coverage was being