Remove unused fixture and update tox to include fixtures that are not easy to find#522
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/verified', '/build-push-pr-image', '/cherry-pick', '/hold', '/lgtm'} |
📝 WalkthroughWalkthroughRemoved a pytest fixture from tests/conftest.py and updated tox.ini’s unused code check configuration by consolidating and expanding excluded function prefixes. No other files or public interfaces were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tox.ini (3)
14-14: Pin or bound python-utility-scripts to a version that supports these flags.Unpinned deps can break this env if the CLI behavior/flags change (e.g., around RedHatQE/python-utility-scripts#203). Recommend pinning or setting a minimum version once verified.
After confirming which version introduced the desired behavior, update deps accordingly. Example (adjust version as appropriate):
[testenv:unused-code] deps = python-utility-scripts>=X.Y.ZIf you want, I can look up the exact released version that includes the needed behavior and propose the precise constraint.
14-14: Reduce maintenance burden by factoring excludes into a variable.The long, quoted CSV is hard to maintain and review. Factor it into an env var so changes are localized and readable.
Apply this change to the command (within the current line):
- pyutils-unusedcode --exclude-function-prefixes "pytest_,fail_if_missing_dependent_operators,enabled_kserve_in_dsc,enabled_modelmesh_in_dsc,http_s3_ovms_external_route_model_mesh_serving_runtime" --exclude-files "generation_pb2_grpc.py" + pyutils-unusedcode --exclude-function-prefixes "{env:UNUSED_EXCLUDE_PREFIXES}" --exclude-files "generation_pb2_grpc.py"And add this to setenv (outside the changed line; illustrative snippet):
setenv = PYTHONPATH = {toxinidir} UNUSED_EXCLUDE_PREFIXES = pytest_,fail_if_missing_dependent_operators,enabled_kserve_in_dsc,enabled_modelmesh_in_dsc,http_s3_ovms_external_route_model_mesh_serving_runtime
14-14: Add a short comment explaining why these are excluded (link to the upstream context).Future maintainers will benefit from knowing these are dynamically used fixtures/hooks and intentionally excluded to avoid false positives related to RedHatQE/python-utility-scripts#203.
You could add above the command:
# These fixtures/hooks are discovered/used dynamically in pytest and can appear "unused". # Exclusions prevent false positives; context: https://github.com/RedHatQE/python-utility-scripts/pull/203
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/conftest.py(0 hunks)tox.ini(1 hunks)
💤 Files with no reviewable changes (1)
- tests/conftest.py
🔇 Additional comments (3)
tox.ini (3)
14-14: Consolidating exclude prefixes into a single flag looks good.This aligns with the PR goal to prevent upcoming tox failures by making hard-to-detect fixtures ignored by the unused-code checker.
14-14: No action required — excluded prefixes match real pytest hooks/fixturesSearch results show each excluded prefix corresponds to real definitions (pytest hooks and the specific fixtures), so the exclusion does not appear to be masking unrelated symbols.
Matches found:
- tests/conftest.py:420 — enabled_modelmesh_in_dsc
- tests/conftest.py:431 — enabled_kserve_in_dsc
- tests/model_serving/model_runtime/model_validation/conftest.py:199 — pytest_generate_tests
- tests/model_serving/model_server/conftest.py:443 — http_s3_ovms_external_route_model_mesh_serving_runtime
- conftest.py:45 — pytest_addoption
- conftest.py:202 — pytest_cmdline_main
- conftest.py:206 — pytest_collection_modifyitems
- conftest.py:274 — pytest_sessionstart
- conftest.py:329 — pytest_fixture_setup
- conftest.py:333 — pytest_runtest_setup
- conftest.py:371 — pytest_runtest_call
- conftest.py:375 — pytest_runtest_teardown
- conftest.py:383 — pytest_report_teststatus
- conftest.py:401 — pytest_sessionfinish
- conftest.py:437 — pytest_exception_interact
- conftest.py:461 — fail_if_missing_dependent_operators
No unrelated matches observed; the current exclusion list is appropriate.
14-14: Verify pyutils-unusedcode --exclude-function-prefixes accepts CSV vs repeatable flagsI couldn't find the pyutils-unusedcode implementation in this repo, so please confirm the CLI syntax locally — the flag in tox.ini may be ineffective if the tool expects repeated flags instead of a comma-separated string.
- File/line to check:
- tox.ini (around line 14)
- Snippet:
pyutils-unusedcode --exclude-function-prefixes "pytest_,fail_if_missing_dependent_operators,enabled_kserve_in_dsc,enabled_modelmesh_in_dsc,http_s3_ovms_external_route_model_mesh_serving_runtime" --exclude-files "generation_pb2_grpc.py"Quick ways to verify (pick one):
- Run: pyutils-unusedcode --help — check the option description (shows “repeatable” or “comma-separated”).
- Functional test:
- CSV form: pyutils-unusedcode --exclude-function-prefixes prefix1,prefix2
- Repeatable form: pyutils-unusedcode --exclude-function-prefixes prefix1 --exclude-function-prefixes prefix2
- Compare outputs.
- Or paste the add_argument/option definition from the pyutils-unusedcode source here and I’ll confirm the exact behavior.
|
/build-push-pr-image |
|
Status of building tag pr-522: success. |
|
Status of building tag latest: success. |
5 participants
These are going to cause tox failures soon.
RedHatQE/python-utility-scripts#203
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit