Add release sanity check to catch pytest leakage before publish#1151
Add release sanity check to catch pytest leakage before publish#1151sfmqrb wants to merge 1 commit intoqdrant:masterfrom
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis pull request adds release-time sanity checks to the CI pipeline to prevent pytest from leaking into the qdrant-client package. A new workflow step is introduced in the Python publish workflow that installs the built package without development dependencies and executes a new sanity check script. The script verifies that pytest imports do not appear in library source code and that all qdrant-client submodules can be imported without requiring pytest as a runtime dependency. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![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
🤖 Fix all issues with AI agents
In `@tests/release_sanity_check.py`:
- Around line 54-58: The module filter in the pkgutil.walk_packages loop (using
module_info / name) only excludes names containing ".tests" or ending with
".conftest" and can still include submodules whose final name component is
prefixed with "test_" (e.g., qdrant_client.pkg.test_foo); update the condition
in the loop to also skip any module whose last dotted component starts with
"test_" (parse name.split(".")[-1] or use name.rsplit(".", 1)[-1]) so the
runtime import check mirrors the source scanner behavior.
🧹 Nitpick comments (2)
tests/release_sanity_check.py (1)
46-69: Consider logging silently-swallowed exceptions for debuggability.Lines 67-69: The bare
except Exception: passis intentional (targeting only pytest leakage), but when a non-pytest import fails, silently swallowing it makes CI debugging harder. A one-line print/warning would help operators diagnose unexpected failures without changing the pass/fail semantics.Also, line 48: the
# noqa: F401is flagged by Ruff as unnecessary (theF401rule isn't enabled). Removing it keeps the file clean.Proposed changes
- import qdrant_client # noqa: F401 + import qdrant_clientexcept Exception: - # Ignore other runtime side effects; this check targets pytest leakage. - pass + # Ignore other runtime side effects; this check targets pytest leakage. + print(f" ⚠ Skipping '{name}': {exc!r}")Note: you'd need to capture the exception to print it:
- except Exception: + except Exception as exc:.github/workflows/python-publish.yml (1)
42-42: The sanity script's source check depends on CWD being the repo root.
check_no_pytest_import_in_library_sources()usesPath("qdrant_client")as a relative path. This works because the default working directory in GitHub Actions is the checkout root. If the working directory ever changes (e.g., via aworking-directory:key on this step), the source check would silently find zero files and pass vacuously. A small guard in the script (e.g., asserting the path exists) would harden this.Proposed guard in release_sanity_check.py
Add at the top of
check_no_pytest_import_in_library_sources():def check_no_pytest_import_in_library_sources() -> None: root = Path("qdrant_client") + if not root.is_dir(): + fail(f"Source directory '{root}' not found (wrong working directory?)") offenders: list[str] = []
| for module_info in pkgutil.walk_packages(qc.__path__, prefix="qdrant_client."): | ||
| name = module_info.name | ||
|
|
||
| # Never import tests in this sanity check. | ||
| if ".tests" in name or name.endswith(".conftest"): |
There was a problem hiding this comment.
Test-module filter could miss test_-prefixed submodules during walk.
Line 58 skips modules where ".tests" appears in the dotted name or the name ends with ".conftest". However, it doesn't skip test_-prefixed module names (e.g., qdrant_client.some_pkg.test_foo), while the source scanner on line 30 does skip files starting with test_. This inconsistency could cause the runtime check to attempt importing a test module that the source check would have excluded.
Proposed fix
# Never import tests in this sanity check.
- if ".tests" in name or name.endswith(".conftest"):
+ parts = name.split(".")
+ if ".tests" in name or name.endswith(".conftest") or parts[-1].startswith("test_"):
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for module_info in pkgutil.walk_packages(qc.__path__, prefix="qdrant_client."): | |
| name = module_info.name | |
| # Never import tests in this sanity check. | |
| if ".tests" in name or name.endswith(".conftest"): | |
| for module_info in pkgutil.walk_packages(qc.__path__, prefix="qdrant_client."): | |
| name = module_info.name | |
| # Never import tests in this sanity check. | |
| parts = name.split(".") | |
| if ".tests" in name or name.endswith(".conftest") or parts[-1].startswith("test_"): | |
| continue |
🤖 Prompt for AI Agents
In `@tests/release_sanity_check.py` around lines 54 - 58, The module filter in the
pkgutil.walk_packages loop (using module_info / name) only excludes names
containing ".tests" or ending with ".conftest" and can still include submodules
whose final name component is prefixed with "test_" (e.g.,
qdrant_client.pkg.test_foo); update the condition in the loop to also skip any
module whose last dotted component starts with "test_" (parse
name.split(".")[-1] or use name.rsplit(".", 1)[-1]) so the runtime import check
mirrors the source scanner behavior.
1 participant
Summary
This PR adds a release sanity check step to the publish workflow to prevent shipping broken packages when
pytestaccidentally leaks into library code.What changed
tests/release_sanity_check.pywhich:pytestis imported inqdrant_client/**outside test modulespytestat runtime.github/workflows/python-publish.ymlto:tests/release_sanity_check.pybefore publishingWhy
Issue #958 mentions multiple broken releases caused by
pytestleaking into package code and only being discovered afterpip install qdrant-clientin a fresh environment. This check makes that failure mode visible in CI before publish.Closes #958