Add code coverage measurement for unit and integration tests - #1005
Open
roydahan wants to merge 3 commits into
Open
Add code coverage measurement for unit and integration tests#1005roydahan wants to merge 3 commits into
roydahan wants to merge 3 commits into
Conversation
jacoco-maven-plugin was already declared in the parent pom (prepare-agent
+ report bound to every module), but two things kept it from doing
anything useful:
1. core/pom.xml and integration-tests/pom.xml (surefire and failsafe,
respectively) set <argLine> to just their own JVM flags
(${mockitoopens.argline} / ${blockhound.argline}), completely
replacing rather than combining with the value jacoco:prepare-agent
injects into that property. Confirmed empirically: before this fix,
running core's unit tests never wrote core/target/jacoco.exec at
all -- the -javaagent flag jacoco set up never reached the forked
test JVM. Fixed by combining both via Maven's deferred-property
syntax, `<argLine>@{argLine} ${mockitoopens.argline}</argLine>`
(`@{...}` rather than `${...}` because prepare-agent sets `argLine`
at build-execution time, after the POM's own `${...}` references
would already have been resolved). distribution-tests/pom.xml had
the same bug and got the same fix, for the modules that do have
real jacoco data.
2. Nothing merged the resulting per-module jacoco.exec files into one
cross-module view -- coverage `core` gets exercised through the
integration suite, for instance, was never attributed back to
core's own source. Added a new `coverage-report` module (packaging
pom, depends on core/query-builder/mapper-runtime/mapper-processor/
metrics-micrometer/metrics-microprofile/integration-tests) that runs
jacoco:report-aggregate over all of them.
Makefile: `test-unit-coverage` is a new target, not a coverage-flavored
variant of the existing `test-unit`. It has to test each module as its
own `mvn` invocation rather than one reactor-wide `mvn test`: in a
single reactor build, a test failure in core makes Maven skip every
module depending on it (query-builder, mapper-runtime, ...) too,
losing their coverage data along with core's -- confirmed empirically,
and unaffected by -fae/-fn, since those only rescue independent
modules in the reactor, not ones with a real dependency on the failed
one. test-integration-scylla/test-integration-cassandra need no such
variant: maven-failsafe-plugin already separates running ITs
(integration-test phase, which always completes) from failing the
build on their results (verify phase), so a test failure there was
never able to lose coverage data to begin with. `coverage-report`
merges and renders whatever the above collected; `clean-coverage`
resets it.
CI (.github/workflows/coverage.yml) runs this against a single canonical
ScyllaDB version on every push/PR, posts a summary to the job log, and
uploads the HTML/XML/CSV reports as a build artifact -- surfaced this
way instead of through a third-party service like Codecov, matching
the choice already made for the Python, Go, and Rust drivers'
equivalent tooling this session.
Fixes: https://scylladb.atlassian.net/browse/DRIVER-891
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughAdded Maven JaCoCo aggregation for core, query-builder, mapper, metrics, and integration-test modules. Added Makefile targets and developer documentation for coverage collection, reporting, and cleanup. Added a GitHub Actions workflow for Scylla unit and integration coverage, CCM image caching, coverage summaries, and report artifacts. Preserved existing Maven Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Maven
participant ScyllaCCM
participant JaCoCo
GitHubActions->>Maven: run unit coverage
GitHubActions->>ScyllaCCM: install and prepare cached image
GitHubActions->>Maven: run Scylla integration coverage
Maven->>JaCoCo: generate aggregate report
GitHubActions->>JaCoCo: publish summary and upload report
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR adds coverage collection, aggregation, developer commands, documentation, and CI reporting without any supplied current-head issue requiring merge-blocking action. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
Collaborator
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
jacoco:report-aggregate only aggregates compile/runtime-scoped reactor dependencies. The root pom's dependencyManagement pins mapper-runtime, mapper-processor, metrics-micrometer, and metrics-microprofile to scope=test (correct for their other consumers like integration-tests), and coverage-report inherited that scope for its own dependency on them, so their classes were silently excluded from the aggregate even though their jacoco.exec data was loaded (confirmed: all 7 exec files load, only 3 modules got analyzed as bundles). This lost their own integration test coverage entirely (MicrometerMetricsIT, MicroProfileMetricsIT). Override the scope to compile for coverage-report's own dependency declarations.
…eport" This reverts commit 6b16577.
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
1 participant
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.
What
jacoco-maven-pluginwas already declared in the parentpom.xml(prepare-agent+reportbound to every module via inheritance), but two things kept it from producing anything useful.1. A real, pre-existing bug: coverage was silently not being collected for
corecore/pom.xml(surefire) andintegration-tests/pom.xml(failsafe, all three test-group executions) set<argLine>to just their own JVM flags --${mockitoopens.argline}/${blockhound.argline}-- completely replacing rather than combining with the valuejacoco:prepare-agentinjects into that same property.distribution-tests/pom.xmlhad the identical bug.Confirmed empirically while working on this: before the fix, running
core's unit tests never wrotecore/target/jacoco.execat all --jacoco:prepare-agentloggedargLine set to -javaagent:...correctly, but the flag never reached the forked test JVM, so no coverage data was ever recorded for the driver's main module. Fixed with Maven's deferred-property syntax:@{...}(not${...}) matters specifically becausejacoco:prepare-agentsetsargLineat build-execution time, after the POM's own${...}references would already have been resolved.2. Nothing merged the per-module exec files into one cross-module view
Coverage
coregets exercised through other modules -- most importantly the integration suite -- was never attributed back tocore's own source, since each module's JaCoCoreportexecution only knows about its own classes. Added a newcoverage-reportmodule (packaging=pom, depends oncore/query-builder/mapper-runtime/mapper-processor/metrics-micrometer/metrics-microprofile/integration-tests) that runsjacoco:report-aggregateover all of them into one report.Makefile
make test-unit-coverage(new target, not a drop-in replacement fortest-unit) tests each module as its ownmvninvocation rather than one reactor-widemvn test. This isn't cosmetic: in a single reactor build, a test failure incoremakes Maven skip every module that depends on it (query-builder,mapper-runtime, ...) too, discarding their coverage data along with core's. Confirmed empirically, and this is unaffected by-fae/--fail-never-- those flags only rescue independent modules in the reactor, not ones with a real dependency on the failed one.test-integration-scylla/test-integration-cassandraneed no such variant:maven-failsafe-pluginalready separates running integration tests (integration-testphase, which always completes regardless of failures) from failing the build on their results (verifyphase), so a test failure there was never able to lose coverage data in the first place.make coverage-reportmerges and renders whatever the above collected: a per-module summary plus an HTML report atcoverage-report/target/site/jacoco-aggregate/index.html, andjacoco.xml/jacoco.csvalongside it.make clean-coverageresets it.Docs added to
README-dev.md(the file that already documents this fork's Makefile-based workflow; the upstreamCONTRIBUTING.mdpredates it and wasn't touched).CI
.github/workflows/coverage.yml: runstest-unit-coverage+test-integration-scylla(a single canonical ScyllaDB version) on every push/PR, posts a summary to the job log, and uploads the HTML/XML/CSV reports as a build artifact -- surfaced this way instead of through a third-party service like Codecov, matching the choice already made for the Python, Go, and Rust drivers' equivalent tooling.Testing
Verified end-to-end locally (JDK 17, since JDK 21+ broke an unrelated
fmt-maven-plugin/google-java-formatcompatibility unrelated to this change, and only a modern GNU Make -- macOS ships GNU Make 3.81 from 2006, which predates.ONESHELL, silently splitting every multi-line recipe in this Makefile, not just my new ones):argLinebug and fix directly:core/target/jacoco.execdidn't exist after a test run before the fix, existed with real data (46KB+) after it.make test-unit-coverageend-to-end; confirmed a genuine test failure incore(a pre-existing, environment-specific timezone test failing only because my sandbox's local timezone happens to beAsia/Jerusalem-- one of the test's own parameterized cases -- not something introduced here) did not preventquery-builder/mapper-runtime/mapper-processor/metrics-micrometer/metrics-microprofilefrom being tested and contributing coverage data, whereas a single reactor-widemvn test -faedid lose all of them, which is what motivated the per-module-invocation design.make coverage-report; got a real aggregate report across all 6 modules (952 + 174 + 17 + 93 + 5 + 5 classes analyzed), 70.4% line coverage from unit tests alone in this constrained environment (no live cluster available locally for the integration leg, which CI's job exercises).Fixes: https://scylladb.atlassian.net/browse/DRIVER-891