Fix release package test dependencies#72
Conversation
6e56de4 to
2a10859
Compare
2a10859 to
89338c7
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Review: Fix release package test dependencies
Overall: correct fix, one concern worth addressing.
What the PR does
Replaces hard-coded in the isolated test venv step with uv pip install --group dev, so each plugin's declared dev dependencies are used instead of a shared static list. Root cause was pydantic missing for secrets_detection tests.
Good
working-directory: ${{ needs.resolve.outputs.plugin_path }}is already set on the test step, souv pip install --group devreads the correct plugin'spyproject.toml. No--projectflag needed.- All 6 Rust Python packages already declare
pytest>=8.0andpytest-asyncio>=0.23in their dev groups — no regression risk there. - Test assertions in
test_plugin_catalog.pymatch the existing string-check pattern in that file.
Concern: PyYAML silently dropped for 5 of 6 plugins
The old hard-coded install included PyYAML. Only secrets_detection carries PyYAML>=6.0 in its dev group. The other 5 plugins (encoded_exfil_detection, pii_filter, rate_limiter, retry_with_backoff, url_reputation) do not.
If the shared test helpers copied into the isolated venv — plugin_hooks.py or conftest.py — import yaml, those five plugin release tests will fail after this merges.
Please verify plugins/tests/plugin_hooks.py and plugins/tests/conftest.py do not import yaml. If they do, either:
- Add
PyYAMLto each affected plugin's dev group, or - Add it back as an explicit install alongside
--group dev:uv pip install --python "${venv_python}" --group dev PyYAML "${venv_python}" -m pip install dist/*.whl
The assertNotIn('pytest pytest-asyncio PyYAML', workflow) test assertion passes either way, so it won't catch this regression.
msureshkumar88
left a comment
There was a problem hiding this comment.
Review: Fix release package test dependencies
Overall: correct fix, one concern worth addressing.
What the PR does
Replaces hard-coded pytest pytest-asyncio PyYAML in the isolated test venv step with uv pip install --group dev, so each plugin's declared dev dependencies are used instead of a shared static list. Root cause was pydantic missing for secrets_detection tests.
Good
working-directory: ${{ needs.resolve.outputs.plugin_path }}is already set on the test step, souv pip install --group devreads the correct plugin'spyproject.toml. No--projectflag needed.- All 6 Rust Python packages already declare
pytest>=8.0andpytest-asyncio>=0.23in their dev groups — no regression risk there. - Test assertions in
test_plugin_catalog.pymatch the existing string-check pattern in that file.
Concern: PyYAML silently dropped for 5 of 6 plugins
The old hard-coded install included PyYAML. Only secrets_detection carries PyYAML>=6.0 in its dev group. The other 5 plugins (encoded_exfil_detection, pii_filter, rate_limiter, retry_with_backoff, url_reputation) do not.
If the shared test helpers copied into the isolated venv — plugin_hooks.py or conftest.py — import yaml, those five plugin release tests will fail after this merges.
Please verify plugins/tests/plugin_hooks.py and plugins/tests/conftest.py do not import yaml. If they do, either:
- Add
PyYAMLto each affected plugin's dev group, or - Add it back as an explicit install alongside
--group dev:uv pip install --python "${venv_python}" --group dev PyYAML "${venv_python}" -m pip install dist/*.whl
The assertNotIn('pytest pytest-asyncio PyYAML', workflow) test assertion passes either way, so it won't catch this regression.
Signed-off-by: lucarlig <luca.carlig@ibm.com>
89338c7 to
6792807
Compare
|
@msureshkumar88 addressed your PyYAML concern. I verified uv pip install --python "${venv_python}" --group dev PyYAMLThis is applied in both wheel and sdist isolated test steps. I also updated the catalog regression test to require that exact install pattern, so we do not silently drop Local verification run:
|
msureshkumar88
left a comment
There was a problem hiding this comment.
Review follow-up
Concern from previous review (PyYAML silently dropped for 5/6 plugins) is addressed — PyYAML is now explicitly passed alongside --group dev, and the test assertion verifies it stays.
One new item to confirm before merge:
uv pip install --group dev requires uv ≥ 0.4.15. If the CI runner pins an older uv version, this will fail. Worth verifying the uv version available in the release workflow runner.
Also note: dev deps and wheel are now installed in two separate commands (uv first, then pip). This differs from the original single-pip call but should be harmless — pip resolves wheel conflicts against already-installed dev deps.
Pending CI checks (coverage, release-validation/preflight, ubuntu/secrets_detection, ubuntu/pii_filter) need to complete before merge.
Overall the fix is correct. LGTM once uv version is confirmed and CI is green.
|
@msureshkumar88 confirmed the CI runner version from the completed release-validation logs. The release workflow installs That is well above the uv pip install --python "${venv_python}" --group dev PyYAMLCurrent PR check summary is green: 34 passing, 3 skipped, 0 pending, 0 failed. |
msureshkumar88
left a comment
There was a problem hiding this comment.
Both concerns from my previous review are resolved:
- PyYAML: explicitly installed alongside
--group devin both wheel and sdist steps; catalog test asserts it stays. - uv version: release workflow pins
uv==0.9.30, well above the--group devminimum (0.4.15). Confirmed in completed release-validation logs. - CI green: 34 passing, 3 skipped, 0 failed.
Any further hardening (e.g. adding PyYAML to all 5 remaining plugin dev groups so the explicit pin can eventually be dropped) is a future enhancement, not a blocker.
LGTM. Approve to merge.
Summary
Fixes the Rust Python package release workflow isolated package tests by installing each plugin's declared
devdependency group into the temporary test virtualenv before installing the built wheel or sdist.PyYAMLremains explicitly installed because the shared release test helper importsyaml.Also makes PR release validation target actual plugin version bumps: when a plugin
Cargo.tomlversion changes, CI now runs the reusable release workflow for that plugin's computed release tag, for examplerate-limiter-v0.0.2.Root Cause
The
secrets-detection-v0.2.1release failed inbuild-sdistand wheel jobs because the isolated test step installed onlypytest pytest-asyncio PyYAML. The checked-insecrets_detectiontest helpers importpydantic, which is declared in the plugin'sdevdependency group but was not installed by the release workflow test environment.The previous PR release-validation check also used a fixed sample tag, so it did not necessarily validate the plugin being version-bumped in a PR.
Impact
Release package tests now use the per-plugin test dependencies already declared in
pyproject.toml, avoiding most hard-coded shared test dependency drift while still preservingPyYAMLfor shared helper compatibility.Version-bump PRs now run release-validation against the bumped plugin package before the release tag is created.
Regression Coverage
The CI-run plugin catalog suite now asserts that:
devdependency group plusPyYAMLpytest pytest-asyncio PyYAMLlistci-selectionemits release-validation tags for plugin version bumpsVerification
plugins/tests/plugin_hooks.pyimportsyamluv pip install --python ... --group dev --dry-runincludespydantic74 passed74 passedpython3 -m unittest tests/test_plugin_catalog.pypython3 tools/plugin_catalog.py ci-selection . diff origin/main HEAD | python3 tools/validate_ci_selection.pygit diff --checkNote
The failed tag run checked out the old workflow from tag
secrets-detection-v0.2.1. After this merges, rerun viaworkflow_dispatchfrommainwith that tag.