Run libev unit tests in CI and stop silently skipping tests#911
Draft
sylwiaszunejko wants to merge 9 commits into
Draft
Run libev unit tests in CI and stop silently skipping tests#911sylwiaszunejko wants to merge 9 commits into
sylwiaszunejko wants to merge 9 commits into
Conversation
test_watchers_are_finished calls libev__cleanup which stops the preparer, which causes the test_multi_timer_validation to hang if it runs later. Simplest fix is to not only restore _global_loop._shutdown but also restart the prepared with _global_loop._preparer.start().
It looks like some recent version of Python removed distutils from stdlib, which causes test collection to fail for me because of missing import. The fix is to add setuptools to dev dependencies.
Tests skip themselves when their requirements are missing, which is a footgun in CI: a libev/lz4/event-loop test could be silently skipped and nobody would notice. Add a CASS_DRIVER_NO_SKIP-gated pytest hook in conftest.py that converts skips into failures (leaving xfail untouched), and enable it in the cibuildwheel test-commands with precise -k/--ignore filters plus -v output. Install the lz4 test-extra so the compression tests actually run, and add -v to the integration-tests workflow.
The final callback was invoked with host_errors (the errors from only the last pool to finish) instead of the accumulated errors dict. If the last pool succeeded, failures from other pools were silently lost. Pass the aggregated errors dict, matching the method's docstring.
The docstring promised ValueError for an invalid scope, but no check existed: an unknown value silently fell through to cluster-wide behaviour. Coerce/validate the argument against SchemaAgreementScope up front (also accepting the plain string values) and raise a clear ValueError listing the valid scopes.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
PyPy never builds the libev/cmurmur3/Cython C extensions (setup.py forces is_pypy to skip them even with CASS_DRIVER_BUILD_EXTENSIONS_ARE_MUST=yes), so the tests depending on those extensions legitimately skip. Under CASS_DRIVER_NO_SKIP those skips became failures. Add a cibuildwheel override for 'pp*' that runs the suite without no-skip (same policy as Windows).
In cibuildwheel the tests are run against the installed wheel, but pytest's default 'prepend' import mode inserts the project root at sys.path[0]. That shadows the installed compiled wheel with the in-tree pure-Python cassandra/ source, so 'import cassandra.row_parser' / 'cassandra.io.libevreactor' / 'cassandra.cmurmur3' all fail and every libev/cmurmur3/Cython test skips. Under CASS_DRIVER_NO_SKIP those skips became failures. Switch to --import-mode=append so site-packages (the wheel) takes precedence while the 'tests' package is still importable from the appended project root.
The atexit subprocess test inserted the project root at sys.path[0], which shadows the installed compiled wheel with the in-tree pure-Python source. Under cibuildwheel that source lacks the libev C extension, so the import failed and the subprocess produced no output. Append the path instead so the installed wheel takes precedence (falling back to the source tree only when the driver isn't installed).
PyPy has no prebuilt lz4 wheel, so 'pip install scylla-driver[compress-lz4]' compiled lz4 from source and failed (gcc 14 rejects the lz4 C sources). The pp* override inherited test-extras from [tool.cibuildwheel.linux]; clear it so lz4 is not installed on PyPy. The lz4 tests just skip there (no-skip is off).
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.
Motivation
Tests skip themselves when their requirements are missing (a library is absent,
the wrong event loop is selected, the C extensions aren't built). That's fine
locally but a footgun in CI: a test can be silently skipped and nobody
notices. The libev unit tests were effectively not running in any CI
configuration — enabling them also surfaced two pre-existing
cluster.pybugs.Changes
CASS_DRIVER_NO_SKIP-gated pytest hook intests/conftest.pyturns skips into failures (xfail untouched). Enabled in thecibuildwheel test-commands where C extensions are mandatory (Linux/macOS).
Tests that genuinely can't run in the default config are listed explicitly via
-k/--ignore(reactor tests run separately perEVENT_LOOP_MANAGER;asyncore, column_encryption, and a few upstream-disabled/flaky tests excluded).
Windows and PyPy keep it off (the C extensions are optional on Windows and are
never built on PyPy, so their extension-dependent skips are legitimate). Local
dev is unaffected.
-vto all pytest invocations, and install thecompress-lz4test-extra so the lz4 tests actually run.Session._set_keyspace_for_all_poolsreported only the last pool's errorsinstead of the aggregated dict; failures from other pools were lost.
Session.wait_for_schema_agreementnow validatesscopeand raisesValueErrorfor unknown values (as the docstring already promised).Fixes: #904
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.