Remove enabled_model_catalog_config_map fixture and it's usage#1002
Remove enabled_model_catalog_config_map fixture and it's usage#1002dbasunag merged 8 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/wip', '/hold', '/verified', '/cherry-pick', '/lgtm'} |
📝 WalkthroughWalkthroughRemoved the enabled_model_catalog_config_map fixture and its usages across tests, deleted the get_catalog_url_and_headers utility, added "enabled": True to two default catalog entries, and added pytest.skip markers to several Hugging Face test modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
dbasunag
force-pushed
the
remove_enabled_catalog_fixture
branch
from
5dc0df6 to
61518af
Compare
|
/build-push-pr-image |
|
Status of building tag pr-1002: success. |
![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/model_registry/model_catalog/metadata/test_sources_endpoint.py:
- Around line 39-42: The test is using the skip function as a decorator
(pytest.skip(...)) which triggers module-level skip; replace the decorator with
the marker pytest.mark.skip(reason="...") on the test (or alternatively remove
the decorator and call pytest.skip("...", allow_module_level=False) inside the
test) so that only the specific test is skipped; update the decorator usage
where pytest.skip is applied to use pytest.mark.skip(...) instead.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py
🧰 Additional context used
🪛 GitHub Actions: Tox Tests
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py
[error] 1-1: pytest collection error: Using pytest.skip outside of a test will skip the entire module. Command 'uv run pytest --collect-only' exited with code 2.
🔇 Additional comments (1)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py (1)
79-108: Thedisabled_catalog_sourcefixture is self-contained and does not depend on the removedenabled_model_catalog_config_map. The fixture directly loads the sources.yaml ConfigMap, setsenabled = Falsefor the specified catalog, and patches the ConfigMap independently. The test should work correctly as written.
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py (2)
39-42: Minor: Backslash continuation embeds extra whitespace in skip reason.The backslash line continuation includes the indentation whitespace in the reason string. Consider using implicit string concatenation or a single line for cleaner output.
♻️ Suggested fix
@pytest.mark.skip( - reason="This test should be included in https://github.com/opendatahub-io/opendatahub-tests/pull/999/ \ - where we could disable a source catalog and verify it" + reason="This test should be included in https://github.com/opendatahub-io/opendatahub-tests/pull/999/ " + "where we could disable a source catalog and verify it" )
79-82: Same string formatting issue as above.Apply the same implicit string concatenation fix here for consistency.
♻️ Suggested fix
@pytest.mark.skip( - reason="This test should be included in https://github.com/opendatahub-io/opendatahub-tests/pull/999/ \ - where we could disable a source catalog and verify it" + reason="This test should be included in https://github.com/opendatahub-io/opendatahub-tests/pull/999/ " + "where we could disable a source catalog and verify it" )
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/model_registry/model_catalog/huggingface/test_huggingface_model_sorting.py (1)
8-9: Add a reason to the skip marker for maintainability.Skipping entire test modules without documenting why makes it difficult for future maintainers to understand the context and know when these tests should be re-enabled. Consider adding a
reasonparameter and optionally referencing a tracking ticket.Suggested improvement
-pytestmark = [pytest.mark.skip] +pytestmark = [pytest.mark.skip(reason="Pending fixture refactor - see RHOAIENG-XXXXX")]tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.py (1)
11-12: Add a reason to the skip marker for maintainability.Same concern as the other test modules—skipping without a documented reason makes future maintenance harder. Include a
reasonparameter explaining why these tests are disabled.Suggested improvement
-pytestmark = [pytest.mark.skip] +pytestmark = [pytest.mark.skip(reason="Pending fixture refactor - see RHOAIENG-XXXXX")]tests/model_registry/model_catalog/huggingface/test_huggingface_model_validation.py (1)
16-19: Add a reason to the skip marker for maintainability.The
usefixturesmarker is retained (presumably for when tests are re-enabled), but theskipmarker lacks areason. Document why these tests are being skipped and when they should be re-enabled.Suggested improvement
pytestmark = [ pytest.mark.usefixtures("updated_dsc_component_state_scope_session", "model_registry_namespace"), - pytest.mark.skip, + pytest.mark.skip(reason="Pending fixture refactor - see RHOAIENG-XXXXX"), ]tests/model_registry/model_catalog/huggingface/test_huggingface_exclude_models.py (1)
10-13: Add a reason to the skip marker for maintainability.Consistent with the other files in this PR, please add a
reasonparameter to document why these tests are skipped and provide a reference for tracking re-enablement.Suggested improvement
pytestmark = [ pytest.mark.usefixtures("updated_dsc_component_state_scope_session", "model_registry_namespace"), - pytest.mark.skip, + pytest.mark.skip(reason="Pending fixture refactor - see RHOAIENG-XXXXX"), ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/model_catalog/huggingface/test_huggingface_exclude_models.pytests/model_registry/model_catalog/huggingface/test_huggingface_model_search.pytests/model_registry/model_catalog/huggingface/test_huggingface_model_sorting.pytests/model_registry/model_catalog/huggingface/test_huggingface_model_validation.py
|
Status of building tag latest: success. |
…atahub-io#1002) * Remove enabled_model_catalog_config_map fixture and it's usage * test: remove catalog that are not the default one * test: skip test to include fix in a follow pr * test: skip because we need to disable the catalog source and this will be done later * fix: use the right marker --------- Co-authored-by: fege <[email protected]>
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.