Add code coverage measurement for unit and integration tests - #967
Add code coverage measurement for unit and integration tests#967roydahan wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds coverage configuration, a local coverage script, contributor documentation, and a GitHub Actions workflow. The script runs unit and reactor-specific tests, optionally runs integration tests, and generates text, HTML, and XML reports. The workflow provisions Python, JDK, system dependencies, and Scylla before running the script. It publishes the coverage summary and report artifacts. Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant uv
participant Scylla
participant coverage.sh
participant CoverageReports
GitHubActions->>uv: install tooling and sync the driver
GitHubActions->>Scylla: provision Scylla
GitHubActions->>coverage.sh: run coverage tests
coverage.sh->>CoverageReports: generate coverage reports
GitHubActions->>CoverageReports: publish summary and artifacts
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds coverage.py-based measurement for unit and integration tests, with local tooling, configuration, CI reporting, and contributor documentation.
Changes:
- Adds a coverage runner that rebuilds without optional Cython modules and combines parallel test results.
- Configures coverage reporting and CI artifact generation.
- Documents usage and ignores generated artifacts.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/coverage.sh |
Runs unit/reactor and optional integration suites under coverage. |
pyproject.toml |
Adds coverage dependency and reporting configuration. |
CONTRIBUTING.rst |
Documents local coverage usage and limitations. |
.gitignore |
Ignores generated coverage data and reports. |
.github/workflows/coverage.yml |
Adds the coverage CI workflow and report artifacts. |
Suppressed comments (1)
.github/workflows/coverage.yml:24
- This filter prevents a pull request that changes only
scripts/coverage.shfrom validating the coverage command in CI. Since the script is the single source of truth invoked below, it must not be excluded from this workflow's PR triggers.
- scripts/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
CONTRIBUTING.rst-124-125 (1)
124-125: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a supported Scylla release.
release:2025.2has been unsupported since December 2025. Userelease:2026.1, which is the CI target. (docs.scylladb.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.rst` around lines 124 - 125, Update the SCYLLA_VERSION value in the coverage command to use the supported CI target release:2026.1 instead of the unsupported release:2025.2, while preserving the existing integration-suite invocation.scripts/coverage.sh-27-47 (1)
27-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGenerate reports after test failures.
A failing test exits under
set -ebeforecoverage combine,coverage html, andcoverage xml. Record the test failure, generate reports, then exit with the recorded status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/coverage.sh` around lines 27 - 47, The test commands in the coverage script currently terminate execution before report generation when any test fails. Update the test execution flow to capture the failing status without immediate exit, always run coverage combine, report, html, and xml generation, then exit with the recorded test status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/coverage.yml:
- Around line 8-19: Remove the scripts/* exclusion from both paths-ignore
trigger blocks in the coverage workflow so changes to scripts/coverage.sh
trigger the workflow, while preserving all other path exclusions.
---
Other comments:
In `@CONTRIBUTING.rst`:
- Around line 124-125: Update the SCYLLA_VERSION value in the coverage command
to use the supported CI target release:2026.1 instead of the unsupported
release:2025.2, while preserving the existing integration-suite invocation.
In `@scripts/coverage.sh`:
- Around line 27-47: The test commands in the coverage script currently
terminate execution before report generation when any test fails. Update the
test execution flow to capture the failing status without immediate exit, always
run coverage combine, report, html, and xml generation, then exit with the
recorded test status.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: c29a0d89-9b07-409e-aa47-d29da8bd209d
📒 Files selected for processing (5)
.github/workflows/coverage.yml.gitignoreCONTRIBUTING.rstpyproject.tomlscripts/coverage.sh
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
.github/workflows/coverage.yml:23
- Ignoring
scripts/*on pull requests means a PR that only changesscripts/coverage.shwill skip the only workflow that exercises that script, so broken coverage tooling can merge without pre-merge validation. Keep script changes in this workflow's path filter; the integration workflow's similar filter is not sufficient because it does not invoke this coverage wrapper.
- scripts/*
scripts/coverage.sh:32
- The script claims to cover all unit-test reactors, but it permanently ignores the asyncore reactor and never runs that file separately. On supported Python versions where
asyncoreis still available (for example 3.9–3.11), this drops all asyncore reactor coverage; on newer Python versions the file can remain in the main run and will skip itself becauseASYNCCORE_AVAILABLEis false. Remove this ignore so the documented coverage scope is accurate.
--ignore=tests/unit/io/test_asyncorereactor.py
.github/workflows/coverage.yml:85
always()does not preserve a report when any pytest invocation fails:set -estopscoverage.shbeforecoverage combine/html/xml, leaving only parallel.coverage.*files. This step then runscoverage reportagainst the missing.coveragefile, and the artifact step has no HTML/XML outputs to upload. Combine partial data and generate the outputs here so failed test runs still publish the diagnostic coverage results.
if: always()
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.rst (1)
131-137: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale
Building the Driverreference.The supplied
CONTRIBUTING.rsthas noBuilding the Driversection. Point this text toDev setupor add the referenced section.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.rst` around lines 131 - 137, Update the documentation reference in the coverage explanation to point to the existing ``Dev setup`` section instead of the missing ``Building the Driver`` section, without changing the surrounding guidance.
🟡 Other comments (1)
scripts/coverage.sh-39-44 (1)
39-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
EVENT_LOOP_MANAGERfor the default unit run.
tests/__init__.py:51reads this variable. The first command inherits the caller's value, soEVENT_LOOP_MANAGER=gevent bash scripts/coverage.shreplaces the default run with gevent coverage. Set the variable to an empty value for the default invocation.Proposed fix
-uv run coverage run -m pytest tests/unit -v \ +EVENT_LOOP_MANAGER= uv run coverage run -m pytest tests/unit -v \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/coverage.sh` around lines 39 - 44, Update the default unit-test invocation in the coverage script to explicitly set EVENT_LOOP_MANAGER to an empty value before running coverage, ensuring tests/__init__.py uses the default event-loop manager regardless of the caller’s environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/coverage.sh`:
- Around line 12-18: Update the coverage script’s setup and cleanup flow to fail
immediately when commands such as cd, rm, or find fail, while preserving
non-fatal handling for test and report-generation commands so the recorded test
status still reaches the final exit. Check each setup/cleanup command explicitly
or scope error suppression only to the test/report invocations, ensuring cleanup
cannot continue in an unintended directory or use stale data.
---
Outside diff comments:
In `@CONTRIBUTING.rst`:
- Around line 131-137: Update the documentation reference in the coverage
explanation to point to the existing ``Dev setup`` section instead of the
missing ``Building the Driver`` section, without changing the surrounding
guidance.
---
Other comments:
In `@scripts/coverage.sh`:
- Around line 39-44: Update the default unit-test invocation in the coverage
script to explicitly set EVENT_LOOP_MANAGER to an empty value before running
coverage, ensuring tests/__init__.py uses the default event-loop manager
regardless of the caller’s environment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: d5cc1f82-ddf0-4502-be0f-7f7de5a22577
📒 Files selected for processing (3)
.github/workflows/coverage.ymlCONTRIBUTING.rstscripts/coverage.sh
💤 Files with no reviewable changes (1)
- .github/workflows/coverage.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/coverage.sh (3)
37-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the asyncore test comment.
Lines 37-39 name
test_asyncorereactor.pyand say it is not ignored. Lines 44 and 48 usetests/unit/io/test_asyncioreactor.py, ignore it in the main run, and execute it separately. Update the comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/coverage.sh` around lines 37 - 49, Correct the comment above the main pytest command to reference test_asyncioreactor.py and accurately state that it is ignored there and executed separately with the asyncio event-loop configuration. Keep the existing test commands unchanged.
12-17: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd regression tests for the failure policy.
Cover setup and cleanup failures, test failures that still generate reports, and report failures that produce a non-zero exit status.
As per coding guidelines: “Add relevant tests for new features and bug fixes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/coverage.sh` around lines 12 - 17, Add regression coverage for the scripts/coverage.sh failure policy, including setup and cleanup failures, test failures that still execute combine/report/html/xml generation, and report-generation failures returning a non-zero status. Use isolated fixtures or mocks around the script’s existing status handling so each scenario verifies both report execution and the final exit status.Source: Coding guidelines
40-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the canonical pytest options.
Add
CASS_DRIVER_NO_SKIP=1and--import-mode=appendto each unit and reactor invocation. Otherwise, skipped tests remain hidden and imports may differ from the configured test commands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/coverage.sh` around lines 40 - 49, Update every pytest invocation in the coverage script, including the main unit run and each EVENT_LOOP_MANAGER reactor run, to set CASS_DRIVER_NO_SKIP=1 and pass --import-mode=append while preserving the existing test paths, ignores, and failure-status handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/coverage.sh`:
- Around line 37-49: Correct the comment above the main pytest command to
reference test_asyncioreactor.py and accurately state that it is ignored there
and executed separately with the asyncio event-loop configuration. Keep the
existing test commands unchanged.
- Around line 12-17: Add regression coverage for the scripts/coverage.sh failure
policy, including setup and cleanup failures, test failures that still execute
combine/report/html/xml generation, and report-generation failures returning a
non-zero status. Use isolated fixtures or mocks around the script’s existing
status handling so each scenario verifies both report execution and the final
exit status.
- Around line 40-49: Update every pytest invocation in the coverage script,
including the main unit run and each EVENT_LOOP_MANAGER reactor run, to set
CASS_DRIVER_NO_SKIP=1 and pass --import-mode=append while preserving the
existing test paths, ignores, and failure-status handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 161a69e4-f94c-448e-ba15-7db88e29ea0c
📒 Files selected for processing (1)
scripts/coverage.sh
Introduces coverage.py as the coverage tool, wrapping each existing pytest invocation (main unit run plus one per event-loop reactor, plus the integration suite) with `coverage run` and combining the results. Several core modules (cluster.py, connection.py, protocol.py, etc.) are optionally Cython-compiled by default, which coverage.py cannot trace into, so scripts/coverage.sh forces CASS_DRIVER_NO_CYTHON=1 and cleans any stale compiled extensions first (Python's import system otherwise prefers a leftover .so over the .py source, silently producing a false 0% report). Cython-only modules with no .py fallback are a documented, known gap for this method. scripts/coverage.sh tracks test failures in $status rather than using `set -e`, so a failing test still lets coverage combine/report/html/xml run -- otherwise there would be no coverage output at all to diagnose the failure with. Setup/cleanup steps before that point still fail fast. tests/unit/io/test_asyncorereactor.py is included rather than ignored, since it already self-skips via ASYNCCORE_AVAILABLE on Python 3.12+, where asyncore was removed from the stdlib. The GitHub Actions workflow (.github/workflows/coverage.yml) runs this against a live Scylla on every push/PR, posts the text report to the job summary, and uploads the HTML/XML report as a build artifact -- surfaced this way instead of through a third-party service like Codecov, to avoid needing an external account or token for this first pass. Fixes: https://scylladb.atlassian.net/browse/DRIVER-889 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fd9d71f to
75d309e
Compare
dkropachev
left a comment
dkropachev
left a comment
There was a problem hiding this comment.
Coverage works, but please address these issues:
-
Add explicit coverage concurrency modes for gevent/eventlet; the default mode can produce incorrect results:
coverage run --concurrency=gevent,thread ... coverage run --concurrency=eventlet,thread ...
-
Update and commit
uv.lock.uv lock --checkcurrently fails. -
Avoid deleting every
.so/.pydundercassandra/; this destroys local builds for all Python versions. -
Set
CASS_DRIVER_NO_CYTHON=1at workflow job level. Currently the summary step rebuilds Cython extensions, wasting about two minutes.
Optional follow-up: add a coverage threshold or regression gate; the current workflow only reports coverage.
…on cleanup, job-level env - scripts/coverage.sh: pass --concurrency=gevent,thread / --concurrency=eventlet,thread to the gevent/eventlet coverage runs. gevent/eventlet monkey-patch threading/sockets, which can confuse coverage.py's default sys.settrace-based collector without an explicit hint about the greenlet scheduler. - scripts/coverage.sh: replace the blanket `find cassandra -name "*.so" -delete` with a targeted cleanup scoped to the current interpreter's own EXTENSION_SUFFIXES (excluding only cmurmur3/libevwrapper, which CASS_DRIVER_NO_CYTHON doesn't affect). The previous version also deleted every other local Python version's compiled extensions, destroying their builds unnecessarily. It also missed the Cython-only modules with no .py fallback (row_parser, obj_parser, ...) when an earlier, narrower fix only targeted the ten cythonizable .py modules by name -- left in place, those keep HAVE_CYTHON true off a stale .so and silently defeat CASS_DRIVER_NO_CYTHON entirely. Verified locally: the full script still produces the same ~56% baseline it did before, and cmurmur3/libevwrapper survive while HAVE_CYTHON correctly reads False during the run. - .github/workflows/coverage.yml: set CASS_DRIVER_NO_CYTHON=1 at job level instead of only inside coverage.sh, so every step -- including "Build driver" and the summary step, both separate `uv run` invocations -- sees the same value instead of flipping the uv cache-key and triggering a full Cython rebuild between steps. - uv.lock: commit it (was gitignored) and bring it in sync with the coverage[toml] dependency added earlier; `uv lock --check` now passes from a clean checkout. Not doing (tracked as a deliberate follow-up, not a defect): a coverage threshold/regression gate. This first pass establishes a baseline; picking a threshold blind isn't useful yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all four required points in 5aeabcc:
Verified locally end-to-end after the fixes: same ~56% baseline as before, On the optional follow-up (threshold/regression gate): leaving this out for now, as noted in the original PR description — this first pass establishes a baseline, and picking a threshold blind isn't useful yet. Happy to add one in a follow-up once we have a few runs of data. |
What
Adds coverage.py-based code coverage measurement, runnable both locally and in CI, across the unit suite (all event-loop reactors) and the integration suite.
scripts/coverage.sh: single source of truth that wraps each existingpytestinvocation (main unit run, plus one per reactor: gevent/asyncio/eventlet, plus the integration suite whenSCYLLA_VERSION/CASSANDRA_VERSIONis set) withcoverage run, then combines and reports.pyproject.toml: addscoverage[toml]as a dev dependency and[tool.coverage.*]config (branch coverage on,source=["cassandra"], HTML report tohtmlcov/)..github/workflows/coverage.yml: new CI job (Ubuntu, Python 3.13) that runsscripts/coverage.shagainst a live Scylla, posts the text report to the job summary, and uploadshtmlcov/+coverage.xmlas a build artifact. Nofail_undergate yet — this establishes a baseline first.CONTRIBUTING.rst/.gitignore: docs and ignores for the new script/artifacts.Why this approach
Several core modules (
cluster.py,connection.py,protocol.py,pool.py,query.py,cqltypes.py,metadata.py,util.py,concurrent.py,shard_info.py) are optionally Cython-compiled whenever Cython is importable (it's already a dev dependency). Once compiled,coverage.pycan't trace into them, soscripts/coverage.shsetsCASS_DRIVER_NO_CYTHON=1to force those specific modules to build as plain Python — the same state already exercised by the Windows/PyPy legs ofcibuildwheel, with existingcythontest/notcythonskip decorators degrading cleanly.One gotcha found while validating this locally: a prior normal (Cython-enabled) build leaves
.sofiles in place, and Python's import system prefers those over the.pysource even afterCASS_DRIVER_NO_CYTHON=1is set — silently producing a false 0% report for every affected file.scripts/coverage.shnow explicitly removes stale compiled extensions and does a cleanuv sync --reinstall-packagebefore running, which was confirmed necessary and sufficient by an end-to-end local run (core modules showed real, non-trivial coverage after the fix, 0% before it).Cython-only modules with no pure-Python fallback (
obj_parser,numpy_parser,row_parser,bytesio, etc.) aren't built at all inCASS_DRIVER_NO_CYTHON=1mode, so they remain unmeasured by this method — documented as a known limitation. Closing that gap would need Cython's own line-tracing (linetrace=True+CYTHON_TRACE=1), which adds real build complexity and is deliberately out of scope here.Coverage results are surfaced via a GitHub Actions job summary + artifact rather than Codecov/another third-party service, to avoid needing an external account or token for this first pass.
Testing
Verified end-to-end locally (unit suite only, since no local Scylla/ccm setup): ran
scripts/coverage.sh, confirmed the stale-extension issue, fixed it, and confirmed core modules report real coverage (protocol.py69%,cqltypes.py80%,metadata.py55%, etc.) withhtmlcov/index.htmlandcoverage.xmlgenerated correctly by the combined report.Pre-review checklist
./docs/source/. (contributor-facing doc lives inCONTRIBUTING.rstinstead)Fixes:annotations to PR description. (not fixing a tracked issue)