From 8ca72545a27b58c62bf43acffc8428fc2a6bb0b3 Mon Sep 17 00:00:00 2001 From: Roy Dahan Date: Wed, 19 Aug 2026 14:46:55 +0300 Subject: [PATCH] Add code-coverage-setup skill Distills a hands-on session that added code coverage measurement (unit + integration tests, combined into one report) to four ScyllaDB driver repositories -- Python, Go, Rust, and Java -- across two rounds of maintainer code review. Not a ScyllaDB-product skill like the others in this repo: it's a general software-engineering skill (how to add/fix coverage tooling in any repository) that happened to come out of ScyllaDB driver work. Opening as a draft to discuss whether/how it fits this repo's scope before merging. The content leads with empirically-verified gotchas rather than generic tool docs, since that's what actually made the difference in practice: a stale compiled extension shadowing instrumented Python source, a test runner's default fail-fast behavior silently discarding coverage data for every test after the first failure (three different disguises, in Go/nextest/Maven), a Maven argLine override silently clobbering JaCoCo's injected agent flag, and a coverage counter-mode clash when merging runs recorded under different modes. Co-Authored-By: Claude Sonnet 5 --- README.md | 2 + skills/code-coverage-setup/SKILL.md | 172 +++++++++++++++ skills/code-coverage-setup/references/go.md | 157 ++++++++++++++ skills/code-coverage-setup/references/java.md | 203 ++++++++++++++++++ .../code-coverage-setup/references/python.md | 116 ++++++++++ skills/code-coverage-setup/references/rust.md | 77 +++++++ 6 files changed, 727 insertions(+) create mode 100644 skills/code-coverage-setup/SKILL.md create mode 100644 skills/code-coverage-setup/references/go.md create mode 100644 skills/code-coverage-setup/references/java.md create mode 100644 skills/code-coverage-setup/references/python.md create mode 100644 skills/code-coverage-setup/references/rust.md diff --git a/README.md b/README.md index 5d0e1c0..de6fbea 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,7 @@ npx skills add scylladb/agent-skills --skill scylladb-data-modeling npx skills add scylladb/agent-skills --skill scylladb-vector-search npx skills add scylladb/agent-skills --skill scylladb-alternator npx skills add scylladb/agent-skills --skill scylladb-kubernetes +npx skills add scylladb/agent-skills --skill code-coverage-setup ``` ### Claude Code plugin @@ -52,6 +53,7 @@ Clone this repo and copy the skill folders into the appropriate directory for yo | [scylladb-vector-search](skills/scylladb-vector-search/SKILL.md) | Implement and optimize Vector Search on ScyllaDB Cloud — vector indexes, ANN queries, filtering, quantization, and driver integration. | | [scylladb-alternator](skills/scylladb-alternator/SKILL.md) | Build against ScyllaDB's DynamoDB-compatible API — write isolation policies, load balancing (no single endpoint like real DynamoDB), authentication via CQL roles, and compatibility differences to know before porting a DynamoDB app. | | [scylladb-kubernetes](skills/scylladb-kubernetes/SKILL.md) | Deploy and operate ScyllaDB on Kubernetes via the ScyllaDB Operator — node preparation (CPU pinning, XFS/local storage), ScyllaCluster configuration, installation/upgrades, client access & networking, TLS/mTLS & authentication, multi-datacenter, scaling, and monitoring/backup. | +| [code-coverage-setup](skills/code-coverage-setup/SKILL.md) | Add, fix, or extend code coverage measurement (unit + integration tests combined into one report) in a repository — Python, Go, Rust, Java in detail, general guidance for other languages. Not ScyllaDB-specific; came out of adding coverage tooling across several ScyllaDB driver repos and captures the gotchas found doing it. | The skills follow the [Agent Skills format](https://agentskills.io). diff --git a/skills/code-coverage-setup/SKILL.md b/skills/code-coverage-setup/SKILL.md new file mode 100644 index 0000000..ae41e4c --- /dev/null +++ b/skills/code-coverage-setup/SKILL.md @@ -0,0 +1,172 @@ +--- +name: code-coverage-setup +description: Add, fix, or extend code coverage measurement (unit + integration tests, combined into one report) in a software repository, in any language. Use this whenever the user asks to "add code coverage", "measure test coverage", "set up a coverage tool", wants a coverage badge/report/CI job, or reports that an existing coverage pipeline shows 0%, wrong numbers, missing modules, or silently-empty data — the empirically-verified gotchas here (stale compiled extensions, test-runner fail-fast eating coverage data, flag-parsing quirks, Maven argLine clobbering, counter-mode clashes) are the actual root causes of most "coverage isn't working" reports and are easy to miss without having hit them before. Covers Python, Go, Rust, and Java in detail (see references/), with a general approach for any other language. +--- + +# Code coverage setup + +This skill distills a hands-on session that added coverage measurement to four real +production repositories (a Python driver, a Go driver, a Rust driver, a Java/Maven +multi-module driver), including two rounds of maintainer code review. The single +biggest lesson: **coverage tooling looks like it's working long after it has quietly +stopped measuring anything real.** A clean exit code, a plausible log line ("argLine +set to -javaagent:..."), or a green CI job are not evidence that coverage data is +correct — in this session, three of four languages had a real, silent bug that only +showed up by directly inspecting the output artifact. Assume the same is true here +until proven otherwise. + +## Workflow + +1. **Research the repo first.** Read the build tool, the test runner, and however + CI currently invokes tests. Identify how "unit" and "integration" tests are + currently separated (env var, build tag, Maven profile, pytest marker, cargo + feature...) and how CI decides pass/fail today. Check for a coverage tool + already partially wired in (common in Java/Maven — see references/java.md) — + if one exists, your job is to fix/complete it, not replace it. +2. **Pick the tool for the language** — see references/<language>.md. Prefer + the ecosystem's modern standard over an older alternative when they behave + differently under load (e.g. LLVM source-based coverage over ptrace-based + tools for anything async/multi-threaded — ptrace assumptions break there). + Don't add a third-party dependency if the language's toolchain already has + coverage built in. +3. **Design for combining unit + integration into one report.** The point of the + exercise is usually "what fraction of the codebase do all our tests together + exercise," not two disconnected numbers. Every language here has a way to + accumulate multiple separate test-runner invocations into one merged dataset + before rendering — find it rather than settling for per-run reports users have + to average in their head. +4. **Implement additively.** Add new targets/scripts/jobs; do not change the + behavior of existing ones. A `COVER_ARGS`-style variable that defaults to + empty and gets set only by the new coverage-specific target is the pattern + used in every language this session — it means `make test` (or equivalent) + is byte-for-byte unchanged for everyone not asking for coverage. +5. **Verify empirically, not by reading the docs and moving on** (see below). + This is the step that actually catches bugs; budget real time for it. +6. **Wire CI as its own job**, not folded into an existing compatibility matrix + (multiple OS/language-version/database-version combinations). One canonical + configuration is enough for a coverage number; running it N× across a matrix + just burns CI time for the same information. Give the job `if: always()` (or + the CI system's equivalent) from the first test-running step onward, so a + test failure still lets the report generate — otherwise a broken test leaves + no coverage output at all to diagnose it with. +7. **Surface results without a third-party account by default**: a CI job + summary (plain text `report -m`/`percent`-style output) plus an uploaded + HTML/XML artifact covers most needs with zero external setup. Ask before + wiring up Codecov/Coveralls/etc. — that needs the user to enable the repo on + an external service and possibly add a secret token, which only they can do. +8. **Don't gate on a threshold yet.** Land the measurement first, observe the + baseline number for a few runs, then propose a `fail_under`-style gate as a + follow-up once there's a real number to set it against. +9. **Document known gaps instead of engineering around them.** Every language + here ended up with at least one thing coverage genuinely can't measure + (Cython-only Python modules, Go modules with no corresponding pure-Go + fallback, Rust doctests, whatever). Say so in the docs in one sentence and + move on — chasing 100% instrumentation coverage of the coverage tool itself + is not the goal. + +## Verify empirically — this is the actual point of this skill + +Every high-value bug found this session was found the same way: run the real +tool, then open the actual output file and check it has real, non-zero, sane +content — not by reading a log line and assuming it worked. + +- **A "success" message can be lying.** Java's `jacoco:prepare-agent` logged + `argLine set to -javaagent:...` on every single run, including the runs where + the flag never reached the test JVM because a module's own Surefire config + silently overwrote `argLine` afterward. The log was accurate about what + Maven's plugin did; it said nothing about whether that value survived to be + used. Trust the artifact (the `.exec`/`.out`/`jacoco.xml` file, its size, its + contents), not the narration around it. +- **Test the "one failure destroys everything" scenario on purpose.** This + recurred in three different disguises: a test runner's default fail-fast + behavior aborting the whole binary (Go, Rust — losing every subsequent + package/module's coverage, not just failing one test), and a build tool's + reactor refusing to build anything that depends on a failed module (Java — + no flag rescues this, since it's not about continuing after failure, it's + about a real dependency edge in the build graph). Deliberately make one test + fail during verification and confirm the *other* tests' coverage still shows + up in the final report. If a real cluster/service isn't available locally to + reproduce the exact failure, reproduce the mechanism with something faster + (e.g. a deliberately-broken unit test) — the failure mode is usually + independent of *why* the test failed. +- **If a reviewer reports something oddly specific, go find the actual + mechanism — don't rationalize it away.** "The CI log says no tests to run" + and "coverage flags before test-binary arguments... writes zero coverage + files" both turned out to be exactly, literally true, with a reproducible + root cause (Go's flag parser silently defaulting a package pattern to `.` + once it hit an unrecognized flag). A specific, surprising bug report from + someone who read real CI output is usually a real bug, not a + misunderstanding. +- **Local environment differences are not the repo's bug — but they can + block you from testing, so fix your own machine.** macOS ships an ancient + GNU Make (3.81, predates `.ONESHELL` from 3.82) that silently splits + multi-line recipes into separate shell invocations with no error; BSD + `find`/`readlink` reject GNU-style flags; a system JDK/Python/Go can be too + new or too old for a plugin the repo depends on. Install a matching modern + version locally (Homebrew, rustup, etc.) so you can actually run the real + commands rather than reasoning about them from a distance — but don't + "fix" the repo to work around your own machine's tool versions. Things + that are safe to write off as local-only and not worth chasing further: + missing Docker networking for a specific database image, no loopback + aliases, no `sudo` in a sandbox. Rely on real CI for whatever leg you + genuinely cannot reproduce locally, and say so plainly instead of guessing + at what it would show. +- **Read the failure, don't just retry past it.** A confusing error 1-2 layers + removed from the real cause (a flag-parse usage dump because a subpackage's + test binary didn't recognize a flag; "no test files" because the wrong + package got selected) is a clue, not noise — trace it back to the actual + mechanism before writing a fix, or the fix will be aimed at the symptom. + +## Common review feedback to anticipate + +Across two review rounds on real PRs, these came up repeatedly — bake them in +up front rather than waiting to be asked: + +- New GitHub Actions workflow files need an explicit `permissions:` block + (`contents: read` is usually sufficient) — CodeQL and CodeRabbit both flag + its absence by default. +- If a job is gated by a "disable this" label, add `labeled`/`unlabeled` to the + `pull_request` trigger's `types`, or removing the label won't re-trigger a + run until the next unrelated push. +- Shell variables built once and re-expanded unquoted later (e.g. a Makefile + variable assembled in one target, used in another) need their own embedded + quoting around anything that might contain a space (a path), because the + *later* expansion is what actually gets word-split. +- Don't silently truncate coverage scope (skip a hard-to-instrument module, + sample only some tests) without saying so in the docs or a log line — a + quiet gap reads as "fully measured" to everyone downstream. + +## Language-specific detail + +Read the matching reference file before implementing — each one has the exact +tool, the invocation shape for combining unit + integration runs, and the +specific bugs found and fixed for that language's ecosystem this session: + +| Language | Tool | Reference | +|---|---|---| +| Python | `coverage.py` (not `pytest-cov`, when the suite already runs as several separate `pytest` invocations) | [references/python.md](references/python.md) | +| Go | Built-in `go test -cover` + `GOCOVERDIR` + `go tool covdata` (Go 1.20+, no dependency needed) | [references/go.md](references/go.md) | +| Rust | `cargo-llvm-cov` (LLVM source-based; not `tarpaulin`, which is ptrace-based and mishandles async/multi-threaded tests) | [references/rust.md](references/rust.md) | +| Java | JaCoCo via `jacoco-maven-plugin` (often already declared but non-functional — check before assuming it needs adding) | [references/java.md](references/java.md) | + +### Other languages + +No hands-on verification yet for these — treat the tool choice as a reasonable +starting point, not a battle-tested recommendation, and lean extra hard on the +empirical-verification section above: + +- **JavaScript/TypeScript**: `c8` (V8's native coverage, works well with any + runner) or `nyc`/Istanbul for older/Babel-based setups. Combine multiple + runner invocations by pointing them at the same `nyc_output`/`.nyc_output` + directory before the final `nyc report`. +- **C/C++**: `gcov`/`lcov` (GCC) or `llvm-cov` (Clang), typically wired through + `--coverage`/`-fprofile-arcs -ftest-coverage` compiler flags. `lcov --add-tracefile` + merges multiple `.info` files from separate test binaries/runs. +- **Ruby**: `SimpleCov`, which already supports merging multiple runs via + `SimpleCov.collate` or `use_merging`. +- **C#/.NET**: `coverlet` (works with `dotnet test`), reports mergeable via + `ReportGenerator`. + +Whatever the tool, the same shape applies: instrument, run unit + integration, +merge, render, verify the merged artifact has real per-file percentages before +declaring victory. diff --git a/skills/code-coverage-setup/references/go.md b/skills/code-coverage-setup/references/go.md new file mode 100644 index 0000000..dc32fc9 --- /dev/null +++ b/skills/code-coverage-setup/references/go.md @@ -0,0 +1,157 @@ +# Go: built-in `go test -cover` + `covdata` + +## Tool choice + +Use Go's built-in coverage tooling — `go test -cover -coverpkg=./...` plus +`GOCOVERDIR`/`go tool covdata` (available since Go 1.20, see the official +["integration test coverage"](https://go.dev/testing/coverage/) feature) — not +a third-party tool. This is the modern standard and needs zero new +dependencies. Multiple separate `go test` invocations (different packages, +different build tags, unit vs. integration, different modules in a +multi-module repo) can all write into the *same* directory and get merged +together at the end — this is exactly the "combine multiple runs into one +report" mechanism this whole exercise is about. + +## Combining unit + integration into one report + +```bash +COVDIR=.coverage/data +mkdir -p "$COVDIR" + +go test -tags unit -cover -coverpkg=./... ./... -args -test.gocoverdir="$COVDIR" +go test -tags integration -cover -coverpkg=./... . -args -test.gocoverdir="$COVDIR" # see gotcha #1 below for the package pattern here + +go tool covdata percent -i="$COVDIR" # per-package summary +go tool covdata textfmt -i="$COVDIR" -o=coverage.out # legacy profile format +go tool cover -html=coverage.out -o=coverage.html +``` + +No `--no-clean`/merge flag is needed between invocations — accumulation into +the same `GOCOVERDIR` happens automatically; each `go test` process writes its +own uniquely-named counter/meta files into that directory. + +`-coverpkg=./...` controls what gets **instrumented** (build-time), completely +separate from what gets **run** (the package pattern argument, e.g. `./...` or +`.`). This distinction matters for gotcha #1 below: you can run a narrower set +of packages while still measuring cross-package coverage of code those +packages call into. + +## Multi-module repos: rendering fails across module boundaries + +If the repo has nested Go modules (its own `go.mod` in a subdirectory — +common for a vendored/forked dependency, e.g. a compression codec), a single +merged coverage profile spanning two modules will fail at render time: + +``` +cover: no required module provides package github.com/you/repo/submodule; to add it: + go get github.com/you/repo/submodule +``` + +`go tool cover` resolves source files against the module rooted at the +*current directory* — it has no concept of "render across two modules from +one profile." Fix: keep everything in **one shared `GOCOVERDIR`** (accumulation +still works fine across modules), but generate/render each module's report +**separately**, filtering with `covdata`'s `-pkg` flag and running `go tool +cover` from *within* that module's own directory: + +```bash +go tool covdata textfmt -i="$COVDIR" -pkg="github.com/you/repo/..." -o=coverage-root.out +go tool cover -html=coverage-root.out -o=coverage-root.html + +(cd submodule && go tool covdata textfmt -i="$COVDIR" -pkg="github.com/you/repo/submodule/..." -o=../coverage-submodule.out) +(cd submodule && go tool cover -html=../coverage-submodule.out -o=../coverage-submodule.html) +``` + +Two reports, one shared underlying dataset — this is the correct outcome, not +a workaround; there's no single Go tool invocation that produces one HTML page +spanning two modules. + +## Gotcha #1 (critical, subtle): a custom test flag silently breaks package resolution + +If the test suite registers its own CLI flags via `flag.String`/etc. (common +for integration tests that take `-cluster=host:port`, `-distribution=...`, +etc.), **`go test`'s flag parser stops recognizing its own flags — including the +package pattern — the moment it hits the first flag it doesn't know about.** +It does not error. It silently defaults the package pattern to `.` (current +directory) instead of whatever was actually specified (`./...`, a subpackage +path, etc.). + +Verify this is happening (or would happen) before assuming your invocation +does what it looks like it does: + +```bash +go test -tags yourtag -distribution somevalue -timeout=10s ./some/subpackage/... 2>&1 | tail -5 +# if the reported package is the ROOT module, not ./some/subpackage, +# you've hit this. Compare against the same command with -distribution removed. +``` + +**Consequences and the fix**: +- Anything placed *after* the point of confusion (including `-cover`, + `-coverpkg`, and especially `-args -test.gocoverdir=...`) is unreliable — + it may or may not be recognized as intended. Put every flag `go test` itself + needs to recognize (`-tags`, `-timeout`, `-race`, `-cover`, `-coverpkg`) + **before** any custom flag, and put the package pattern itself before the + first custom flag too. +- Use `-args` as an explicit, unambiguous boundary: everything after `-args` + goes verbatim to the compiled test binary. Put *all* custom flags there, + together with `-test.gocoverdir=...`. +- **This can still leave the wrong package selected** even with `-args` + correctly placed, if the package pattern itself (e.g. `./...`) needs to + reach packages that don't understand the custom flags at all — every + package matched by the pattern receives the *same* global test-binary args, + and a package whose test binary doesn't register a given custom flag will + fail outright with `flag provided but not defined: ...`. If the tests that + actually need those custom flags all live in one specific package, and + *other* matched packages have unrelated (even unconditionally-compiled) + test files that don't understand them, **scope the package pattern down to + exactly that one package** (e.g. `.` instead of `./...`) rather than trying + to make every package tolerate flags it doesn't need. `-coverpkg=./...` + still measures the whole module's code as it gets exercised transitively — + you lose nothing by not literally running every package's own tests in the + same invocation, if those tests don't exist under this flag/tag + configuration anyway. +- If a *different* subset of tests (e.g. a build-tag-gated suite living in its + own subpackage, with no custom flags of its own) needs to run too, give it + its **own separate, minimal invocation** targeting exactly that package, + rather than reusing the flag-heavy command meant for a different package. + Don't assume reusing an existing invocation "with a different tag" actually + runs what you think — verify which package actually executed (see above). + +## Gotcha #2: fail-fast destroys every other package's coverage data + +`go test`'s default is to run every matched package's tests regardless of +earlier failures when invoked directly — but check this is actually true for +your case (e.g. if wrapped by `cargo-nextest`-style tooling or a custom runner, +fail-fast may be the default instead; see references/rust.md for the same +issue in a different tool). If any wrapper/runner here defaults to stopping +after the first failure, and you're also using `--no-report`-style deferred +reporting to accumulate multiple runs, a single early failure can silently +discard *every other test's* coverage data, not just fail that one test. +Deliberately break one test during verification and confirm the *other* +tests' coverage still appears in the final merged report. + +## Gotcha #3: counter mode clash when merging + +`go tool covdata` refuses to merge data recorded under different coverage +counter modes: + +``` +error: counter mode clash while reading meta-data file ...: previous file had atomic, new file has set +``` + +`-race` implicitly forces `atomic` counter mode. If only *some* of your +coverage-instrumented invocations use `-race` (e.g. unit tests do, integration +tests don't), merging them fails with the error above. Fix: pass +`-covermode=atomic` **explicitly on every coverage-instrumented invocation**, +not just the one that happens to also use `-race` — don't rely on `-race` to +provide it implicitly for only one of several runs that need to merge later. + +## Review feedback seen in practice + +- New workflow files need an explicit `permissions: contents: read` block. +- If a job is gated by a "disable this" label, add `labeled`/`unlabeled` to + `pull_request.types`, or removing the label doesn't re-trigger a run. +- A shell variable assembled in one Makefile target and re-expanded, unquoted, + in another target's recipe needs an embedded escaped quote around any path + that might contain spaces — the word-splitting happens at the *later* + expansion, not where the variable was built. diff --git a/skills/code-coverage-setup/references/java.md b/skills/code-coverage-setup/references/java.md new file mode 100644 index 0000000..9632f2d --- /dev/null +++ b/skills/code-coverage-setup/references/java.md @@ -0,0 +1,203 @@ +# Java: JaCoCo (`jacoco-maven-plugin`) + +## Tool choice and first step: check if it's already there, and whether it works + +JaCoCo is the de facto standard for JVM coverage. In an established Maven +project, it's common to find `jacoco-maven-plugin` **already declared** in the +POM — `prepare-agent` bound to instrument the JVM, `report` bound to generate +an HTML/XML report. Before assuming coverage needs to be added from scratch, +check for this, and if it's there, **verify it actually produces non-empty +data** (see the gotcha below) before doing anything else. In the one real case +this was checked, the plugin was fully declared and had been for a while, but +was silently producing zero data for exactly the module that mattered most. + +If it's genuinely absent, add it to the parent POM's `pluginManagement` and +`plugins`: + +```xml + + org.jacoco + jacoco-maven-plugin + + + prepare-agent + + + report + prepare-package + report + + + +``` + +## Gotcha #1 (critical, high-value): a module's own `` silently clobbers JaCoCo's + +`jacoco:prepare-agent` works by setting the Maven property `argLine` to +include a `-javaagent:...` JVM flag, at **build-execution time**. If a +module's own Surefire (unit tests) or Failsafe (integration tests) +configuration sets `` to just its *own* flags — + +```xml + +${some.other.jvm.flag} +``` + +— it **completely replaces** the property instead of appending to it. The +`-javaagent` flag never reaches the forked test JVM. No error, no warning. +`jacoco:prepare-agent`'s own log line (`argLine set to -javaagent:...`) still +prints, correctly describing what *it* did — it has no way to know a later +plugin execution will overwrite the property before the JVM actually forks. + +**How to detect this**: run the module's tests, then check whether +`target/jacoco.exec` exists and has non-trivial size (tens of KB at least for +a real module, not 0 bytes or missing entirely). If prepare-agent's log line +looks right but this file is missing or absent, this is almost certainly why. + +**The fix** — Maven's *deferred* property syntax, `@{...}` instead of +`${...}`, combined with the module's own flags: + +```xml +@{argLine} ${some.other.jvm.flag} +``` + +`@{argLine}` (not `${argLine}`) matters specifically because +`jacoco:prepare-agent` sets the property at build-execution time, *after* the +POM's own `${...}` references would already have been resolved — `${argLine}` +would just be empty/undefined at the point Maven interpolates it, while +`@{argLine}` is resolved later, when the actual value is available. This is +the standard, documented JaCoCo/Surefire integration pattern; search for +"argLine is not applied" in JaCoCo's own docs/issue tracker if more context is +needed. Check **every** module's Surefire/Failsafe config for this pattern, +not just the obvious main one — it's easy for a project to have made the same +mistake in more than one module (e.g. a distribution/packaging test module +that copies the same config as the main one). + +## Multi-module aggregation: a dedicated `report-aggregate` module + +Each module's own `jacoco:report` only knows about that module's own classes. +Code in module A exercised *through* module B's tests (very common — e.g. a +core library module exercised by a separate integration-test module) never +gets attributed back to module A's source without an aggregation step. + +Add a new module (`packaging=pom`) that depends on every module whose exec +data + class files should be merged, and runs `jacoco:report-aggregate`: + +```xml + + ... + your-project-coverage-report + pom + + com.youmodule-a + com.youmodule-b + com.youintegration-tests${project.version} + + + + + + org.jacoco + jacoco-maven-plugin + + + + default + none + + + report + none + + + report-aggregate + verify + report-aggregate + + + + + + +``` + +Register the new module in the parent POM's ``, and exclude it from +any "don't publish this artifact" list the project already maintains (a +distribution/examples/integration-tests module is usually already excluded +from publishing the same way — follow that existing convention). + +**Do not use `jacoco.skip=true`** to try to suppress the aggregator module's +inherited default executions — that property is checked by *every* JaCoCo +goal, including `report-aggregate` itself, so it silences the one execution +you actually want to run. Use `none` overrides with matching +execution IDs instead (confirmed empirically: `jacoco.skip=true` produces +"Skipping JaCoCo execution because property jacoco.skip is set" for the +`report-aggregate` goal too, not just the inherited ones). + +To generate: run whichever test targets you want measured first (so their +`jacoco.exec` files exist on disk), *then* build the aggregator module with +its dependencies (`-am`) and `-DskipTests` (so that rebuild doesn't re-run — +or overwrite the already-collected data for — any module's tests): + +```bash +mvn verify -pl your-project-coverage-report -am -DskipTests +``` + +## Gotcha #2: Maven's reactor stops at the first failing module — and skips everything depending on it + +A plain `mvn test` (or any reactor-wide build) marks a module "FAILURE" the +moment its own tests fail, and **Maven then refuses to build any module that +depends on it**, even just to run *that* module's own unrelated tests. This is +not a fail-fast *setting* that a flag can override — it's a real edge in the +build dependency graph. Confirmed empirically: `-fae` (fail-at-end) and +`--fail-never` do **not** rescue this for a module with a genuine `` +on the failed one; those flags only let Maven continue building *independent* +modules that don't depend on the failure. A test failure in a foundational +module (e.g. `core`) silently costs every dependent module's coverage data +too, not just `core`'s own. + +**Fix**: for a coverage-instrumented unit-test run across a multi-module +reactor, don't rely on one `mvn test` call for the whole reactor. Instead: + +```bash +mvn install -DskipTests ... # once: get every module's artifact into the local repo, tests skipped so nothing can fail here +for module in core module-b module-c ...; do # every module with real unit tests + mvn test -pl "$module" ... || true # each is now its own invocation +done +``` + +Because every module is already installed in step 1, testing them +independently in step 2 doesn't need `-am` (also-make) and doesn't re-trigger +a rebuild of the whole reactor per module. One module's test failure in the +loop can now only cost that module's own coverage data — verified empirically +by deliberately having one module's tests fail and confirming the other +modules' `jacoco.exec` files still populated with real data afterward. + +Integration tests using `maven-failsafe-plugin` typically **don't** need this +treatment: Failsafe already separates *running* integration tests +(`integration-test` phase, which completes regardless of failures) from +*failing the build* on their results (`verify` phase) — so a test failure +there was never able to prevent coverage data from being written in the first +place. Confirm this is actually the plugin in use before assuming it, though. + +## Local environment gotchas (don't fix the repo for these) + +- macOS ships GNU Make 3.81 (2006), which predates `.ONESHELL` (introduced in + 3.82) — a Makefile using `.ONESHELL:` for multi-line recipes silently gets + each line run as a *separate* shell invocation instead, with no error, + breaking anything that depends on shared shell state (loops, variables) across + lines. Install a modern Make (`brew install make`, use as `gmake` or put + `gnubin` first on `PATH`) to verify locally; this is not something to change + in the project's Makefile. +- A build-time formatter/annotation-processing plugin can be incompatible with + a JDK newer than what CI uses (e.g. a Java-source-manipulating plugin built + against older `javac` internals breaking on JDK 21+). Match the JDK version + CI actually uses (check the workflow file) rather than assuming "newest + available" is the safe choice for local verification. +- A target name that happens to match a real file or directory in the repo + (e.g. a new module directory named the same as its Make target) needs an + explicit `.PHONY:` declaration, or `make` treats the target as already + up to date (since the path exists) and silently skips the recipe entirely. diff --git a/skills/code-coverage-setup/references/python.md b/skills/code-coverage-setup/references/python.md new file mode 100644 index 0000000..aefd02f --- /dev/null +++ b/skills/code-coverage-setup/references/python.md @@ -0,0 +1,116 @@ +# Python: coverage.py + +## Tool choice + +Use `coverage.py` directly (CLI: `coverage run` / `coverage combine` / `coverage +report` / `coverage html` / `coverage xml`), not the `pytest-cov` plugin, **when** +the suite already runs as several separate `pytest` invocations (one per +reactor/backend/test category, as is common in drivers with multiple I/O +backends, or a separate unit vs. integration invocation). `pytest-cov` only adds +value inside a single pytest invocation; wrapping each existing invocation with +`coverage run` (parallel mode) and merging with `coverage combine` afterward is +more transparent here and avoids an extra pytest plugin dependency for no real +benefit. If the project only ever runs `pytest` once for everything, `pytest-cov` +is a perfectly fine, simpler choice — don't force the CLI-wrapping approach where +it isn't needed. + +## Config (pyproject.toml or .coveragerc) + +```toml +[tool.coverage.run] +source = ["your_package"] +branch = true +parallel = true # each `coverage run` writes .coverage. instead of clobbering +relative_files = true +omit = ["your_package/_version.py"] # generated files, vendored code + +[tool.coverage.report] +show_missing = true +exclude_lines = ["pragma: no cover", "raise NotImplementedError", "if TYPE_CHECKING:"] + +[tool.coverage.html] +directory = "htmlcov" +``` + +`parallel = true` means every `coverage run` invocation gets its own uniquely- +suffixed data file automatically — no `--parallel-mode` flag needed on each +call, and no risk of one invocation's data clobbering another's. + +## Combining unit + integration + multiple test categories + +```bash +rm -f .coverage .coverage.* +coverage run -m pytest tests/unit ... +SOME_BACKEND=x coverage run -m pytest tests/unit/backend_x_tests ... # repeat per category +coverage run -m pytest tests/integration ... # only if a live service is reachable + +coverage combine +coverage report -m +coverage html +coverage xml +``` + +Every invocation above accumulates into the same coverage data set as long as +`parallel = true` is set and you don't `coverage erase` between them. Guard the +integration leg on whatever env var signals a live service is configured +(`DATABASE_URL`, `SCYLLA_VERSION`, etc.) so contributors without one locally +still get a useful unit-only report. + +## The gotcha: stale compiled extensions shadow the instrumented source + +If the package has **optional C/Cython/native extensions** (common in drivers — +a compiled accelerator with a pure-Python fallback), this is the single most +likely reason a coverage report comes back at 0% for exactly the files that +matter most, while everything else looks fine. + +**Mechanism**: `coverage.py` (like any `sys.settrace`-based tool) can only trace +plain Python bytecode — it cannot see inside a compiled extension. If the +package is normally built with the accelerator enabled, you need to force a +pure-Python build (an env var like `PACKAGE_NO_EXTENSIONS=1`, or whatever the +project's build system exposes) so the files you actually want line coverage +for compile to bytecode instead. That much is usually documented and easy. + +**The trap**: Python's import system prefers a compiled extension (`.so`/`.pyd`) +over the `.py` source **whenever both exist in the same directory**, regardless +of what env var is set for the *current* build. If a normal (accelerated) build +ran even once before — which it will have, for any contributor who just cloned +and set up the project normally — the old `.so` file is still sitting there, +and forcing pure-Python mode for the *next* build doesn't delete it. The new +build tool run may not even recreate that particular file (since the env var +tells the build "don't build this one"), so the stale compiled file from the +previous build silently wins on import. `coverage.py` then dutifully traces +the *compiled* module — which produces zero coverage line hits, since it isn't +Python bytecode at all — while every log line looks completely normal. + +**Fix**: before switching build modes for a coverage run, explicitly delete the +compiled artifacts for the modules affected, then do a clean rebuild/reinstall: + +```bash +find package_dir -name "*.so" -delete -o -name "*.pyd" -delete +PACKAGE_NO_EXTENSIONS=1 pip install --reinstall-package your-package # or the project's equivalent +``` + +Don't delete *every* compiled artifact indiscriminately if the package also has +extensions with no pure-Python fallback at all (rare, but check) — those simply +won't build in this mode and won't be measured either way; deleting them just +adds unnecessary rebuild cost elsewhere. + +**Verify this actually worked**: after the rebuild, `import the_module; print(the_module.__file__)` +should point at a `.py` file, not a `.so`/`.pyd`. Don't just trust that the env +var was set — confirm the import resolved to source. + +## Review feedback seen in practice + +- **Concurrency modes**: if any test category uses `gevent`/`eventlet` + monkey-patching or greenlets, pass `coverage run --concurrency=gevent,thread` + (or `eventlet,thread`) explicitly for that invocation — the default + concurrency setting can silently produce incomplete or incorrect trace data + under monkey-patched I/O. +- Set env vars that affect the build (like the no-extensions flag) at the CI + **job level**, not just for the test step — some build systems cache and + rebuild based on env var changes, and setting it only for the test-run step + can trigger a redundant rebuild partway through, wasting a couple of minutes + per run. +- If the project pins a lockfile (`uv.lock`, `poetry.lock`, etc.), remember to + update/commit it if the new coverage dependency changes resolved versions — + a lockfile-check CI step will otherwise fail on an unrelated-looking diff. diff --git a/skills/code-coverage-setup/references/rust.md b/skills/code-coverage-setup/references/rust.md new file mode 100644 index 0000000..3a7628b --- /dev/null +++ b/skills/code-coverage-setup/references/rust.md @@ -0,0 +1,77 @@ +# Rust: cargo-llvm-cov + +## Tool choice + +Use [`cargo-llvm-cov`](https://github.com/taiki-e/cargo-llvm-cov) — LLVM +source-based coverage — not `cargo-tarpaulin`. Tarpaulin is ptrace-based, and +ptrace-based instrumentation is known to misbehave with async/multi-threaded +test suites (which is most nontrivial Rust code using tokio/async-std). If the +project uses `cargo-nextest` as its test runner (common — check for a +`.config/nextest.toml` or `cargo nextest run` in CI/Makefile/justfile), +`cargo-llvm-cov` has a `nextest` subcommand that wraps it directly: +`cargo llvm-cov nextest ...` instead of `cargo llvm-cov ...` (which drives +plain `cargo test`). + +Install (also needs the `llvm-tools` rustup component, which `cargo install` +prompts for automatically): + +```bash +rustup component add llvm-tools-preview +cargo install cargo-llvm-cov cargo-nextest --locked +``` + +## Combining unit + integration into one report + +```bash +cargo llvm-cov clean --workspace # once, at the start of the whole session + +cargo llvm-cov nextest --all-features --no-report --no-fail-fast +cargo llvm-cov nextest --all-features --no-report --no-fail-fast -E 'test(integration::)' # or however the project selects its integration suite + +cargo llvm-cov report --summary-only +cargo llvm-cov report --html --output-dir target/llvm-cov +cargo llvm-cov report --lcov --output-path target/llvm-cov/lcov.info +``` + +`--no-report` on each run defers rendering; accumulation across multiple +`--no-report` invocations happens **automatically** — no `--no-clean` flag is +needed between them, only the one `cargo llvm-cov clean --workspace` at the +very start of the session to avoid mixing in stale data from a previous run. + +## Gotcha: nextest's default fail-fast destroys every other test's coverage data + +`cargo nextest run`'s default behavior is to **stop the entire run** after the +first failing test binary. Combined with `--no-report` (needed so multiple +invocations can accumulate into one merged report instead of each producing +an independent one), a single failure can silently throw away coverage data +for every test that would have run after it — not just fail that one test. + +Confirmed by direct comparison in this session: without `--no-fail-fast`, 11 +failing tests (out of 667, due to no live external service being reachable in +that environment) reduced the number of *collected* tests all the way down to +11 — everything after the failure point never ran and contributed nothing. +With `--no-fail-fast` added, all 667 ran and contributed real coverage data, +regardless of the same 11 still failing. + +**Fix**: always pass `--no-fail-fast` on every coverage-instrumented +`cargo llvm-cov nextest` invocation. Verify this by deliberately breaking one +test and confirming coverage numbers for unrelated modules are still +populated in the final report, not zeroed out. + +## Known gap: doctests aren't measured + +`cargo-llvm-cov`'s doctest coverage support requires a **nightly** toolchain. +If the project targets stable Rust (check the MSRV / `rust-toolchain.toml`), +doctests simply won't be instrumented. Don't chase this — run doctests +separately via plain `cargo test --doc` for correctness (so they still catch +regressions), and document in one sentence that they aren't included in the +coverage number. This mirrors how the project's own test runner may already +treat doctests as a separate concern (e.g. because `cargo-nextest` itself +doesn't support running them either, forcing `cargo test --doc` to be a +separate step already). + +## Review feedback seen in practice + +- New workflow files need an explicit `permissions: contents: read` block. +- Surface results via CI job summary + uploaded artifact by default, not a + third-party service, unless asked.