Remove RHEC tests and add multiple source tests#597
Remove RHEC tests and add multiple source tests#597dbasunag merged 2 commits intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughWalkthroughSwitches model-catalog tests from single static catalog data to multi-catalog, dynamic YAML generation. Adds YAML helper utilities, a fixture to append a model to a custom catalog, local ResourceNotFoundError handling, and test refactors that add/rename tests for listing sources, retrieving models/artifacts, and add/remove flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/lgtm', '/verified', '/wip', '/cherry-pick', '/build-push-pr-image', '/hold'} |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/model_registry/model_catalog/utils.py (2)
22-30: Differentiate 404 from other HTTP failures; broaden retry coverageConflating all non-2xx as “not found” can mask real server/auth errors and cause false-green tests. Raise a specific NotFound only on 404 and a generic API error otherwise; retry on both.
-class ResourceNotFoundError(Exception): - pass +class APIRequestError(Exception): + pass + +class ResourceNotFoundError(APIRequestError): + pass @@ def _execute_get_call(url: str, headers: dict[str, str], verify: bool | str = False) -> requests.Response: resp = requests.get(url=url, headers=headers, verify=verify, timeout=60) - if resp.status_code not in [200, 201]: - raise ResourceNotFoundError(f"Get call failed for resource: {url}, {resp.status_code}: {resp.text}") + if resp.status_code == 404: + raise ResourceNotFoundError(f"Resource not found: {url} (404)") + if resp.status_code not in (200, 201): + raise APIRequestError(f"GET {url} failed: {resp.status_code}: {resp.text}") return resp @@ -@retry(wait_timeout=60, sleep=5, exceptions_dict={ResourceNotFoundError: []}) +@retry(wait_timeout=60, sleep=5, exceptions_dict={ResourceNotFoundError: [], APIRequestError: []}) def wait_for_model_catalog_api(url: str, headers: dict[str, str], verify: bool | str = False) -> requests.Response: return _execute_get_call(url=f"{url}sources", headers=headers, verify=verify)Also applies to: 33-35
47-53: Guard against missing env list to avoid NPEcontainer.env can be None. Iterate safely.
def validate_model_catalog_enabled(pod: Pod) -> bool: - for container in pod.instance.spec.containers: - for env in container.env: + for container in pod.instance.spec.containers: + for env in (getattr(container, "env", None) or []): if env.name == "ENABLE_MODEL_CATALOG": return True return False
🧹 Nitpick comments (10)
tests/model_registry/model_catalog/utils.py (3)
38-45: Prefer Response.json() with tighter exception handlingSlightly simpler and avoids an intermediate string parse.
def execute_get_command(url: str, headers: dict[str, str], verify: bool | str = False) -> dict[Any, Any]: resp = _execute_get_call(url=url, headers=headers, verify=verify) - try: - return json.loads(resp.text) - except json.JSONDecodeError: + try: + return resp.json() + except (ValueError, json.JSONDecodeError): LOGGER.error(f"Unable to parse {resp.text}") raise
95-109: YAML builder: avoid shadowing built-in and trailing whitespaceMinor cleanup: don’t shadow id(), and drop the trailing spaces-only line.
-def get_catalog_str(ids: list[str]) -> str: - catalog_str: str = "" - for id in ids: - catalog_str += f""" +def get_catalog_str(ids: list[str]) -> str: + catalog_str: str = "" + for catalog_id in ids: + catalog_str += f""" - name: Sample Catalog - id: {id} + id: {catalog_id} type: yaml enabled: true properties: - yamlCatalogPath: {id.replace("_", "-")}.yaml + yamlCatalogPath: {catalog_id.replace("_", "-")}.yaml """ - return f"""catalogs: -{catalog_str} - """ + return f"""catalogs: +{catalog_str} +"""
123-136: Optional: make provider/license configurableHard-coding “Mistral AI” and license is fine for fixtures, but consider parameters if you need to add non-Mistral models later.
tests/model_registry/model_catalog/constants.py (1)
1-10: Centralize catalog-id → filename mapping to avoid driftYou derive filenames via id.replace('_','-') elsewhere while hardcoding here (e.g., “sample-custom-catalog1.yaml”). Export a helper (e.g., catalog_yaml_filename) to keep this consistent.
Example (in utils.py):
def catalog_yaml_filename(catalog_id: str) -> str: return f"{catalog_id.replace('_', '-')}.yaml"tests/model_registry/model_catalog/conftest.py (2)
51-52: Simplify patch populationUse dict.update to copy all sample YAML entries.
- if "sample_yaml" in request.param: - for key in request.param["sample_yaml"]: - patches["data"][key] = request.param["sample_yaml"][key] + if "sample_yaml" in request.param: + patches["data"].update(request.param["sample_yaml"])
66-66: Remove unused parameter per Ruff ARG001request isn’t used in updated_catalog_config_map_add_model.
tests/model_registry/model_catalog/test_custom_model_catalog.py (4)
37-46: Give distinct parametrize ids and avoid hardcoded filenamesUnique ids improve test reports; prefer deriving filenames from catalog ids to prevent drift.
- id="file_test_catalog", + id="single_source", @@ - id="file_test_catalog", + id="multi_source",Optionally:
from tests.model_registry.model_catalog.utils import catalog_yaml_filename ... "sample_yaml": {catalog_yaml_filename(CUSTOM_CATALOG_ID1): ...}
55-61: Fixture type annotation mismatchupdated_catalog_config_map yields a ConfigMap; drop the tuple[...] annotation (or omit typing) to avoid confusion.
- updated_catalog_config_map: tuple[ConfigMap, str, str], + updated_catalog_config_map,
143-161: Remove unused arg; fix fixture typingexpected_catalog_values isn’t used; also the fixture type hint is inaccurate.
- def test_model_custom_catalog_add_model( + def test_model_custom_catalog_add_model( self: Self, model_catalog_rest_url: list[str], model_registry_rest_headers: dict[str, str], - expected_catalog_values: dict[str, str], - updated_catalog_config_map_add_model: dict[str, str], + updated_catalog_config_map_add_model, ):
169-179: Docstring and exception semanticsUpdate docstring to “remove” to reflect behavior. With the utils change, this will now fail only on true 404, not any non-2xx.
- Add a model to a source and ensure it is added to the catalog + Ensure previously added model is no longer present (after fixture cleanup)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/model_catalog/conftest.py(3 hunks)tests/model_registry/model_catalog/constants.py(1 hunks)tests/model_registry/model_catalog/test_custom_model_catalog.py(6 hunks)tests/model_registry/model_catalog/utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/conftest.pytests/model_registry/model_catalog/test_custom_model_catalog.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/model_catalog/conftest.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/conftest.py
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/model_catalog/conftest.py
🧬 Code graph analysis (2)
tests/model_registry/model_catalog/conftest.py (3)
tests/model_registry/model_catalog/utils.py (3)
is_model_catalog_ready(55-65)wait_for_model_catalog_api(34-35)get_model_str(123-136)tests/model_registry/conftest.py (2)
model_registry_namespace(62-63)model_registry_rest_headers(290-295)tests/conftest.py (1)
admin_client(68-69)
tests/model_registry/model_catalog/test_custom_model_catalog.py (3)
tests/model_registry/model_catalog/utils.py (4)
execute_get_command(38-44)get_catalog_str(95-108)get_sample_yaml_str(111-120)ResourceNotFoundError(22-23)tests/model_registry/model_catalog/conftest.py (3)
model_catalog_rest_url(28-37)expected_catalog_values(62-63)updated_catalog_config_map_add_model(67-81)tests/model_registry/conftest.py (1)
model_registry_rest_headers(290-295)
🪛 Ruff (0.12.2)
tests/model_registry/model_catalog/conftest.py
68-68: Unused function argument: request
(ARG001)
tests/model_registry/model_catalog/test_custom_model_catalog.py
72-72: Use of assert detected
(S101)
75-75: Use of assert detected
(S101)
95-95: Use of assert detected
(S101)
116-116: Use of assert detected
(S101)
140-140: Use of assert detected
(S101)
141-141: Use of assert detected
(S101)
148-148: Unused method argument: expected_catalog_values
(ARG002)
149-149: Unused method argument: updated_catalog_config_map_add_model
(ARG002)
160-160: Use of assert detected
(S101)
167-167: Unused method argument: expected_catalog_values
(ARG002)
🔇 Additional comments (9)
tests/model_registry/model_catalog/utils.py (1)
111-121: LGTM: simple model list YAML generationThe structure aligns with get_model_str output and current test needs.
tests/model_registry/model_catalog/constants.py (1)
15-15: LGTM: added SAMPLE_MODEL_NAME3Expands coverage for add/remove flow.
tests/model_registry/model_catalog/conftest.py (1)
11-13: LGTM: imports for multi-source and YAML snippet generationImports are consistent with new helpers.
tests/model_registry/model_catalog/test_custom_model_catalog.py (6)
3-9: LGTM: expanded constants for multi-source scenariosImports align with new constants.
15-20: LGTM: utils usageSwitch to local helpers keeps tests cohesive.
65-76: LGTM: sources list assertionsCovers count and ids, good signal.
87-96: LGTM: per-source model listingSimple and effective presence check.
107-116: LGTM: get-by-name validationDirect name equality is sufficient here.
128-142: LGTM: artifact presence checkAsserts existence and non-empty uri.
dbasunag
force-pushed
the
multiple_sources
branch
from
3fa8c36 to
eaf686e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
tests/model_registry/model_catalog/conftest.py (1)
66-81: Limit patch scope; remove unused arg; assert file exists (same as prior feedback)Reiterating earlier feedback: patching the full to_dict risks clobbering metadata; request is unused; and you should assert the YAML key exists before appending.
@pytest.fixture(scope="function") -def update_configmap_data_add_model( - request: pytest.FixtureRequest, +def update_configmap_data_add_model( catalog_config_map: ConfigMap, model_registry_namespace: str, admin_client: DynamicClient, model_catalog_rest_url: list[str], model_registry_rest_headers: dict[str, str], ) -> Generator[ConfigMap, None, None]: - patches = catalog_config_map.instance.to_dict() - patches["data"][f"{CUSTOM_CATALOG_ID1.replace('_', '-')}.yaml"] += get_model_str(model=SAMPLE_MODEL_NAME3) - with ResourceEditor(patches={catalog_config_map: patches}): + file_name = f"{CUSTOM_CATALOG_ID1.replace('_', '-')}.yaml" + base_yaml = catalog_config_map.instance.data.get(file_name) + assert base_yaml is not None, f"Catalog YAML {file_name} not found in ConfigMap" + new_yaml = f"{base_yaml}{get_model_str(model=SAMPLE_MODEL_NAME3)}" + with ResourceEditor(patches={catalog_config_map: {"data": {file_name: new_yaml}}}): is_model_catalog_ready(client=admin_client, model_registry_namespace=model_registry_namespace) wait_for_model_catalog_api(url=model_catalog_rest_url[0], headers=model_registry_rest_headers) yield catalog_config_map is_model_catalog_ready(client=admin_client, model_registry_namespace=model_registry_namespace)
🧹 Nitpick comments (6)
tests/model_registry/model_catalog/utils.py (2)
97-105: Avoid shadowing built-in name 'id'Use catalog_id to avoid shadowing the built-in id().
- for id in ids: + for catalog_id in ids: - catalog_str += f""" - - name: Sample Catalog - id: {id} + catalog_str += f""" + - name: Sample Catalog + id: {catalog_id} - yamlCatalogPath: {id.replace("_", "-")}.yaml + yamlCatalogPath: {catalog_id.replace("_", "-")}.yaml
95-136: Prefer generating YAML via a dumper over string concatenationTo prevent indentation/format drift, consider using PyYAML (or ruamel.yaml) to build these structures.
import yaml def get_catalog_str(ids: list[str]) -> str: data = {"catalogs": [ { "name": "Sample Catalog", "id": cid, "type": "yaml", "enabled": True, "properties": {"yamlCatalogPath": cid.replace("_","-") + ".yaml"}, } for cid in ids ]} return yaml.safe_dump(data, sort_keys=False) def get_sample_yaml_str(models: list[str]) -> str: data = { "source": "Hugging Face", "models": [get_model_dict(m) for m in models], } return yaml.safe_dump(data, sort_keys=False) def get_model_dict(model: str) -> dict: return { "name": model, "description": "test description.", "readme": f"# test read me information {model}", "provider": "Mistral AI", "logo": "temp placeholder logo", "license": "apache-2.0", "licenseLink": "https://www.apache.org/licenses/LICENSE-2.0.txt", "libraryName": "transformers", "artifacts": [{"uri": f"https://huggingface.co/{model}/resolve/main/consolidated.safetensors"}], }tests/model_registry/model_catalog/test_custom_model_catalog.py (4)
36-47: Give each parametrize case a unique idTwo cases both use id="file_test_catalog". Use distinct ids for clarity in reports.
- id="file_test_catalog", + id="file_test_catalog_single", @@ - id="file_test_catalog", + id="file_test_catalog_multi",
143-161: Silence unused params and encode model in add flowRename unused fixture param to underscore to satisfy linters, and encode SAMPLE_MODEL_NAME3 in the URL.
- def test_model_custom_catalog_add_model( + def test_model_custom_catalog_add_model( self: Self, model_catalog_rest_url: list[str], model_registry_rest_headers: dict[str, str], - expected_catalog_values: dict[str, str], - update_configmap_data_add_model: dict[str, str], + _expected_catalog_values: list[dict[str, str]], + _update_configmap_data_add_model: dict[str, str], ): @@ - url = f"{model_catalog_rest_url[0]}sources/{CUSTOM_CATALOG_ID1}/models/{SAMPLE_MODEL_NAME3}" + from urllib.parse import quote + url = f"{model_catalog_rest_url[0]}sources/{CUSTOM_CATALOG_ID1}/models/{quote(SAMPLE_MODEL_NAME3, safe='')}"
162-179: Fix docstring; silence unused param; encode model; keep negative assertionAdjust docstring, rename unused param to underscore, and encode the model name.
- def test_model_custom_catalog_remove_model( + def test_model_custom_catalog_remove_model( self: Self, model_catalog_rest_url: list[str], model_registry_rest_headers: dict[str, str], - expected_catalog_values: dict[str, str], + _expected_catalog_values: list[dict[str, str]], ): - """ - Add a model to a source and ensure it is added to the catalog - """ - url = f"{model_catalog_rest_url[0]}sources/{CUSTOM_CATALOG_ID1}/models/{SAMPLE_MODEL_NAME3}" + """ + Ensure the previously added model is no longer present after fixture teardown. + """ + from urllib.parse import quote + url = f"{model_catalog_rest_url[0]}sources/{CUSTOM_CATALOG_ID1}/models/{quote(SAMPLE_MODEL_NAME3, safe='')}"
60-61: Type hints for expected_catalog_values should be a sequence of mappingsThese tests treat expected_catalog_values as an iterable of dict entries. Update hints accordingly.
- expected_catalog_values: dict[str, str], + expected_catalog_values: list[dict[str, str]],Also applies to: 82-83, 102-103, 123-124, 148-149, 167-168
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/model_catalog/conftest.py(3 hunks)tests/model_registry/model_catalog/constants.py(1 hunks)tests/model_registry/model_catalog/test_custom_model_catalog.py(6 hunks)tests/model_registry/model_catalog/utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/conftest.pytests/model_registry/model_catalog/test_custom_model_catalog.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/model_catalog/conftest.py
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/model_catalog/conftest.py
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/model_catalog/conftest.py
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/test_custom_model_catalog.py (2)
tests/model_registry/model_catalog/utils.py (4)
execute_get_command(38-44)get_catalog_str(95-108)get_sample_yaml_str(111-120)ResourceNotFoundError(22-23)tests/model_registry/conftest.py (1)
model_registry_rest_headers(290-295)
🪛 Ruff (0.12.2)
tests/model_registry/model_catalog/conftest.py
68-68: Unused function argument: request
(ARG001)
tests/model_registry/model_catalog/test_custom_model_catalog.py
148-148: Unused method argument: expected_catalog_values
(ARG002)
149-149: Unused method argument: update_configmap_data_add_model
(ARG002)
167-167: Unused method argument: expected_catalog_values
(ARG002)
🔇 Additional comments (7)
tests/model_registry/model_catalog/utils.py (2)
22-24: Localize and standardize the not-found exceptionDefining a local ResourceNotFoundError here is fine and aligns callers. No issues.
33-36: Retry wrapper is appropriate for transient 404sWrapping the readiness check with retry on ResourceNotFoundError looks correct. No changes needed.
tests/model_registry/model_catalog/constants.py (2)
1-10: Multi-catalog constants look goodNaming is clear and supports the new tests.
15-15: Additional sample model constant is fineSAMPLE_MODEL_NAME3 adds coverage for add/remove flows.
tests/model_registry/model_catalog/conftest.py (2)
11-13: Targeted imports are appropriateImporting SAMPLE_MODEL_NAME3, CUSTOM_CATALOG_ID1, and get_model_str here is correct for the new flows.
51-52: Merging sample_yaml keys into the ConfigMap patch is fineThis keeps the patch narrow to data entries. Looks good.
tests/model_registry/model_catalog/test_custom_model_catalog.py (1)
55-76: Sources listing assertions look correctGood coverage validating count and IDs.
| assert result[0]["id"] == expected_catalog_values["id"] | ||
|
|
||
| def test_model_custom_catalog_models( | ||
| LOGGER.info(f"Results: for uri {url}: {results}") |
| )["items"] | ||
| assert result, f"Expected custom models to be present. Actual: {result}" | ||
| for expected_entry in expected_catalog_values: | ||
| LOGGER.info(f"Expected entry: {expected_entry}") |
| url=url, | ||
| headers=model_registry_rest_headers, | ||
| )["items"] | ||
| LOGGER.info(f"URL: {url} Result: {result}") |
|
Status of building tag latest: success. |
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Tests
Chores