Add negative test for allowedOrganization#1004
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/wip', '/verified', '/hold', '/lgtm', '/build-push-pr-image'} |
📝 WalkthroughWalkthroughConsolidates source error assertions into a new helper, moves Hugging Face negative cases into a new test module, renames a default catalog test, and adds a post-teardown Model Catalog API readiness wait in the catalog config fixture. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py (2)
🪛 Ruff (0.14.11)tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.py83-83: Unused method argument: (ARG002) tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py123-123: 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: 0
🧹 Nitpick comments (1)
tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.py (1)
159-173: Reduce test flakiness and brittle error matchingThe test case uses
allowedOrganization: "abc-random", which could become a real HuggingFace organization in the future, causing this negative test to unexpectedly pass. Additionally, since the assertion uses substring matching (in), you can safely match only the stable suffix ("no models found") instead of the full error message, making the test less sensitive to error message prefix changes.Suggested improvements
pytest.param( """ catalogs: - name: HuggingFace Hub id: error_catalog type: hf enabled: true properties: - allowedOrganization: "abc-random" + allowedOrganization: "this-org-should-not-exist-000000000000000000" includedModels: - '*' """, - "failed to expand model patterns: no models found", + "no models found", id="test_hf_source_non_existent_allowed_organization", ),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.py
tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.py
Outdated
Show resolved
Hide resolved
fege
left a comment
fege
left a comment
There was a problem hiding this comment.
move the HF test in the HF folder
tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py:
- Around line 22-33: Remove the duplicate pytest.param entry with id
"test_source_error_invalid_path" from the HuggingFace negative test file (the
pytest.param that defines a YAML catalog with type: yaml and yamlCatalogPath:
non-existent-catalog.yaml); this case tests generic YAML catalog error handling
and is already covered in test_model_catalog_negative.py, so delete that
pytest.param from
tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py
(inside the failing_cases/param list) so the file only contains
HuggingFace-specific negative tests.
🧹 Nitpick comments (2)
tests/model_registry/model_catalog/utils.py (2)
256-261: Add return type annotation for completeness.The function is missing a return type annotation. For consistency with the codebase's type hints:
Suggested fix
def assert_source_error_state_message( model_catalog_rest_url: list[str], model_registry_rest_headers: dict[str, str], expected_error_message: str, source_id: str, -): +) -> None:
262-268: Consider defensive handling for API response.Direct access to
["items"]will raise aKeyErrorif the API returns an unexpected response format. Consider using.get("items", [])for more resilient behavior:Suggested fix
results = execute_get_command( url=f"{model_catalog_rest_url[0]}sources", headers=model_registry_rest_headers, - )["items"] + ).get("items", []) + assert results, "No items returned from sources API"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.pytests/model_registry/model_catalog/conftest.pytests/model_registry/model_catalog/huggingface/test_huggingface_negative.pytests/model_registry/model_catalog/utils.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/model_registry/model_catalog/utils.py (3)
tests/model_registry/model_catalog/conftest.py (1)
model_catalog_rest_url(413-422)tests/model_registry/conftest.py (1)
model_registry_rest_headers(399-400)tests/model_registry/utils.py (1)
execute_get_command(730-738)
tests/model_registry/model_catalog/conftest.py (2)
tests/model_registry/utils.py (1)
wait_for_model_catalog_api(718-727)tests/model_registry/conftest.py (1)
model_registry_rest_headers(399-400)
tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py (3)
tests/model_registry/model_catalog/utils.py (1)
assert_source_error_state_message(256-272)tests/model_registry/model_catalog/conftest.py (2)
updated_catalog_config_map_scope_function(382-397)model_catalog_rest_url(413-422)tests/model_registry/conftest.py (1)
model_registry_rest_headers(399-400)
🪛 Ruff (0.14.10)
tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.py
83-83: Unused method argument: updated_catalog_config_map_scope_function
(ARG002)
tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py
135-135: Unused method argument: updated_catalog_config_map_scope_function
(ARG002)
🔇 Additional comments (3)
tests/model_registry/model_catalog/conftest.py (1)
396-397: LGTM!Adding the API readiness wait after teardown aligns with the pattern used in other fixtures (
updated_catalog_config_map,sparse_override_catalog_source) and ensures subsequent tests don't encounter race conditions from incomplete API initialization.tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.py (1)
81-93: LGTM! Good refactoring to use the shared helper.The test is correctly simplified to use
assert_source_error_state_message, reducing code duplication.Regarding the static analysis hint about
updated_catalog_config_map_scope_functionbeing unused: this is a false positive. The fixture is an indirect parameterized fixture that pytest invokes to set up the ConfigMap configuration before the test runs. It must be declared as a test parameter even though the test body doesn't reference it directly.tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py (1)
133-145: Good coverage of HuggingFace negative scenarios.The test method correctly uses the shared
assert_source_error_state_messagehelper. The parameterized cases provide comprehensive coverage of invalid HuggingFace catalog configurations including:
- Missing
includedModels- Various invalid wildcard patterns
- Non-existent
allowedOrganizationRegarding the static analysis hint about
updated_catalog_config_map_scope_functionbeing unused: this is a false positive – the fixture is invoked indirectly by pytest to configure the ConfigMap before each test case.
tests/model_registry/model_catalog/huggingface/test_huggingface_negative.py
Outdated
Show resolved
Hide resolved
|
Status of building tag latest: success. |
* Add negative test for allowedOrganization * Move HF tests to separate location
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.