Skip to content

coverage: install lz4 for the unit-test step - #1114

Open
roydahan wants to merge 2 commits into
masterfrom
coverage/lz4-unit-step
Open

roydahan wants to merge 2 commits into
masterfrom
coverage/lz4-unit-step

Conversation

@roydahan

@roydahan roydahan commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Update: #1088 ("connection: don't auto-negotiate LZ4 from the built-in codec alone") has merged, so this PR no longer waits on anything. Master, including #1115, has been merged into this branch. The unit-test step now reads uv run --with lz4 --with "pyasyncore; python_version >= '3.12'" coverage run ..., and the comment blocks for both overlays are kept.

Summary

The coverage lane builds with CASS_DRIVER_NO_CYTHON=1, so the Cython-only cassandra.cython_lz4 codec isn't built. The lz4 package is only available through the optional compress-lz4 extra. As a result, the lane has no lz4 codec at all:

  • the lz4 import/compat block in cassandra/connection.py (lines 72-101) never runs;
  • 7 lz4-dependent tests in tests/unit/test_segment.py are skipped.

This PR adds --with lz4 to the main unit-test uv run coverage run call in scripts/coverage.sh. uv run --with layers lz4 on top of the project env for that one call only, and leaves the project env itself unchanged, so later steps run without lz4. Coverage still reads the same [tool.coverage] config and writes parallel data files, which coverage combine merges as before.

Not changed: the workflow, the dev group, pyproject.toml, the asyncio reactor step, and the integration step.

Why only the unit step

Cluster.compression defaults to True, and the connection picks the first codec that both the driver and the server support. If lz4 were installed for the whole lane, the ~20-minute integration run would switch to LZ4.

With #1088, compression=True picks LZ4 only when the lz4 package is installed; otherwise the connection stays uncompressed. That makes LZ4 opt-in, so the integration step should keep measuring the default, uncompressed path that most users get.

Unit tests are the right place to cover the lz4 code, including the "lz4 package installed" branch that #1088 adds. The asyncio reactor step doesn't exercise compression (its helpers send 'COMPRESSION': []), so it doesn't get lz4 either.

Before / after

Unit-only scripts/coverage.sh on Python 3.13, no SCYLLA_VERSION, measured before the merges described above:

master (c362d53) this PR
test_segment.py skipped 7 0
unit run 1462 passed, 59 skipped 1469 passed, 52 skipped
cassandra/connection.py 68% (403 missed, incl. 72-101) 68% (392 missed; only fallback lines 74-75 and 80-81 of that block remain)
cassandra/segment.py 85% (16 missed) 98% (3 missed)
TOTAL 63% (6655 missed) 63% (6630 missed)

In CI (unit + integration), the first run of this PR measured 3232 missed statements, against 3278 on master at the time.

The 7 tests in tests/unit/cython/test_cython_lz4.py still skip, as expected, because the Cython codec isn't built in this lane.

Verification

  • Ran the unit-only scripts/coverage.sh on master and on this branch; numbers are in the table above. Both runs exited 0, and the asyncio reactor step passed 2/2 in both.
  • After the branch run, import lz4 fails in the project env, so lz4 isn't left behind and the integration step runs without it.
  • After merging master: bash -n passes. lz4 and asyncore (via pyasyncore) both import inside the combined --with overlay. test_segment.py and test_asyncorereactor.py pass together under it (31 passed). CI on the merged head is green.
  • The integration-step command is unchanged (see diff).
  • Not verified locally: the integration step itself, since no cluster was available. This PR's coverage job runs it.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes. (N/A: CI script change; it un-skips existing tests)
  • All commits compile, pass static checks and pass test. (unit suite; integration via this PR's CI)
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce. (N/A)
  • I have adjusted the documentation in ./docs/source/. (N/A; CONTRIBUTING.rst checked and still accurate)
  • I added appropriate Fixes: annotations to PR description. (N/A)

🤖 Generated with Claude Code

https://claude.ai/code/session_01QYcPjKDEQaQTfxpdfsFjZA

The coverage lane builds without Cython (CASS_DRIVER_NO_CYTHON=1), so the
Cython lz4 codec isn't available, and the `lz4` package is only in the
optional compress-lz4 extra. As a result no lz4 codec exists in the lane:
connection.py's lz4 import/compat block never runs and the lz4-dependent
tests in tests/unit/test_segment.py skip.

Add `--with lz4` to the main unit-test `uv run` so those paths are
measured. `--with` layers lz4 over the project env for that invocation
only; the project env itself is left untouched.

Only the unit step gets it. Cluster.compression defaults to True and the
connection picks the first codec both sides support, so installing lz4
for the whole lane would make the integration run negotiate LZ4. With
#1088, LZ4 is auto-selected only when the `lz4` package is installed, so
it is opt-in; the integration step should keep measuring the default,
uncompressed behaviour. The asyncio reactor step doesn't exercise
compression and doesn't need it either.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QYcPjKDEQaQTfxpdfsFjZA
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: QUIET
  • Plan: Advanced
  • Run ID: ce2d6c6c-cc1e-426a-9ae6-fcf4d3412d0b


📥 Commits

Reviewing files that changed from the base of the PR and between fc285ca and 4a03231.



📒 Files selected for processing (1)
  • scripts/coverage.sh


Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.




📝 Walkthrough
📝 Walkthrough

Walkthrough

The unit-test coverage command adds lz4 to its temporary environment. The conditional pyasyncore dependency remains unchanged. The integration coverage command does not add lz4.



Priority: ⬇️ Low

Change: Other

Merge Risk: ⚪ Minimal · up to 4a032

This change makes LZ4 available only to the unit-test run; the integration run retains its uncompressed path. No merge-blocking risk is established.

Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely identifies the main change: installing lz4 for the unit-test coverage step.
Description check Passed The description follows the template, explains the change and rationale, records verification results, and marks non-applicable checklist items. It also identifies that the integration step was not ve…

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@roydahan
roydahan marked this pull request as ready for review October 11, 2026 15:07

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants