test: source returned regardless of their status#996
test: source returned regardless of their status#996dbasunag merged 2 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/lgtm', '/cherry-pick', '/wip', '/build-push-pr-image', '/verified', '/hold'} |
📝 WalkthroughWalkthroughShortened class docstring and added a new pytest to the sources endpoint tests that asserts the endpoint returns both enabled and disabled sources (status and counts); other existing source-status-related tests' docstrings were refined. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/model_registry/model_catalog/metadata/test_sources_endpoint.py (4)
🪛 Ruff (0.14.10)tests/model_registry/model_catalog/metadata/test_sources_endpoint.py82-82: Unused method argument: (ARG002) 83-83: Unused method argument: (ARG002) 🔇 Additional comments (3)
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: 2
🤖 Fix all issues with AI agents
In @tests/model_registry/model_catalog/metadata/test_sources_endpoint.py:
- Around line 49-56: The test should avoid KeyError by using safe dict access
for the error field: replace direct access of source["error"] with
source.get("error") when computing error_value (in the loop over enabled_sources
that calls validate_source_status and checks error_value), and keep the existing
assertion logic and message but reference the retrieved error_value from get to
ensure missing keys are handled gracefully.
- Around line 57-71: The test may raise a KeyError by accessing
disabled_catalog["error"]; change to safe access using
disabled_catalog.get("error") when reading the error field (and update the
assertion accordingly) so the check in validate_source_status and the subsequent
assertion use the .get() accessor; keep references to disabled_catalog,
validate_source_status and LOGGER unchanged.
🧹 Nitpick comments (1)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py (1)
38-47: Consider handling unexpected status values more explicitly.The current logic assumes all sources have a status of either "available" or "disabled". If a source has a missing status field (
None) or an unexpected value, it will be excluded from both lists, causing the assertion at lines 45-47 to fail with a generic message.Consider adding explicit handling to make debugging easier if unexpected status values are encountered.
🛡️ Proposed refactor for better error handling
# Split sources by status enabled_sources = [item for item in items if item.get("status") == "available"] disabled_sources = [item for item in items if item.get("status") == "disabled"] +unexpected_sources = [ + item for item in items + if item.get("status") not in ["available", "disabled"] +] # Verify we have both enabled and disabled sources assert enabled_sources, "Expected at least one enabled source" assert disabled_sources, "Expected at least one disabled source" +assert not unexpected_sources, ( + f"Found sources with unexpected status values: " + f"{[{'id': s.get('id'), 'status': s.get('status')} for s in unexpected_sources]}" +) assert len(enabled_sources) + len(disabled_sources) == len(items), ( "All sources should have either 'available' or 'disabled' status" )
📜 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
🪛 Ruff (0.14.10)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py
21-21: Unused method argument: enabled_model_catalog_config_map
(ARG002)
🔇 Additional comments (3)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py (3)
15-15: LGTM!The updated class docstring is clear and concise.
26-37: LGTM!The test setup and initial validation are clear and correct.
17-25: No action needed. Theenabled_model_catalog_config_mapfixture is required for test setup. Although the fixture parameter is not directly referenced in the test code, it is essential for establishing preconditions: the fixture enables all catalogs in the default model catalog ConfigMap as part of its initialization. The test subsequently validates source status/error fields with this setup in place. This is a standard pytest pattern where fixtures are used for their side effects (setup/teardown) rather than for their return values.
sheltoncyril
left a comment
sheltoncyril
left a comment
There was a problem hiding this comment.
/lgtm but would have preferred a shorter name for the method
|
Status of building tag latest: success. |
* test: source returned regardless of their status * fix: add single test to retrieve disabled and enabled sources
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.