test: Add test for merge catalog sources#999
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/wip', '/hold', '/lgtm', '/build-push-pr-image', '/cherry-pick'} |
📝 WalkthroughWalkthroughThe PR introduces a data-driven test for catalog source merge validation with sparse overrides, adds a reusable fixture to apply temporary field overrides via ConfigMap modifications, removes the obsolete disabled_catalog_source fixture, and migrates two existing tests to use the new fixture pattern. 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 |
![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_catalog_source_merge.py (1)
24-24: Minor formatting: add space after colon in ticket reference.The JIRA ticket reference should have a space after the colon for consistency.
✨ Suggested fix
- RHOAIENG-41738:Test that a sparse override in custom ConfigMap successfully overrides + RHOAIENG-41738: Test that a sparse override in custom ConfigMap successfully overrides
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/catalog_config/conftest.pytests/model_registry/model_catalog/catalog_config/test_catalog_source_merge.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/catalog_config/test_catalog_source_merge.py (4)
tests/model_registry/utils.py (1)
execute_get_command(730-738)tests/model_registry/model_catalog/catalog_config/conftest.py (1)
sparse_override_catalog_source(20-77)tests/model_registry/model_catalog/conftest.py (1)
model_catalog_rest_url(405-414)tests/model_registry/conftest.py (1)
model_registry_rest_headers(399-400)
🔇 Additional comments (9)
tests/model_registry/model_catalog/catalog_config/test_catalog_source_merge.py (4)
1-9: LGTM!Imports and module-level fixture markers are correctly configured.
31-43: LGTM!Fixture data extraction and API query logic are correct. The merged catalog lookup includes a clear assertion message.
46-72: LGTM!The validation logic correctly distinguishes between overridden fields (name, labels) and preserved fields, accumulating detailed error messages for comprehensive test feedback.
74-78: LGTM!Summary logging provides clear visibility into which fields were overridden versus preserved, aiding in test debugging and validation.
tests/model_registry/model_catalog/catalog_config/conftest.py (5)
1-17: LGTM!All required imports are present and properly organized.
19-31: LGTM!Function scope is appropriate for this fixture since it modifies ConfigMap state, ensuring proper test isolation.
33-39: LGTM!Capturing the original catalog state before applying the override is essential for validating that unspecified fields are correctly preserved during the merge.
41-56: LGTM!The sparse YAML override is well-crafted to test merge behavior. Including only
id,name, andlabelsensures that other fields must be inherited from the default ConfigMap.
58-77: LGTM!Excellent use of ResourceEditor context manager for automatic cleanup and proper wait operations to ensure API stability before and after the test. This pattern prevents race conditions and ensures test isolation.
tests/model_registry/model_catalog/catalog_config/test_catalog_source_merge.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/model_registry/model_catalog/catalog_config/test_catalog_source_merge.py (1)
74-78: Consider edge case: what iforiginal_cataloghas unexpected fields?The log message assumes
len(original_catalog) - 1preserved fields, but if the API response (merged_catalog) contains additional fields not present inoriginal_catalog, they won't be validated. This is likely acceptable for this test's scope, but worth noting if the API schema could evolve.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/catalog_config/conftest.pytests/model_registry/model_catalog/catalog_config/test_catalog_source_merge.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/model_catalog/catalog_config/conftest.py
🔇 Additional comments (5)
tests/model_registry/model_catalog/catalog_config/test_catalog_source_merge.py (5)
1-9: LGTM!The imports, logger setup, and module-level pytest markers are appropriate for this test module.
18-26: Good parametrization approach for data-driven testing.The use of
indirect=Truecorrectly delegates parameter handling to thesparse_override_catalog_sourcefixture. The test covers multiple field types (string, list, boolean) which is good for testing merge behavior across different data types.
42-49: Test query and lookup logic is well-structured.The approach of querying the sources endpoint, finding the specific catalog by ID, and asserting its presence is correct. Using
next()with aNonedefault followed by an explicit assertion provides a clear error message when the catalog is missing.
56-72: Good handling of field preservation validation with special case forenabled.The logic correctly:
- Excludes the overridden field from preservation checks.
- Handles the
enabled=Falsespecial case wherestatusautomatically becomes "disabled".- Iterates over remaining fields to verify they retain original values.
Using
is Falseon line 60 is the correct strict boolean comparison.
30-31: The fixturemodel_catalog_rest_urlincludes assertions that guarantee the returned list is never empty, making the[0]access at line 43 safe. The type hintlist[str]is appropriate for representing multiple catalog API URLs across routes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py (1)
80-113: Fix false-positive risk + address Ruff ARG002 by asserting the specific catalog is disabled.
Currently this can pass if any source is disabled (even ifREDHAT_AI_CATALOG_IDwasn’t actually disabled).Proposed diff
def test_sources_endpoint_returns_all_sources_regardless_of_enabled_field( self, sparse_override_catalog_source: dict, model_catalog_rest_url: list[str], model_registry_rest_headers: dict[str, str], ): @@ response = execute_get_command(url=f"{model_catalog_rest_url[0]}sources", headers=model_registry_rest_headers) items = response.get("items", []) assert len(items) > 1, "Expected multiple sources to be returned" + catalog_id = sparse_override_catalog_source["catalog_id"] + target = next((item for item in items if item.get("id") == catalog_id), None) + assert target is not None, f"Expected catalog '{catalog_id}' to be present in sources response" + validate_source_status(catalog=target, expected_status="disabled") + # Verify we have at least one enabled source enabled_sources = [item for item in items if item.get("status") == "available"] assert enabled_sources, "Expected at least one enabled source" # Verify we have at least one disabled source disabled_sources = [item for item in items if item.get("status") == "disabled"] assert disabled_sources, "Expected at least one disabled source"
🤖 Fix all issues with AI agents
In @tests/model_registry/model_catalog/conftest.py:
- Around line 65-98: The fixture currently restarts pods and waits for the API
but does not confirm the sparse ConfigMap override was reconciled; update the
code after writing the ConfigMap inside the ResourceEditor so that, before
yielding, you poll the /sources API (using execute_get_command with
model_catalog_rest_url[0] and model_registry_rest_headers) until you find the
item with id == catalog_id and that item's field_name equals field_value (use
the same TimeoutSampler pattern used in other tests like
test_labels_endpoint.py/test_default_model_catalog.py); keep existing calls to
is_model_catalog_ready and wait_for_model_catalog_api but add this
TimeoutSampler-based assertion referencing catalog_id, field_name, and
field_value to ensure the override is visible in the API before yield.
🧹 Nitpick comments (1)
tests/model_registry/model_catalog/conftest.py (1)
41-64: Harden fixture parameter validation (avoid noisy KeyError / IndexError).
Right now missing keys (or emptymodel_catalog_rest_url) will fail with less actionable errors.Proposed diff
@pytest.fixture() def sparse_override_catalog_source( request: pytest.FixtureRequest, admin_client, model_registry_namespace: str, model_catalog_rest_url: list[str], model_registry_rest_headers: dict[str, str], ) -> Generator[dict, None, None]: @@ # Get fields from pytest param param = getattr(request, "param", None) - assert param, "sparse_override_catalog_source requires request.param dict" + if not isinstance(param, dict): + raise pytest.UsageError("sparse_override_catalog_source requires request.param dict") + missing = {"id", "field_name", "field_value"} - set(param) + if missing: + raise pytest.UsageError(f"sparse_override_catalog_source missing request.param keys: {sorted(missing)}") + assert model_catalog_rest_url, "model_catalog_rest_url is empty" + base_url = model_catalog_rest_url[0] - catalog_id = param["id"] - field_name = param["field_name"] - field_value = param["field_value"] + catalog_id = param["id"] + field_name = param["field_name"] + field_value = param["field_value"]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_registry/model_catalog/conftest.pytests/model_registry/model_catalog/metadata/conftest.pytests/model_registry/model_catalog/metadata/test_sources_endpoint.py
💤 Files with no reviewable changes (1)
- tests/model_registry/model_catalog/metadata/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py (1)
tests/model_registry/model_catalog/conftest.py (1)
sparse_override_catalog_source(42-97)
🪛 Ruff (0.14.10)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py
88-88: Unused method argument: sparse_override_catalog_source
(ARG002)
🔇 Additional comments (1)
tests/model_registry/model_catalog/metadata/test_sources_endpoint.py (1)
40-78: Good migration to sparse override fixture + targeted disabled-source validation.
The test now validates the specific catalog by id (vs relying on global state).
|
Status of building tag latest: success. |
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.