ModelCatalog: update default tests to reflect expected behavior#578
ModelCatalog: update default tests to reflect expected behavior#578dbasunag merged 3 commits intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughWalkthroughRenames a custom catalog test class and reduces fixtures; significantly rewrites default catalog tests to validate resources and ConfigMap contents via DynamicClient and parameterization; adds constants and two test utility validators for resource existence and default catalog structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
dbasunag
force-pushed
the
update_mc_tests
branch
from
5d24e78 to
7396b8a
Compare
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/hold', '/wip', '/lgtm', '/cherry-pick', '/verified'} |
![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
🧹 Nitpick comments (5)
tests/model_registry/model_catalog/test_custom_model_catalog.py (1)
34-34: Class rename looks good; drop redundant fixture usage and confirm Self typing support
- The rename to TestModelCatalogCustom is fine.
- Minor: updated_catalog_config_map is already provided via class-level parametrize and each test’s signature; having it also in @pytest.mark.usefixtures is redundant. Safe to remove from usefixtures to avoid confusion.
- Ensure the CI Python version is 3.11+. If not, add a typing_extensions fallback for Self in this file.
Apply this diff to remove the redundant fixture in the class-level usefixtures:
@pytest.mark.usefixtures( "model_registry_namespace", - "updated_catalog_config_map", ) class TestModelCatalogCustom:If Python < 3.11 is possible, add this near the imports:
-from typing import Self +try: + from typing import Self +except ImportError: # Python < 3.11 + from typing_extensions import Self # type: ignoretests/model_registry/model_catalog/test_default_model_catalog.py (4)
3-8: Ensure compatibility for Self typing on Python < 3.11If CI isn’t guaranteed to be 3.11+, add a typing_extensions fallback.
Apply this diff:
-from typing import Self, Any +try: + from typing import Self +except ImportError: # Python < 3.11 + from typing_extensions import Self # type: ignore +from typing import Any
28-35: ConfigMap test reads well; tiny comment nit“configmaps is created” → “ConfigMap is created”.
Apply this diff:
- # Check that the default configmaps is created when model registry is + # Check that the default ConfigMap is created when model registry is
65-66: Optionally verify the env var value, not just presencevalidate_model_catalog_enabled only checks for the presence of ENABLE_MODEL_CATALOG. Consider also asserting its value is truthy (“true”/“1”).
You can update the helper in utils.py:
-def validate_model_catalog_enabled(pod: Pod) -> bool: +def validate_model_catalog_enabled(pod: Pod) -> bool: for container in pod.instance.spec.containers: for env in container.env: - if env.name == "ENABLE_MODEL_CATALOG": - return True + if env.name == "ENABLE_MODEL_CATALOG": + val = (env.value or "").strip().lower() + return val in {"true", "1", "yes"} return False
73-82: Fix assertion message to match what’s being assertedThe message mentions “custom models” while the API here returns sources. Make it explicit that only the default catalog source should exist.
Apply this diff:
- assert len(result) == 1, f"Expected no custom models to be present. Actual: {result}" + assert len(result) == 1, ( + f"Expected only the default catalog source (no custom catalogs). Actual: {result}" + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
tests/model_registry/model_catalog/test_custom_model_catalog.py(1 hunks)tests/model_registry/model_catalog/test_default_model_catalog.py(1 hunks)tests/model_registry/model_catalog/utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/model_catalog/utils.pytests/model_registry/model_catalog/test_custom_model_catalog.pytests/model_registry/model_catalog/test_default_model_catalog.py
📚 Learning: 2025-07-17T15:42:23.880Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/model_catalog/test_custom_model_catalog.py
🧬 Code graph analysis (2)
tests/model_registry/model_catalog/utils.py (1)
tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/model_catalog/test_default_model_catalog.py (4)
tests/model_registry/model_catalog/utils.py (4)
validate_model_catalog_enabled(38-43)execute_get_command(29-35)validate_model_catalog_resource(66-69)validate_default_catalog(72-76)tests/model_registry/model_catalog/conftest.py (2)
catalog_config_map(15-16)model_catalog_rest_url(27-36)tests/conftest.py (1)
admin_client(66-67)tests/model_registry/conftest.py (3)
model_registry_namespace(63-64)model_registry_operator_pod(273-280)model_registry_rest_headers(290-295)
🔇 Additional comments (2)
tests/model_registry/model_catalog/utils.py (1)
72-76: Default catalog validation is clear and aligned with expectationsAssertions for name, id, type, and yamlCatalogPath match the intended defaults.
tests/model_registry/model_catalog/test_default_model_catalog.py (1)
59-63: Label assumption verificationThis relies on resources labeled component=model-catalog. Confirm the operator consistently applies this label across Deployment/Service/Route/Pod; otherwise the lookup may miss resources.
dbasunag
force-pushed
the
update_mc_tests
branch
from
5d83b1d to
d2f172e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/model_registry/model_catalog/test_default_model_catalog.py (6)
3-7: Confirm Python version for typing.Self; optionally guard type-only imports.Self is built-in from Python 3.11+. If CI may run on 3.10, import Self from typing_extensions. Also consider moving DynamicClient under TYPE_CHECKING to avoid runtime dependency for type hints.
Example (no behavior change):
-from kubernetes.dynamic import DynamicClient +from typing import TYPE_CHECKING +try: + from typing import Self # py>=3.11 +except Exception: # pragma: no cover + from typing_extensions import Self # py<=3.10 +if TYPE_CHECKING: + from kubernetes.dynamic import DynamicClientAlso applies to: 11-18
28-35: Harden parsing of sources.yaml and improve failure diagnostics.If safe_load returns None or the key is missing, the test fails with a KeyError/TypeError. Add explicit asserts with clearer messages.
- catalogs = yaml.safe_load(catalog_config_map.instance.data["sources.yaml"])["catalogs"] - assert catalogs - assert len(catalogs) == 1, f"{catalog_config_map.name} should have 1 catalog" + assert "sources.yaml" in catalog_config_map.instance.data, ( + f"{catalog_config_map.name} missing 'sources.yaml' key" + ) + data = yaml.safe_load(catalog_config_map.instance.data["sources.yaml"]) or {} + assert "catalogs" in data, f"{catalog_config_map.name} 'sources.yaml' missing 'catalogs' section" + catalogs = data["catalogs"] + assert catalogs, f"{catalog_config_map.name} 'sources.yaml' has empty 'catalogs'" + assert len(catalogs) == 1, f"{catalog_config_map.name} should have exactly 1 catalog; got {len(catalogs)}" validate_default_catalog(default_catalog=catalogs[0])
37-57: Verify “exactly one resource” assumption, especially for Pods.validate_model_catalog_resource asserts len == 1 for all kinds. If Pod replicas can be >1, this will be flaky. Either scope the selector more narrowly for Pods or relax the cardinality in the helper to expected_count per kind.
65-66: Clarify assertion message and consider validating the value of ENABLE_MODEL_CATALOG.The helper only checks presence of the env var. Add an assertion message here; optionally extend the helper to assert truthy value (e.g., "true"/"True").
- assert validate_model_catalog_enabled(pod=model_registry_operator_pod) + assert validate_model_catalog_enabled(pod=model_registry_operator_pod), ( + "ENABLE_MODEL_CATALOG env var missing or disabled on MR operator pod" + )
80-81: Tighten API assertion messaging and validate the default catalog shape.Message says “no custom models” while asserting len == 1; make it explicit and also validate the default catalog fields returned by the API.
- assert result - assert len(result) == 1, f"Expected no custom models to be present. Actual: {result}" + assert result, f"No sources returned from {model_catalog_rest_url[0]}sources" + assert len(result) == 1, f"Expected exactly 1 source (default); got {len(result)}: {result}" + validate_default_catalog(default_catalog=result[0])
83-87: Pass admin_client to ConfigMap for consistency; consider gating this temporary test.Instantiate ConfigMap with the fixture client to mirror other tests and avoid implicit global client resolution. Since this is temporary (RHOAIENG-33246), add a skip/xfail guard tied to release or env to prevent future flakiness.
- def test_default_config_map_not_present(self: Self, model_registry_namespace: str): + def test_default_config_map_not_present(self: Self, admin_client: DynamicClient, model_registry_namespace: str): @@ - cfg_map = ConfigMap(name="default-model-catalog", namespace=model_registry_namespace) + cfg_map = ConfigMap(name="default-model-catalog", client=admin_client, namespace=model_registry_namespace) assert not cfg_map.exists, f"{cfg_map.name} should not exist"Option: mark with skipif based on a release/env flag until 2.25 is broadly available.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
tests/model_registry/model_catalog/test_custom_model_catalog.py(1 hunks)tests/model_registry/model_catalog/test_default_model_catalog.py(1 hunks)tests/model_registry/model_catalog/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/model_registry/model_catalog/test_custom_model_catalog.py
- tests/model_registry/model_catalog/utils.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/model_catalog/test_default_model_catalog.py
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/test_default_model_catalog.py (4)
tests/model_registry/model_catalog/utils.py (4)
validate_model_catalog_enabled(38-43)execute_get_command(29-35)validate_model_catalog_resource(66-69)validate_default_catalog(72-76)tests/model_registry/model_catalog/conftest.py (2)
catalog_config_map(15-16)model_catalog_rest_url(27-36)tests/conftest.py (1)
admin_client(66-67)tests/model_registry/conftest.py (3)
model_registry_namespace(63-64)model_registry_operator_pod(273-280)model_registry_rest_headers(290-295)
🔇 Additional comments (1)
tests/model_registry/model_catalog/test_default_model_catalog.py (1)
58-63: LGTM: shared validator use keeps tests DRY.
|
/build-push-pr-image |
|
Status of building tag pr-578: success. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_registry/model_catalog/utils.py (1)
72-76: Don’t assert exactly one Pod; allow >=1 and improve diagnostics.Pods can scale. Keep strict singleton for other resources; also add clearer failure messages and a reusable label selector.
Per our retrieved learning, the Model Catalog ConfigMap is shared and only exists in py_config["model_registry_namespace"]; ensure callers pass that namespace when validating the ConfigMap.
-def validate_model_catalog_resource(kind: Any, admin_client: DynamicClient, namespace: str) -> None: - resource = list(kind.get(namespace=namespace, label_selector="component=model-catalog", dyn_client=admin_client)) - assert resource - assert len(resource) == 1, f"Unexpected number of {kind} resources found: {[res.name for res in resource]}" +def validate_model_catalog_resource(kind: Any, admin_client: DynamicClient, namespace: str) -> None: + label_selector = "component=model-catalog" + resources = list( + kind.get(namespace=namespace, label_selector=label_selector, dyn_client=admin_client) + ) + kind_name = getattr(kind, "__name__", str(kind)) + assert resources, f"No {kind_name} found in {namespace} with label '{label_selector}'" + if issubclass(kind, Pod): + assert len(resources) >= 1, f"Expected at least 1 Pod, found {[res.name for res in resources]}" + else: + assert len(resources) == 1, f"Unexpected number of {kind_name}: {[res.name for res in resources]}"
🧹 Nitpick comments (2)
tests/model_registry/model_catalog/constants.py (1)
41-44: Name clarity: prefer DEFAULT_CATALOG_TYPE over generic CATALOG_TYPE.CATALOG_TYPE can be ambiguous alongside other catalog types defined above; DEFAULT_CATALOG_TYPE better communicates scope. Not a blocker.
tests/model_registry/model_catalog/utils.py (1)
78-82: Type hint and add assertion messages for faster triage.Small tweaks make failures self-explanatory and resilient to missing keys.
-def validate_default_catalog(default_catalog) -> None: - assert default_catalog["name"] == DEFAULT_CATALOG_NAME - assert default_catalog["id"] == DEFAULT_CATALOG_ID - assert default_catalog["type"] == CATALOG_TYPE - assert default_catalog["properties"].get("yamlCatalogPath") == DEFAULT_CATALOG_FILE +def validate_default_catalog(default_catalog: dict[str, Any]) -> None: + assert default_catalog.get("name") == DEFAULT_CATALOG_NAME, f"name mismatch: {default_catalog.get('name')!r}" + assert default_catalog.get("id") == DEFAULT_CATALOG_ID, f"id mismatch: {default_catalog.get('id')!r}" + assert default_catalog.get("type") == CATALOG_TYPE, f"type mismatch: {default_catalog.get('type')!r}" + props = default_catalog.get("properties") or {} + assert props.get("yamlCatalogPath") == DEFAULT_CATALOG_FILE, f"yamlCatalogPath mismatch: {props.get('yamlCatalogPath')!r}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/model_registry/model_catalog/constants.py(1 hunks)tests/model_registry/model_catalog/utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/model_catalog/utils.py
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/utils.py (1)
tests/conftest.py (1)
admin_client(68-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
tests/model_registry/model_catalog/constants.py (1)
41-44: Centralizing default-catalog expectations via typed constants looks good.This removes literals from tests and makes expectations explicit and reusable.
tests/model_registry/model_catalog/utils.py (1)
12-17: Good move replacing literals with shared constants.This addresses prior feedback to avoid hardcoded strings and keeps tests aligned with expected defaults.
|
Status of building tag latest: success. |
…datahub-io#578) * ModelCatalog: update default tests to reflect expected behavior * review comments
…datahub-io#578) * ModelCatalog: update default tests to reflect expected behavior * review comments
4 participants
Add validations for default resources, update configmap and api test
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit