Add new tests for filterQuery artifact endpoint#913
Add new tests for filterQuery artifact endpoint#913dbasunag merged 6 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/build-push-pr-image', '/verified', '/cherry-pick', '/wip', '/lgtm'} |
📝 WalkthroughWalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/conftest.py(1 hunks)tests/model_registry/model_catalog/test_model_artifact_search.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/model_registry/model_catalog/test_model_artifact_search.py (3)
tests/model_registry/model_catalog/utils.py (4)
fetch_all_artifacts_with_dynamic_paging(791-822)validate_model_artifacts_match_criteria_and(1088-1106)validate_model_artifacts_match_criteria_or(1109-1125)ResourceNotFoundError(32-33)tests/model_registry/model_catalog/conftest.py (1)
randomly_picked_model_from_catalog_api_by_source(177-225)tests/model_registry/conftest.py (2)
model_catalog_rest_url(639-648)model_registry_rest_headers(323-324)
tests/model_registry/model_catalog/conftest.py (2)
tests/model_registry/conftest.py (2)
model_registry_rest_headers(323-324)model_catalog_rest_url(639-648)tests/model_registry/utils.py (2)
get_rest_headers(670-675)execute_get_command(731-739)
🪛 Ruff (0.14.7)
tests/model_registry/model_catalog/test_model_artifact_search.py
45-45: Unused method argument: enabled_model_catalog_config_map
(ARG002)
146-146: Unused method argument: enabled_model_catalog_config_map
(ARG002)
192-192: Avoid specifying long messages outside the exception class
(TRY003)
199-199: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
tests/model_registry/model_catalog/conftest.py
212-212: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
🔇 Additional comments (5)
tests/model_registry/model_catalog/conftest.py (1)
195-216: LGTM!The conditional model selection logic is well-structured. When no model_name is provided, it correctly fetches models from the catalog, validates the response, and randomly selects one for testing. The use of
random.choiceis appropriate here for non-cryptographic test data selection.tests/model_registry/model_catalog/test_model_artifact_search.py (4)
45-46: Fixture used for setup dependency — no issue.The
enabled_model_catalog_config_mapfixture parameter ensures the catalog configuration is properly set up before tests execute, even though the value isn't accessed directly. This is a common pytest pattern for fixture dependencies.
21-42: Well-structured parametrized test for invalid filter queries.Good coverage of different invalid filter query scenarios: malformed syntax, data type mismatches for comparison and equality operations. The use of indirect fixtures for model/catalog selection is appropriate.
72-143: Comprehensive test cases for filter query logic.The parameterization covers important scenarios: no-results queries, single-criteria filters, multi-criteria AND operations, and OR operations. The expected_value and logic_type parameters enable flexible validation.
43-70: Verify thatResourceNotFoundErroris the correct exception type for HTTP-based artifact search.The test expects
ResourceNotFoundErrorfromkubernetes.dynamic.exceptions, butfetch_all_artifacts_with_dynamic_pagingperforms HTTP requests viaexecute_get_command. HTTP utilities typically raise HTTP-related exceptions (e.g., from therequestslibrary) orjson.JSONDecodeError, not Kubernetes client exceptions. Confirm whether this function translates HTTP errors toResourceNotFoundErroror if a different exception type should be caught.
tests/model_registry/model_catalog/test_model_artifact_search.py
Outdated
Show resolved
Hide resolved
dbasunag
force-pushed
the
artifact_tests
branch
from
16623d0 to
6aa144d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/model_registry/model_catalog/test_model_artifact_search.py (2)
194-200: Good use ofpytest.failfor explicit test failuresReplacing
assert Falsewithpytest.fail(...)makes the failure explicit and robust even under optimized Python runs, while keeping the message clear.
170-176: Assertion comparing length to[]is incorrect and breaks the no‑results test
len(result["items"])returns anint, solen(result["items"]) == []is alwaysFalse, causing the “no_results” test case to fail even when the API behaves correctly. For the no‑results scenario you want to assert that the returned list itself is empty.- if expected_value is None: - # Simple validation of length and size for basic filter queries - assert len(result["items"]) == [], f"Filter query '{filter_query}' should return valid results" - assert result["size"] == 0, f"Size should be 0 for filter query '{filter_query}'" + if expected_value is None: + # Validate that query returns no results for the "no_results" test case + assert result["items"] == [], f"Filter query '{filter_query}' should return no results" + assert result["size"] == 0, f"Size should be 0 for filter query '{filter_query}'" LOGGER.info( f"Successfully validated that filter query '{filter_query}' returns {len(result['items'])} artifacts" )
🧹 Nitpick comments (3)
tests/model_registry/model_catalog/conftest.py (2)
195-224: Targeted vs random model selection logic looks solidThe new
model_name-aware branch correctly preserves existing random behavior while enabling deterministic selection via/sources/{catalog_id}/models/{model_name}, and the fixed URL avoids the previous double‑slash issue. As an optional refinement, consider usingif model_name is None:instead ofif not model_name:to avoid treating an empty string as “no model specified”.
254-277: Filter‑query fixture is straightforward and reusable
models_from_filter_querycleanly wrapsget_models_from_catalog_apiand enforces that provided filter queries return at least one model, which fits positive test cases. If you later need to test “no‑result” filters, you may want a separate fixture (or a flag) that doesn’t assert non‑emptiness.tests/model_registry/model_catalog/test_model_artifact_search.py (1)
44-50: Make intentional use ofenabled_model_catalog_config_mapto satisfy lintersThe
enabled_model_catalog_config_mapparameter is intentionally unused (you rely on the fixture side effects), which triggers ARG002 from Ruff. To make this intent explicit and silence the warning without changing behavior, you can reference it in a no‑op assignment in both tests:def test_search_artifacts_by_invalid_filter_query( self: Self, enabled_model_catalog_config_map: ConfigMap, @@ - ): + ): """ Tests the API's response to invalid filter queries syntax when searching artifacts. It verifies that an invalid filter query syntaxt raises the correct error. """ + # Ensure fixture is evaluated; variable itself is unused by design. + _ = enabled_model_catalog_config_map @@ def test_filter_query_advanced_artifact_search( self: Self, enabled_model_catalog_config_map: ConfigMap, @@ - ): + ): """ Advanced filter query test for artifact-based filtering with AND/OR logic """ + # Ensure fixture is evaluated; variable itself is unused by design. + _ = enabled_model_catalog_config_mapAlso applies to: 145-153
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/conftest.py(1 hunks)tests/model_registry/model_catalog/test_model_artifact_search.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/model_registry/model_catalog/test_model_artifact_search.py (2)
tests/model_registry/model_catalog/utils.py (4)
fetch_all_artifacts_with_dynamic_paging(791-822)validate_model_artifacts_match_criteria_and(1088-1106)validate_model_artifacts_match_criteria_or(1109-1125)ResourceNotFoundError(32-33)tests/model_registry/model_catalog/conftest.py (1)
randomly_picked_model_from_catalog_api_by_source(177-225)
tests/model_registry/model_catalog/conftest.py (2)
tests/model_registry/conftest.py (2)
model_registry_rest_headers(323-324)model_catalog_rest_url(639-648)tests/model_registry/utils.py (2)
get_rest_headers(670-675)execute_get_command(731-739)
🪛 Ruff (0.14.7)
tests/model_registry/model_catalog/test_model_artifact_search.py
45-45: Unused method argument: enabled_model_catalog_config_map
(ARG002)
146-146: Unused method argument: enabled_model_catalog_config_map
(ARG002)
192-192: Avoid specifying long messages outside the exception class
(TRY003)
tests/model_registry/model_catalog/conftest.py
212-212: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
tests/model_registry/model_catalog/test_model_artifact_search.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/conftest.py(1 hunks)tests/model_registry/model_catalog/test_model_artifact_search.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/conftest.py (2)
tests/model_registry/conftest.py (2)
model_registry_rest_headers(323-324)model_catalog_rest_url(639-648)tests/model_registry/utils.py (2)
get_rest_headers(670-675)execute_get_command(731-739)
🪛 Ruff (0.14.7)
tests/model_registry/model_catalog/conftest.py
216-216: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
tests/model_registry/model_catalog/test_model_artifact_search.py
45-45: Unused method argument: enabled_model_catalog_config_map
(ARG002)
146-146: Unused method argument: enabled_model_catalog_config_map
(ARG002)
192-192: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
tests/model_registry/model_catalog/conftest.py (1)
183-228: LGTM! Clean implementation of optional model selection.The two-path logic is well-structured: when
model_nameis provided, the fixture validates the specific model exists in the catalog; otherwise, it maintains the original random selection behavior. The URL construction on line 224 is now correct without the double-slash issue.tests/model_registry/model_catalog/test_model_artifact_search.py (2)
21-70: Test structure looks good.The parameterized invalid filter query tests correctly expect
ResourceNotFoundErrorwith proper error matching. The test cases cover malformed syntax, data type mismatches, and type equality violations. The fixture dependency pattern forenabled_model_catalog_config_mapis acceptable—it ensures the catalog is properly configured before tests run, even though the fixture value isn't directly referenced.
144-200: Advanced filter test structure is well-designed.Good use of parameterization with
logic_typeto select the appropriate validation function. The use ofpytest.fail()on line 200 correctly handles test failures.One minor note: the validation on lines 194-200 could be simplified by asserting directly on the validation result, but the current structure with explicit logging is acceptable for debugging clarity.
dbasunag
force-pushed
the
artifact_tests
branch
from
80ec544 to
25236f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_registry/model_catalog/test_model_artifact_search.py (1)
36-39: Remove unintendedfilterQuery=prefix causing double encoding.The filter query value on line 38 includes a
filterQuery=prefix, but line 68 constructs the URL asf"filterQuery={invalid_filter_query}", resulting infilterQuery=filterQuery=ttft_p90.double_value < abc. This changes the test from validating data-type mismatch to validating malformed syntax (duplicate parameter prefix).Apply this diff to fix the test:
pytest.param( {"catalog_id": VALIDATED_CATALOG_ID, "header_type": "registry"}, - "filterQuery=ttft_p90.double_value < abc", + "ttft_p90.double_value < abc", id="test_invalid_artifact_filter_query_data_type_mismatch", ),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/conftest.py(1 hunks)tests/model_registry/model_catalog/test_model_artifact_search.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/conftest.py (2)
tests/model_registry/conftest.py (2)
model_registry_rest_headers(323-324)model_catalog_rest_url(639-648)tests/model_registry/utils.py (2)
get_rest_headers(670-675)execute_get_command(731-739)
🪛 Ruff (0.14.7)
tests/model_registry/model_catalog/conftest.py
215-215: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
tests/model_registry/model_catalog/test_model_artifact_search.py
51-51: Unused method argument: enabled_model_catalog_config_map
(ARG002)
85-85: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
96-96: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
107-107: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
118-118: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
132-132: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
152-152: Unused method argument: enabled_model_catalog_config_map
(ARG002)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
tests/model_registry/model_catalog/conftest.py (1)
206-229: LGTM! Clean two-path implementation.The conditional logic correctly handles both random selection and pre-selected model scenarios. The URL construction is correct (previous double-slash issue was resolved), and both paths validate the catalog association appropriately.
tests/model_registry/model_catalog/test_model_artifact_search.py (4)
1-24: LGTM! Clean test setup.Imports and test constants are well-organized. The model list provides good variety for testing artifact search functionality.
49-77: LGTM! Invalid query validation is well-structured.The test correctly validates that invalid filter queries raise
ResourceNotFoundError. The unused argument warning forenabled_model_catalog_config_mapis a false positive—the fixture has necessary side effects for test setup.
78-149: LGTM! Comprehensive test parametrization.The test cases cover a good range of scenarios: no results, simple filters, AND logic, and OR logic. The
random.choice()calls at parametrization time ensure each test case picks a model during collection, which is appropriate for test variety. (The S311 static analysis warnings are false positives—this is test code, not cryptographic usage.)
150-206: LGTM! Test implementation is solid.The test correctly handles both no-results validation and criteria matching with AND/OR logic. Past issues have been resolved:
- Line 178 now correctly uses
result["items"] == []instead oflen(result["items"]) == []- Line 206 now uses
pytest.fail()instead ofassert FalseThe defensive ValueError for invalid logic_type is good practice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_registry/model_catalog/conftest.py (1)
187-193: Documentation still incorrectly listsmodel_nameunderheader_type.Line 190 continues to list
'model_name': Name of the modelas if it's a value for theheader_typeparameter, butmodel_nameis an independent fixture parameter. This creates confusion about the fixture's API.Apply this diff to fix the documentation:
Supports parameterized headers via 'header_type': - 'user_token': Uses user_token_for_api_calls (default for user-specific tests) - 'registry': Uses model_registry_rest_headers (for catalog/registry tests) - - 'model_name': Name of the model - Accepts 'catalog_id' or 'source' (alias) to specify the catalog. Accepts 'model_name' to specify the model to - look for. + Additional parameters: + - 'catalog_id' or 'source' (alias): Specify the catalog to query + - 'model_name': Optional model name to retrieve (skips random selection)
🧹 Nitpick comments (1)
tests/model_registry/model_catalog/test_model_artifact_search.py (1)
18-24: Fix inconsistent variable naming.The variable name
MODEL_NAMEs_ARTIFACT_SEARCHhas inconsistent capitalization (NAMEswith capitalNAMEand lowercases). This violates Python naming conventions for constants.Apply this diff:
-MODEL_NAMEs_ARTIFACT_SEARCH: list[str] = [ +MODEL_NAMES_ARTIFACT_SEARCH: list[str] = [ "RedHatAI/Llama-3.1-8B-Instruct", "RedHatAI/Mistral-Small-3.1-24B-Instruct-2503-FP8-dynamic", "RedHatAI/Mistral-Small-3.1-24B-Instruct-2503-quantized.w4a16", "RedHatAI/Mistral-Small-3.1-24B-Instruct-2503-quantized.w8a8", "RedHatAI/Mixtral-8x7B-Instruct-v0.1", ]Then update all references on lines 85, 96, 107, 118, and 132.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/conftest.py(1 hunks)tests/model_registry/model_catalog/test_model_artifact_search.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/test_model_artifact_search.py (2)
tests/model_registry/model_catalog/utils.py (3)
fetch_all_artifacts_with_dynamic_paging(791-822)validate_model_artifacts_match_criteria_and(1088-1106)validate_model_artifacts_match_criteria_or(1109-1125)tests/model_registry/model_catalog/conftest.py (2)
enabled_model_catalog_config_map(42-78)randomly_picked_model_from_catalog_api_by_source(177-229)
🪛 Ruff (0.14.7)
tests/model_registry/model_catalog/conftest.py
215-215: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
tests/model_registry/model_catalog/test_model_artifact_search.py
51-51: Unused method argument: enabled_model_catalog_config_map
(ARG002)
85-85: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
96-96: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
107-107: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
118-118: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
132-132: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
152-152: Unused method argument: enabled_model_catalog_config_map
(ARG002)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
tests/model_registry/model_catalog/conftest.py (1)
199-229: LGTM! The dual-path logic for optional model selection is well-implemented.The addition of the
model_nameparameter elegantly extends the fixture to support both random model selection (original behavior) and targeted model retrieval. The validation logic correctly ensures the model exists and belongs to the specified catalog in both paths.tests/model_registry/model_catalog/test_model_artifact_search.py (2)
49-76: LGTM! Invalid filter query tests are well-structured.The test correctly validates that malformed and type-mismatched filter queries raise
ResourceNotFoundErrorwith the expected error message. The test coverage includes syntax errors and type mismatches.
150-206: LGTM! Advanced filter query validation is comprehensive and correct.The test effectively covers multiple scenarios:
- No-results case (lines 176-182)
- Single-condition filters with AND logic
- Multi-condition filters with AND/OR operators
- Proper validation using dedicated helper functions
The control flow correctly handles both empty and populated result sets, with appropriate validation based on
logic_type.
|
Status of building tag latest: success. |
* Add new tests for filterQuery artifact endpoint * addressed review comments * Add a list of models for pytest.param
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.