test: reorganize test structure in preparation of top folder renaming#976
test: reorganize test structure in preparation of top folder renaming#976fege merged 8 commits intoopendatahub-io:mainfrom
Conversation
Restructure model_registry tests into a clearer hierarchy: - Split into model_registry/ and model_catalog/ top-level domains - Group model_catalog tests by function: catalog_config, metadata, upgrade
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/build-push-pr-image', '/verified', '/lgtm', '/hold', '/wip'} |
📝 WalkthroughWalkthroughConsolidates model registry test fixtures and reorganizes model catalog utilities into domain-specific submodules with corresponding import path updates. Removes legacy validation functions, refactors tests to use new module structure, and adds new cluster health monitoring test. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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
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: 2
🧹 Nitpick comments (15)
tests/model_registry/health/test_mr_operator_health.py (1)
9-19: Add defensive error handling for unexpected Prometheus query results.The code assumes
entry["metric"]["pod"]exists without error handling. If the Prometheus query returns an unexpected structure, this will raise aKeyError.🔎 Proposed fix with defensive error handling
@pytest.mark.order("last") def test_mr_operator_not_oomkilled(prometheus_for_monitoring: Prometheus): result = prometheus_for_monitoring.query_sampler( query='kube_pod_container_status_last_terminated_reason{reason="OOMKilled"}' ) if result: for entry in result: LOGGER.info(entry) - pod_name = entry["metric"]["pod"] + pod_name = entry.get("metric", {}).get("pod") + if not pod_name: + LOGGER.warning(f"Unexpected entry structure, skipping: {entry}") + continue if pod_name.startswith("model-registry-operator-controller-manager"): pytest.fail(f"Pod {pod_name} was oomkilled: {entry}")tests/model_registry/model_catalog/catalog_config/utils.py (3)
33-43: Collect all validation errors before asserting.Line 37 immediately raises an
AssertionErrorfor unexpected catalogs, preventing validation of subsequent catalogs. Consider collecting all errors first.🔎 Proposed fix to collect all errors
def validate_default_catalog(catalogs: list[dict[Any, Any]]) -> None: errors = [] for catalog in catalogs: expected_catalog = DEFAULT_CATALOGS.get(catalog["id"]) - assert expected_catalog, f"Unexpected catalog: {catalog}" + if not expected_catalog: + errors.append(f"Unexpected catalog: {catalog}") + continue for key, expected_value in expected_catalog.items(): actual_value = catalog.get(key) if actual_value != expected_value: errors.append(f"For catalog '{catalog['id']}': expected {key}={expected_value}, but got {actual_value}") assert not errors, "\n".join(errors)
54-98: Add error handling for schema references.Lines 78 and 85 directly access nested dictionary keys without error handling. If the OpenAPI schema structure is unexpected or a referenced schema doesn't exist, this will raise a
KeyError.🔎 Proposed fix with error handling
def _extract_properties_and_required(schema: dict[Any, Any]) -> tuple[set[str], set[str]]: """Recursively extract properties and required fields from a schema.""" props = set(schema.get("properties", {}).keys()) required = set(schema.get("required", [])) # Properties from allOf (inheritance/composition) if "allOf" in schema: for item in schema["allOf"]: sub_schema = item if "$ref" in item: # Follow reference and recursively extract ref_schema_name = item["$ref"].split("/")[-1] - sub_schema = openapi_schema["components"]["schemas"][ref_schema_name] + try: + sub_schema = openapi_schema["components"]["schemas"][ref_schema_name] + except KeyError: + LOGGER.warning(f"Referenced schema not found: {ref_schema_name}") + continue sub_props, sub_required = _extract_properties_and_required(schema=sub_schema) props.update(sub_props) required.update(sub_required) return props, required - target_schema = openapi_schema["components"]["schemas"][schema_name] + try: + target_schema = openapi_schema["components"]["schemas"][schema_name] + except KeyError: + raise ValueError(f"Schema '{schema_name}' not found in OpenAPI spec") all_properties, required_fields = _extract_properties_and_required(schema=target_schema)
101-114: Add error handling for ConfigMap data parsing.Line 111 directly accesses ConfigMap data keys without error handling. If the ConfigMap structure is unexpected or the YAML is malformed, this will raise exceptions.
🔎 Proposed fix with error handling
def validate_model_catalog_configmap_data(configmap: ConfigMap, num_catalogs: int) -> None: """ Validate the model catalog configmap data. Args: configmap: The ConfigMap object to validate num_catalogs: Expected number of catalogs in the configmap """ # Check that model catalog configmaps is created when model registry is # enabled on data science cluster. - catalogs = yaml.safe_load(configmap.instance.data["sources.yaml"])["catalogs"] + try: + sources_yaml = configmap.instance.data.get("sources.yaml") + if not sources_yaml: + raise ValueError("ConfigMap does not contain 'sources.yaml' key") + parsed_data = yaml.safe_load(sources_yaml) + catalogs = parsed_data.get("catalogs", []) + except yaml.YAMLError as e: + raise ValueError(f"Failed to parse YAML from ConfigMap: {e}") + except (KeyError, AttributeError) as e: + raise ValueError(f"Unexpected ConfigMap structure: {e}") assert len(catalogs) == num_catalogs, f"{configmap.name} should have {num_catalogs} catalog(s)" if num_catalogs: validate_default_catalog(catalogs=catalogs)tests/model_registry/model_registry/conftest.py (5)
1-9: Consider grouping and ordering imports per PEP8.Standard library imports (
time,os,subprocess,contextlib) are mixed with third-party imports. While not blocking, consistent ordering improves readability.Suggested import ordering
+import os +import subprocess import time from contextlib import ExitStack -import os -import subprocess +from typing import Generator, Any, List, Dict import pytest import shlex from pyhelper_utils.shell import run_command -from typing import Generator, Any, List, Dict
57-95: Unused fixture parametermodel_registry_metadata_db_resources.The parameter is declared but never referenced in the function body. It's likely used to establish a dependency ordering (ensuring DB resources are created first), but this should be documented or use
pytest.mark.usefixturesinstead.Suggested fix
@pytest.fixture(scope="class") def model_registry_instance( request: pytest.FixtureRequest, pytestconfig: Config, admin_client: DynamicClient, teardown_resources: bool, - model_registry_metadata_db_resources: dict[Any, Any], + model_registry_metadata_db_resources: dict[Any, Any], # Required dependency for fixture ordering model_registry_namespace: str, ) -> Generator[list[Any], Any, Any]:Alternatively, if it's purely for ordering, consider using
@pytest.mark.usefixtures("model_registry_metadata_db_resources")as a decorator.
90-91: Remove invalidnoqadirective.The
FCN001code is not a recognized Ruff/flake8 rule. If thetime.sleep(60.0)is intentional as noted by the TODO, a comment explaining the workaround is sufficient.# TODO remove when RHOAIENG-41728 is addressed - time.sleep(60.0) # noqa: FCN001 + time.sleep(60.0) # Workaround for RHOAIENG-41728
159-161: Consider using a custom exception class for clearer error messages.Static analysis flags that long messages outside exception classes can be harder to maintain. For test infrastructure code, a simple custom exception would improve clarity.
271-304: Unused unpacked variables and exception handling improvements.The
resanderrvariables fromrun_commandare never used. Additionally, the successful return could be moved to anelseblock for clearer flow.Suggested improvements
try: cmd = f"oc create token {sa_name} -n {namespace} --duration={DEFAULT_TOKEN_DURATION}" LOGGER.debug(f"Executing command: {cmd}") - res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30) + _, out, _ = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30) token = out.strip() if not token: raise ValueError("Retrieved token is empty after successful command execution.") - - LOGGER.info(f"Successfully retrieved token for SA '{sa_name}'") - return token - except Exception as e: # Catch all exceptions from the try block error_type = type(e).__name__ log_message = ( f"Failed during token retrieval for SA '{sa_name}' in namespace '{namespace}'. " - f"Error Type: {error_type}, Message: {str(e)}" + f"Error Type: {error_type}, Message: {e!s}" ) # ... rest of exception handling LOGGER.error(log_message, exc_info=True) raise + else: + LOGGER.info(f"Successfully retrieved token for SA '{sa_name}'") + return tokentests/model_registry/model_catalog/metadata/utils.py (5)
32-37: SSL verification disabled - ensure this is intentional for test environments only.The
verify=Falsedisables SSL certificate verification. While common in test environments with self-signed certificates, this should be documented to prevent accidental use in production-like scenarios.headers = {"Authorization": f"Bearer {token}"} LOGGER.info(f"Executing model catalog POST: {url}") - response = requests.post(url=url, headers=headers, files=files, verify=False, timeout=60) + # verify=False: Test environment uses self-signed certificates + response = requests.post(url=url, headers=headers, files=files, verify=False, timeout=60)
337-344: Uselogging.exceptionfor better stack trace logging.When catching and re-raising exceptions,
LOGGER.exception()automatically includes the stack trace without needingexc_info=True.Suggested fix
try: metadata_json = model_catalog_pod.execute(command=["cat", metadata_path], container=CATALOG_CONTAINER) metadata = json.loads(metadata_json) LOGGER.info(f"Successfully loaded metadata.json for model '{model_name}'") return metadata except Exception as e: - LOGGER.error(f"Failed to read metadata.json for model '{model_name}': {e}") + LOGGER.exception(f"Failed to read metadata.json for model '{model_name}'") raise
493-496: Remove unusednoqadirective.The
# noqa: E501directive is flagged as unused. If the line length is acceptable, remove the directive.LOGGER.info( - f"After applying API filtering, expecting {len(expected_properties)} properties: {list(expected_properties.keys())}" # noqa: E501 + f"After applying API filtering, expecting {len(expected_properties)} properties: " + f"{list(expected_properties.keys())}" )
526-529: Uselogging.exceptionfor exception context.Similar to the earlier suggestion, use
LOGGER.exception()when logging errors in exception handlers.except (ValueError, TypeError) as e: - error_msg = f"Property '{prop_name}': Failed to parse numeric values - {e}" - LOGGER.error(error_msg) + error_msg = f"Property '{prop_name}': Failed to parse numeric values" + LOGGER.exception(error_msg) comparison_errors.append(error_msg)
644-655: Consider using a more Pythonic comparison approach.The nested loop for comparing labels could be simplified using
any()with a generator expression for better readability.Suggested improvement
for expected_label in expected_labels: - found = False - for api_label in api_labels: - if ( - expected_label.get("name") == api_label.get("name") - and expected_label.get("displayName") == api_label.get("displayName") - and expected_label.get("description") == api_label.get("description") - ): - found = True - break - - assert found, f"Expected label not found in API response: {expected_label}" + found = any( + expected_label.get("name") == api_label.get("name") + and expected_label.get("displayName") == api_label.get("displayName") + and expected_label.get("description") == api_label.get("description") + for api_label in api_labels + ) + assert found, f"Expected label not found in API response: {expected_label}"tests/model_registry/model_catalog/conftest.py (1)
384-386: Addensure_exists=Truefor consistency.For consistency with other ConfigMap instantiations in this file (lines 53, 73, 100), consider adding the
ensure_exists=Trueparameter to fail fast if the ConfigMap doesn't exist in the cluster.🔎 Proposed change
@pytest.fixture(scope="class") def catalog_config_map(admin_client: DynamicClient, model_registry_namespace: str) -> ConfigMap: - return ConfigMap(name=DEFAULT_CUSTOM_MODEL_CATALOG, client=admin_client, namespace=model_registry_namespace) + return ConfigMap(name=DEFAULT_CUSTOM_MODEL_CATALOG, client=admin_client, namespace=model_registry_namespace, ensure_exists=True)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
tests/model_registry/conftest.pytests/model_registry/health/__init__.pytests/model_registry/health/conftest.pytests/model_registry/health/test_mr_health_check.pytests/model_registry/health/test_mr_operator_health.pytests/model_registry/model_catalog/catalog_config/__init__.pytests/model_registry/model_catalog/catalog_config/test_custom_model_catalog.pytests/model_registry/model_catalog/catalog_config/test_default_model_catalog.pytests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.pytests/model_registry/model_catalog/catalog_config/utils.pytests/model_registry/model_catalog/conftest.pytests/model_registry/model_catalog/metadata/__init__.pytests/model_registry/model_catalog/metadata/test_catalog_preview.pytests/model_registry/model_catalog/metadata/test_custom_properties.pytests/model_registry/model_catalog/metadata/test_filter_options_endpoint.pytests/model_registry/model_catalog/metadata/test_labels_endpoint.pytests/model_registry/model_catalog/metadata/utils.pytests/model_registry/model_catalog/upgrade/__init__.pytests/model_registry/model_catalog/upgrade/test_model_catalog_upgrade.pytests/model_registry/model_registry/__init__.pytests/model_registry/model_registry/async_job/__init__.pytests/model_registry/model_registry/async_job/conftest.pytests/model_registry/model_registry/async_job/constants.pytests/model_registry/model_registry/async_job/test_async_upload_e2e.pytests/model_registry/model_registry/async_job/utils.pytests/model_registry/model_registry/conftest.pytests/model_registry/model_registry/negative_tests/__init__.pytests/model_registry/model_registry/negative_tests/conftest.pytests/model_registry/model_registry/negative_tests/test_db_migration.pytests/model_registry/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/model_registry/negative_tests/utils.pytests/model_registry/model_registry/python_client/__init__.pytests/model_registry/model_registry/python_client/test_model_registry_creation.pytests/model_registry/model_registry/rbac/__init__.pytests/model_registry/model_registry/rbac/conftest.pytests/model_registry/model_registry/rbac/group_utils.pytests/model_registry/model_registry/rbac/multiple_instance_utils.pytests/model_registry/model_registry/rbac/test_mr_rbac.pytests/model_registry/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/model_registry/rbac/utils.pytests/model_registry/model_registry/rest_api/__init__.pytests/model_registry/model_registry/rest_api/conftest.pytests/model_registry/model_registry/rest_api/constants.pytests/model_registry/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/model_registry/rest_api/test_multiple_mr.pytests/model_registry/model_registry/rest_api/utils.pytests/model_registry/model_registry/upgrade/__init__.pytests/model_registry/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/scc/constants.pytests/model_registry/scc/test_model_catalog_scc.py
💤 Files with no reviewable changes (2)
- tests/model_registry/scc/constants.py
- tests/model_registry/health/conftest.py
🧰 Additional context used
🧬 Code graph analysis (12)
tests/model_registry/model_registry/async_job/conftest.py (2)
tests/model_registry/model_registry/async_job/utils.py (1)
upload_test_model_to_minio_from_image(48-136)tests/model_registry/utils.py (2)
get_mr_service_by_label(51-75)get_endpoint_from_mr_service(78-82)
tests/model_registry/model_registry/rbac/test_mr_rbac.py (1)
tests/model_registry/model_registry/rbac/utils.py (2)
grant_mr_access(87-124)revoke_mr_access(127-144)
tests/model_registry/model_registry/rbac/test_mr_rbac_sa.py (1)
tests/model_registry/model_registry/rbac/utils.py (1)
build_mr_client_args(14-35)
tests/model_registry/model_registry/rbac/conftest.py (4)
tests/model_registry/model_registry/rbac/utils.py (1)
create_role_binding(67-84)utilities/user_utils.py (1)
UserTestSession(22-45)utilities/infra.py (1)
login_with_user_password(449-473)tests/model_registry/model_registry/rbac/group_utils.py (1)
create_group(11-37)
tests/model_registry/health/test_mr_operator_health.py (1)
tests/model_registry/health/conftest.py (1)
prometheus_for_monitoring(15-19)
tests/model_registry/model_registry/negative_tests/conftest.py (1)
tests/model_registry/model_registry/negative_tests/utils.py (2)
execute_mysql_command(21-33)create_mysql_credentials_file(6-18)
tests/model_registry/model_registry/rest_api/test_model_registry_secure_db.py (1)
tests/model_registry/model_registry/rest_api/utils.py (2)
register_model_rest_api(57-90)validate_resource_attributes(93-114)
tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.py (1)
tests/model_registry/model_catalog/catalog_config/utils.py (1)
validate_model_catalog_configmap_data(101-114)
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py (1)
tests/model_registry/model_registry/rest_api/utils.py (1)
validate_resource_attributes(93-114)
tests/model_registry/model_registry/rest_api/conftest.py (1)
tests/model_registry/model_registry/rest_api/utils.py (1)
generate_ca_and_server_cert(117-152)
tests/model_registry/model_registry/async_job/test_async_upload_e2e.py (1)
tests/model_registry/model_registry/async_job/utils.py (2)
get_latest_job_pod(15-33)pull_manifest_from_oci_registry(36-45)
tests/model_registry/model_catalog/metadata/utils.py (1)
tests/model_registry/utils.py (2)
execute_get_command(730-738)get_rest_headers(669-674)
🪛 Ruff (0.14.10)
tests/model_registry/model_registry/conftest.py
54-54: Possible hardcoded password assigned to: "DEFAULT_TOKEN_DURATION"
(S105)
63-63: Unused function argument: model_registry_metadata_db_resources
(ARG001)
91-91: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
160-160: Avoid specifying long messages outside the exception class
(TRY003)
190-190: Avoid specifying long messages outside the exception class
(TRY003)
274-274: Unpacked variable res is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
274-274: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
277-277: Abstract raise to an inner function
(TRY301)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
280-280: Consider moving this statement to an else block
(TRY300)
286-286: Use explicit conversion flag
Replace with conversion flag
(RUF010)
tests/model_registry/model_catalog/metadata/utils.py
35-35: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
341-341: Consider moving this statement to an else block
(TRY300)
343-343: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
495-495: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
528-528: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ 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: tox-tests
🔇 Additional comments (32)
tests/model_registry/model_registry/negative_tests/conftest.py (1)
28-31: Import path correctly reflects the reorganized test structure.The import has been properly updated from the flat module structure to the new hierarchical path
tests.model_registry.model_registry.negative_tests.utils, and the multi-line formatting with parentheses follows PEP 8 conventions. The imported functions (execute_mysql_commandandcreate_mysql_credentials_file) are used correctly in theset_mr_db_dirtyfixture.tests/model_registry/model_registry/async_job/conftest.py (1)
9-38: LGTM! Fixture imports updated consistently.All import statements have been correctly updated to reflect the new package structure (
tests.model_registry.model_registry.async_job.*). The fixture definitions remain unchanged, preserving the existing test infrastructure functionality.tests/model_registry/model_registry/async_job/test_async_upload_e2e.py (1)
8-19: Import refactoring is complete and consistent.All import paths have been successfully updated to
tests.model_registry.model_registry.async_job.*with no remaining references to the oldtests.model_registry.async_job.*paths in the codebase. The required module files (constants.py,utils.py) exist with proper package structure (__init__.pypresent).Note: There are two separate imports from
constants.py(lines 8-10 and line 19). Consider consolidating them into a single import statement for better code style:from tests.model_registry.model_registry.async_job.constants import ( ASYNC_UPLOAD_JOB_NAME, MODEL_SYNC_CONFIG, REPO_NAME, TAG, )tests/model_registry/model_registry/rbac/conftest.py (2)
21-21: LGTM! Import path correctly updated.The import path for
create_role_bindinghas been correctly updated to reflect the new package structure.
24-24: LGTM! Import path correctly updated.The import path for
create_grouphas been correctly updated to reflect the new package structure.tests/model_registry/model_registry/rbac/test_mr_rbac.py (3)
25-25: LGTM! Import path correctly updated.The import path for
MR_MULTIPROJECT_TEST_SCENARIO_PARAMShas been correctly updated to reflect the new package structure.
26-30: LGTM! Import paths correctly updated.The import paths for RBAC utility functions have been correctly updated to reflect the new package structure.
38-38: LGTM! Import path correctly updated.The import path for
grant_mr_accessandrevoke_mr_accesshas been correctly updated to reflect the new package structure.tests/model_registry/model_registry/rbac/test_mr_rbac_sa.py (1)
6-6: Import path update is correct and complete.The new import path
tests.model_registry.model_registry.rbac.utilsis valid and exists. No stale references to the old path remain in the codebase, and all related imports have been consistently updated across the test suite.tests/model_registry/scc/test_model_catalog_scc.py (1)
15-15: LGTM!The local constant definition is clear and aligns with the PR's objective to reorganize test structure.
tests/model_registry/model_catalog/metadata/test_labels_endpoint.py (1)
10-14: LGTM!The import path update correctly reflects the reorganized module structure.
tests/model_registry/health/test_mr_operator_health.py (1)
1-6: LGTM!The imports and logger setup are appropriate for the health monitoring test.
tests/model_registry/conftest.py (3)
3-3: LGTM!The import additions are appropriate and used within the file.
Also applies to: 29-29
45-114: LGTM!The fixture properly manages DSC component state transitions and namespace lifecycle with appropriate cleanup logic.
117-263: LGTM!The authentication and authorization fixtures are well-structured with proper cleanup logic and conditional handling for different authentication methods.
tests/model_registry/model_catalog/catalog_config/utils.py (2)
14-19: LGTM!The function correctly checks for the
ENABLE_MODEL_CATALOGenvironment variable across all containers.
46-51: LGTM!The function correctly validates that the default catalog sources match expectations.
tests/model_registry/model_catalog/metadata/test_catalog_preview.py (1)
6-11: Import path update aligns with test restructuring.The import path change from
tests.model_registry.model_catalog.utilstotests.model_registry.model_catalog.metadata.utilsis consistent with the PR's goal of reorganizing test utilities into a clearer hierarchy.tests/model_registry/model_catalog/metadata/test_custom_properties.py (1)
8-14: Import path update consistent with module reorganization.The imports are correctly updated to reference the new
metadata.utilsmodule location. The inclusion ofget_metadata_from_catalog_podin the import aligns with its definition in the new utils file.tests/model_registry/model_catalog/metadata/test_filter_options_endpoint.py (1)
5-12: Clean separation of import sources reflects logical module organization.The split between
metadata.utils(for validation functions) andmodel_catalog.utils(for database operations) demonstrates good separation of concerns. This organization makes the codebase easier to navigate and maintain.tests/model_registry/model_registry/rest_api/test_model_registry_secure_db.py (1)
4-5: Import path update consistent with REST API module reorganization.The import path change aligns with the restructuring pattern seen in other REST API test files. The test logic and functionality remain unchanged.
tests/model_registry/model_registry/rest_api/test_multiple_mr.py (1)
15-19: Import path correctly reflects the new module structure.The import path change to
tests.model_registry.model_registry.rest_api.utilsis consistent across all REST API test files (conftest.py, test_multiple_mr.py, test_model_registry_rest_api.py, and test_model_registry_secure_db.py). The file exists at the specified location and the nestedmodel_registry.model_registrypattern mirrors the actual directory structure.tests/model_registry/model_registry/rest_api/utils.py (1)
15-15: LGTM! Import path updated to reflect package reorganization.The import path change from
tests.model_registry.rest_api.constantstotests.model_registry.model_registry.rest_api.constantsis consistent with the PR's objective to reorganize the test structure.tests/model_registry/model_catalog/catalog_config/test_model_catalog_negative.py (1)
9-9: LGTM! Import path correctly updated for catalog_config utilities.The import path change from
tests.model_registry.model_catalog.utilstotests.model_registry.model_catalog.catalog_config.utilsproperly reflects the reorganization of catalog utilities into a more specific subdirectory structure.tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py (1)
11-26: LGTM! Import paths updated consistently for REST API modules.Both import path changes correctly reflect the deeper package structure:
- Constants:
tests.model_registry.rest_api.constants→tests.model_registry.model_registry.rest_api.constants- Utils:
tests.model_registry.rest_api.utils→tests.model_registry.model_registry.rest_api.utilsThe reorganization maintains consistency across the test suite.
tests/model_registry/model_catalog/catalog_config/test_default_model_catalog.py (1)
20-26: LGTM! Import path updated for catalog_config utilities.The import path change from
tests.model_registry.model_catalog.utilstotests.model_registry.model_catalog.catalog_config.utilscorrectly reflects the reorganization of catalog utilities into the catalog_config subdirectory. All imported functions are properly used throughout the test file.tests/model_registry/model_registry/rest_api/conftest.py (3)
8-13: LGTM! Import paths updated consistently for REST API fixtures.The import paths for both constants and utils modules have been correctly updated to reflect the deeper package structure (
tests.model_registry.model_registry.rest_api). All imported items are properly used in the fixture definitions below.
40-40: LGTM! Import path updated for certificate generation utility.The import path for
generate_ca_and_server_certhas been correctly updated to match the new package structure. The function is properly used in themysql_ssl_artifact_pathsfixture at line 282.
8-40: All imports resolve correctly. Verification confirms that all updated import paths exist and all imported symbols are present in their source modules. No issues detected.tests/model_registry/model_catalog/conftest.py (3)
12-12: LGTM!The Route import is correctly placed and required for the new
model_catalog_routesfixture.
389-393: LGTM!The fixture correctly retrieves and filters model catalog routes using the appropriate label selector.
396-406: LGTM!The fixture correctly constructs REST URLs from model catalog routes with appropriate assertions for failure cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/model_registry/model_catalog/catalog_config/utils.py (1)
22-30: LGTM - previous issue resolved.The extra closing parenthesis in the log message has been fixed.
tests/model_registry/model_catalog/metadata/utils.py (1)
1-14: LGTM - previous import issue resolved.The imports of
execute_get_commandandget_rest_headersare now correctly placed at module level (line 12), addressing the previous review feedback.
🧹 Nitpick comments (6)
tests/model_registry/model_catalog/catalog_config/utils.py (2)
14-19: Consider checking the env var value, not just presence.The function returns
TrueifENABLE_MODEL_CATALOGexists, regardless of its value. If the env var could be set to"false"or"0", this logic would incorrectly report the catalog as enabled.🔎 Proposed fix if value checking is needed
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 + return env.value.lower() in ("true", "1", "yes") return False
46-51: Function name inconsistent with behavior.The function is named
get_validate_default_model_catalog_sourcebut returnsNone. Aget_*prefix typically implies a return value. Consider renaming tovalidate_default_model_catalog_sourcefor consistency with the other validation functions in this module.tests/model_registry/model_catalog/metadata/utils.py (4)
33-38: SSL verification disabled - acceptable for test utilities.The
verify=Falsedisables SSL certificate verification. For test code connecting to internal OpenShift routes with self-signed certificates, this is acceptable. Consider adding a brief comment explaining why this is intentional to prevent future security audit flags.LOGGER.info(f"Executing model catalog POST: {url}") + # verify=False: Internal OpenShift routes use self-signed certificates response = requests.post(url=url, headers=headers, files=files, verify=False, timeout=60)
338-345: UseLOGGER.exceptionfor better error diagnostics.When catching exceptions,
LOGGER.exceptionautomatically includes the traceback, which is more useful for debugging thanLOGGER.error.🔎 Proposed fix
try: metadata_json = model_catalog_pod.execute(command=["cat", metadata_path], container=CATALOG_CONTAINER) metadata = json.loads(metadata_json) LOGGER.info(f"Successfully loaded metadata.json for model '{model_name}'") return metadata except Exception as e: - LOGGER.error(f"Failed to read metadata.json for model '{model_name}': {e}") + LOGGER.exception(f"Failed to read metadata.json for model '{model_name}'") raise
494-497: Remove unusednoqadirective.Static analysis indicates the
# noqa: E501comment on line 496 is unused. The line may now be within the length limit.LOGGER.info( - f"After applying API filtering, expecting {len(expected_properties)} properties: {list(expected_properties.keys())}" # noqa: E501 + f"After applying API filtering, expecting {len(expected_properties)} properties: {list(expected_properties.keys())}" )
527-530: UseLOGGER.exceptionfor better error diagnostics.Similar to the earlier suggestion,
LOGGER.exceptionautomatically includes the traceback.except (ValueError, TypeError) as e: - error_msg = f"Property '{prop_name}': Failed to parse numeric values - {e}" - LOGGER.error(error_msg) + error_msg = f"Property '{prop_name}': Failed to parse numeric values" + LOGGER.exception(error_msg) comparison_errors.append(error_msg)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_registry/model_catalog/catalog_config/utils.pytests/model_registry/model_catalog/metadata/utils.pytests/model_registry/model_catalog/utils.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/model_registry/model_catalog/catalog_config/utils.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/model_catalog/metadata/utils.py (3)
tests/model_registry/utils.py (2)
execute_get_command(730-738)get_rest_headers(669-674)tests/conftest.py (1)
admin_client(79-80)tests/model_registry/model_catalog/conftest.py (1)
model_catalog_rest_url(397-406)
🪛 Ruff (0.14.10)
tests/model_registry/model_catalog/metadata/utils.py
36-36: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
342-342: Consider moving this statement to an else block
(TRY300)
344-344: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
496-496: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
529-529: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ 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: tox-tests
🔇 Additional comments (12)
tests/model_registry/model_catalog/catalog_config/utils.py (3)
1-11: LGTM!Imports are well-organized and appropriate for the module's functionality.
54-98: LGTM - well-structured schema extraction.The recursive handling of
allOfand$refis correctly implemented. One minor consideration: line 85 will raiseKeyErrorifschema_namedoesn't exist in the OpenAPI schema, which is acceptable behavior for test utilities (fail fast).
101-114: LGTM!Clear validation logic with appropriate assertions for test utilities.
tests/model_registry/model_catalog/metadata/utils.py (4)
41-74: LGTM!The manual YAML string building provides precise control over the output format needed for the preview API.
77-219: LGTM - well-structured validation utilities.Good use of error collection pattern (collecting all errors before asserting) and proper factoring with the shared
_should_include_modelhelper.
348-469: LGTM - comprehensive filter options validation.Thorough validation covering structure, types, value constraints, and logical relationships. The flexible return type
(is_valid, errors)allows callers to choose how to handle failures.
585-655: LGTM!The labels utility functions are well-implemented. The
verify_labels_matchfunction has O(n*m) complexity, but this is acceptable for test utilities dealing with small label sets.tests/model_registry/model_catalog/utils.py (5)
1-12: LGTM!Import changes correctly reflect the refactoring, with
HF_MODELSnow imported from the constants module.
15-30: LGTM!Good use of the
@retrydecorator for resilience. Note that only the first route is used if multiple exist, which appears intentional.
33-156: LGTM - well-structured PostgreSQL utilities.The parsing logic correctly handles both single-column and array aggregation formats from psql output. The hardcoded database credentials are acceptable for test utilities.
158-211: LGTM - flexible API query helper.The function handles optional filtering and sorting parameters well. Note:
model_catalog_rest_urlis typed aslist[str]but only the first element is used (line 184). This matches the fixture return type but could be simplified to accept a single URL string if only one is ever needed.
259-274: The YAML generated by these functions is valid and parses correctly. While the indentation creates extra blank lines between keys and list items, this does not produce malformed output. The structure is parsed as intended by YAML parsers.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/model_registry/model_catalog/conftest.py (1)
383-385: Consider addingensure_exists=Truefor consistency.The fixture returns a ConfigMap without verifying its existence. Other similar fixtures in this file (e.g.,
enabled_model_catalog_config_mapat line 73) useensure_exists=True. Consider adding this parameter to fail early if the ConfigMap doesn't exist.🔎 Proposed fix
@pytest.fixture(scope="class") def catalog_config_map(admin_client: DynamicClient, model_registry_namespace: str) -> ConfigMap: - return ConfigMap(name=DEFAULT_CUSTOM_MODEL_CATALOG, client=admin_client, namespace=model_registry_namespace) + return ConfigMap(name=DEFAULT_CUSTOM_MODEL_CATALOG, client=admin_client, namespace=model_registry_namespace, ensure_exists=True)tests/model_registry/model_registry/conftest.py (1)
156-197: Comprehensive error handling; consider simplifying.The fixture has extensive error handling, which is good, but may be more than necessary for test code. One minor improvement: line 167 has unused variables
resanderrthat should be prefixed with underscores.🔎 Fix unused variables
- res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30) + _, out, _ = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_registry/conftest.pytests/model_registry/model_catalog/conftest.pytests/model_registry/model_registry/conftest.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/model_registry/conftest.py (2)
utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)tests/model_registry/utils.py (2)
get_model_registry_objects(542-575)get_model_registry_metadata_resources(578-624)
tests/model_registry/model_catalog/conftest.py (2)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (1)
model_registry_namespace(54-55)
🪛 Ruff (0.14.10)
tests/model_registry/conftest.py
286-286: Unused function argument: model_registry_metadata_db_resources
(ARG001)
314-314: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
tests/model_registry/model_registry/conftest.py
39-39: Possible hardcoded password assigned to: "DEFAULT_TOKEN_DURATION"
(S105)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
167-167: Unpacked variable res is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
167-167: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
170-170: Abstract raise to an inner function
(TRY301)
170-170: Avoid specifying long messages outside the exception class
(TRY003)
173-173: Consider moving this statement to an else block
(TRY300)
179-179: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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: tox-tests
🔇 Additional comments (9)
tests/model_registry/model_catalog/conftest.py (2)
388-392: LGTM!The fixture correctly retrieves catalog routes using the appropriate label selector and namespace filter.
395-405: LGTM!The fixture correctly constructs HTTPS URLs from model catalog routes with appropriate error handling for missing routes.
tests/model_registry/conftest.py (1)
321-369: LGTM!The fixture correctly manages model registry metadata and database resources, with appropriate handling for both post-upgrade scenarios and different database backends (default, mysql, mariadb).
tests/model_registry/model_registry/conftest.py (6)
42-54: LGTM!The fixture correctly retrieves REST endpoints from model registry services with appropriate error handling for missing services.
57-84: Verifyis_secure=Falseis intentional for the test environment.Line 79 sets
is_secure=Falsewhen creating the ModelRegistryClient. While this is acceptable in test environments (likely disabling certificate verification), please confirm this is intentional and that the test environment uses self-signed certificates or similar.
87-101: LGTM!The fixture correctly registers a model using parameterized test data, properly delegating to the model registry client.
104-129: LGTM!These helper fixtures correctly construct REST URLs, headers, and retrieve deployment containers and pods. The implementations are straightforward and appropriate for test fixtures.
132-153: LGTM!The fixtures correctly create test namespace and service account resources with proper context manager usage for automatic cleanup.
200-272: LGTM!The RBAC fixtures correctly create Role and RoleBinding resources with appropriate permissions (get access to specific MR service) and proper label tagging for test cleanup.
|
/build-push-pr-image |
|
Status of building tag pr-976: success. |
|
/build-push-pr-image |
|
Status of building tag pr-976: success. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_registry/conftest.py (1)
281-318: Acknowledge pre-existing issues in moved code.This fixture consolidates MR instance management and the logic is sound. As noted in previous review comments, the 60-second sleep (line 314) and the unused
model_registry_metadata_db_resourcesparameter (intentional for fixture dependency ordering) are pre-existing issues being tracked separately (RHOAIENG-41728).The invalid
noqa: FCN001directive should still be removed as a quick cleanup since it serves no purpose.
🧹 Nitpick comments (2)
tests/model_registry/conftest.py (1)
372-376: Consider moving the import to the top of the file.The lazy import inside the fixture works but is unconventional. Since this module already imports from
tests.model_registry.utils, theget_rest_headersimport could be added to the existing import block at the top of the file for consistency.🔎 Suggested refactor
Add to existing imports at the top:
from tests.model_registry.utils import ( get_byoidc_user_credentials, get_model_registry_objects, get_model_registry_metadata_resources, wait_for_default_resource_cleanedup, + get_rest_headers, )Then simplify the fixture:
@pytest.fixture(scope="class") def model_registry_rest_headers(current_client_token: str) -> dict[str, str]: - from tests.model_registry.utils import get_rest_headers - return get_rest_headers(token=current_client_token)tests/model_registry/model_registry/conftest.py (1)
150-191: Prefix unused variables with underscore.The
resanderrvariables fromrun_commandare not used. Prefix them with underscores to indicate intentional non-use.🔎 Suggested fix
- res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30) + _res, out, _err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)The exception handling is thorough and provides good diagnostic information for different failure modes. The broad
except Exceptionis acceptable here since it re-raises after logging.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/conftest.pytests/model_registry/model_registry/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/conftest.py (3)
utilities/general.py (1)
wait_for_oauth_openshift_deployment(474-492)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)tests/model_registry/utils.py (3)
get_byoidc_user_credentials(853-896)get_model_registry_objects(542-575)get_model_registry_metadata_resources(578-624)
🪛 Ruff (0.14.10)
tests/model_registry/conftest.py
286-286: Unused function argument: model_registry_metadata_db_resources
(ARG001)
314-314: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
tests/model_registry/model_registry/conftest.py
38-38: Possible hardcoded password assigned to: "DEFAULT_TOKEN_DURATION"
(S105)
52-52: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
161-161: Unpacked variable res is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
161-161: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
164-164: Abstract raise to an inner function
(TRY301)
164-164: Avoid specifying long messages outside the exception class
(TRY003)
167-167: Consider moving this statement to an else block
(TRY300)
173-173: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (12)
tests/model_registry/conftest.py (3)
4-5: LGTM!The import additions are appropriate for the new fixtures added below.
25-31: LGTM!Import updates correctly reflect the consolidated fixture utilities being used in this refactored conftest.
320-369: LGTM!The
model_registry_metadata_db_resourcesfixture properly handles both post-upgrade and normal test flows:
- Uses
ExitStackfor proper context management of multiple resources- Creates resources in the correct order (Secret → PVC → Service → ConfigMap → Deployment)
- Waits for deployments to be ready before yielding
- Handles the
defaultdb_backend case by yielding empty resourcestests/model_registry/model_registry/conftest.py (9)
1-36: LGTM!Imports are appropriate for the fixtures defined in this file. Note:
subprocess(line 2) is imported but only used for type checking in the exception handling block, which is acceptable.
38-39: False positive from static analysis.The
S105hint flagging this as a "possible hardcoded password" is incorrect—DEFAULT_TOKEN_DURATION = "10m"is clearly a duration string for token validity, not a credential.
41-53: LGTM!The fixture correctly retrieves REST endpoints for all model registry instances and raises an appropriate error if none are found.
56-83: LGTM!The fixture properly creates ModelRegistryClient instances for each endpoint with correct HTTPS protocol handling.
103-106: LGTM!Simple and correct URL transformation.
109-123: LGTM!Both fixtures correctly access deployment containers and wait for pods with appropriate labels.
126-147: LGTM!Both
sa_namespaceandservice_accountfixtures properly use context managers for automatic cleanup and wait for resource readiness.
194-227: LGTM!The
mr_access_rolefixture correctly creates a Role with appropriate rules scoped to the specific MR service, with proper labeling for test identification.
230-267: LGTM!The
mr_access_role_bindingfixture correctly binds the service account group to the role with appropriate labels. Usingsystem:serviceaccounts:{namespace}as the Group subject is the correct pattern for granting access to all SAs in a namespace.
|
/build-push-pr-image |
|
Status of building tag pr-976: success. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_registry/model_registry/conftest.py (1)
83-97:AttributeErrorrisk if fixture is used without parametrization.Accessing
request.param.get()assumes the fixture is always parametrized. If used directly without@pytest.mark.parametrize,request.paramwill raiseAttributeError.🔎 Defensive approach
@pytest.fixture(scope="class") def registered_model( request: FixtureRequest, model_registry_client: list[ModelRegistryClient] ) -> Generator[RegisteredModel, None, None]: + param = getattr(request, "param", {}) yield model_registry_client[0].register_model( - name=request.param.get("model_name"), - uri=request.param.get("model_uri"), - version=request.param.get("model_version"), - description=request.param.get("model_description"), - model_format_name=request.param.get("model_format"), - model_format_version=request.param.get("model_format_version"), - storage_key=request.param.get("model_storage_key"), - storage_path=request.param.get("model_storage_path"), - metadata=request.param.get("model_metadata"), + name=param.get("model_name"), + uri=param.get("model_uri"), + version=param.get("model_version"), + description=param.get("model_description"), + model_format_name=param.get("model_format"), + model_format_version=param.get("model_format_version"), + storage_key=param.get("model_storage_key"), + storage_path=param.get("model_storage_path"), + metadata=param.get("model_metadata"), )
🧹 Nitpick comments (2)
tests/model_registry/model_registry/conftest.py (1)
123-164: Prefix unused unpacked variables with underscores.At Line 134,
resanderrare unpacked but never used. Prefix them with underscores to indicate they are intentionally unused:_, out, _ = run_command(...).🔎 Proposed fix
- res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30) + _, out, _ = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)tests/model_registry/conftest.py (1)
372-377: Consider moving import to module level.The import at Line 374 is inside the function. For consistency with other imports in the file and to avoid repeated imports on each fixture invocation, consider moving it to the top of the file.
🔎 Suggested refactor
At the top of the file with other imports:
+from tests.model_registry.utils import get_rest_headersThen simplify the fixture:
@pytest.fixture(scope="class") def model_registry_rest_headers(current_client_token: str) -> dict[str, str]: - from tests.model_registry.utils import get_rest_headers - return get_rest_headers(token=current_client_token)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/conftest.pytests/model_registry/model_registry/conftest.py
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/model_registry/conftest.py
286-286: Unused function argument: model_registry_metadata_db_resources
(ARG001)
314-314: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
tests/model_registry/model_registry/conftest.py
35-35: Possible hardcoded password assigned to: "DEFAULT_TOKEN_DURATION"
(S105)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Unpacked variable res is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
134-134: Unpacked variable err is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
137-137: Abstract raise to an inner function
(TRY301)
137-137: Avoid specifying long messages outside the exception class
(TRY003)
140-140: Consider moving this statement to an else block
(TRY300)
146-146: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (4)
tests/model_registry/model_registry/conftest.py (2)
53-80: Verifyis_secure=Falseis appropriate for the test environment.The
is_secure=Falseparameter at Line 75 disables TLS certificate verification. This is typical for test environments with self-signed certificates, but ensure this aligns with your security testing requirements.
167-240: LGTM! Well-structured RBAC fixtures.The Role and RoleBinding fixtures are properly implemented with:
- Appropriate resource restrictions (Line 183 limits access to specific MR service)
- Correct context manager usage for automatic cleanup
- Good labeling for tracking test resources
tests/model_registry/conftest.py (2)
320-370: LGTM! Well-structured resource management.The fixture properly handles different scenarios:
- Post-upgrade path: Ensures existing resources, then cleans up
- Default backend: Returns empty dict (no resources needed)
- Other backends: Creates resources with proper context management via
ExitStackSafe parameter access pattern using
getattrat Lines 328-329 prevents attribute errors.
379-405: LGTM! Namespace and ServiceAccount fixtures are well-implemented.Both fixtures properly use context managers for automatic cleanup and include appropriate waiting for resource readiness.
|
/build-push-pr-image |
|
Status of building tag pr-976: success. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/model_registry/model_catalog/conftest.py (1)
403-413: Consider minor refinements to URL construction.The fixture correctly builds REST URLs from routes and includes appropriate assertions. Two optional improvements:
- Port 443 is redundant in HTTPS URLs (line 407) as it's the default port.
- The assertion message at lines 409-412 could be more precise—it checks that URL construction succeeded rather than route existence.
🔎 Proposed refinements
@pytest.fixture(scope="class") def model_catalog_rest_url(model_registry_namespace: str, model_catalog_routes: list[Route]) -> list[str]: assert model_catalog_routes, f"Model catalog routes does not exist in {model_registry_namespace}" - route_urls = [ - f"https://{route.instance.spec.host}:443/api/model_catalog/v1alpha1/" for route in model_catalog_routes - ] - assert route_urls, ( - "Model catalog routes information could not be found from " - f"routes:{[route.name for route in model_catalog_routes]}" - ) + route_urls = [f"https://{route.instance.spec.host}/api/model_catalog/v1alpha1/" for route in model_catalog_routes] + assert route_urls, f"Failed to build URLs from routes: {[route.name for route in model_catalog_routes]}" return route_urls
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_registry/model_catalog/conftest.py
🔇 Additional comments (3)
tests/model_registry/model_catalog/conftest.py (3)
12-12: LGTM!The Route import is correctly added to support the new
model_catalog_routesfixture.
336-365: LGTM!The addition of readiness synchronization (
is_model_catalog_readyandwait_for_model_catalog_api) ensures the model catalog is ready both after applying patches and after teardown. This improves test reliability and is consistent with similar fixtures in this file.
396-400: LGTM!The fixture correctly retrieves model catalog routes using label selector and converts the generator to a list. The implementation is clean and properly typed.
dbasunag
left a comment
dbasunag
left a comment
There was a problem hiding this comment.
I like the over all direction of this PR. A few things to consider:
- Should we rename the base package to ai_hub?
- We should create tickets for testops: both for upgrade and health etc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/model_registry/cluster_health/test_mr_operator_health.py (1)
1-4: Consider organizing imports per PEP 8 convention.Group imports as: standard library, third-party packages, then local modules. Currently
simple_logger(line 1) precedespytest(line 3).📦 Proposed import organization
-from simple_logger.logger import get_logger - import pytest from ocp_utilities.monitoring import Prometheus +from simple_logger.logger import get_logger
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/cluster_health/__init__.pytests/model_registry/cluster_health/conftest.pytests/model_registry/cluster_health/test_mr_operator_health.pytests/model_registry/conftest.py
💤 Files with no reviewable changes (1)
- tests/model_registry/cluster_health/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/cluster_health/test_mr_operator_health.py (1)
tests/model_registry/cluster_health/conftest.py (1)
prometheus_for_monitoring(15-19)
🪛 Ruff (0.14.10)
tests/model_registry/conftest.py
289-289: Unused function argument: model_registry_metadata_db_resources
(ARG001)
317-317: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (4)
tests/model_registry/cluster_health/test_mr_operator_health.py (1)
9-19: LGTM! Health check test is well-structured.The test appropriately:
- Runs last to capture any OOMKilled events during test execution
- Queries Prometheus for the specific termination reason
- Filters results by pod name prefix to target only the model registry operator
- Provides clear failure messages with the full entry details
tests/model_registry/conftest.py (3)
284-321: LGTM! Fixture correctly manages model registry instances.The
model_registry_instancefixture appropriately:
- Handles both post-upgrade and normal test scenarios
- Uses ExitStack for proper context management of multiple resources
- Waits for conditions before yielding instances
- Cleans up default resources when needed
Note: The
model_registry_metadata_db_resourcesparameter ensures pytest executes that fixture first (dependency ordering), even though it's not directly referenced in the function body.
324-373: LGTM! Database resources fixture is well-implemented.The fixture correctly:
- Manages different resource types (Secret, PVC, Service, ConfigMap, Deployment) via ExitStack
- Handles post-upgrade scenario by ensuring existing resources exist
- Waits for deployment readiness before yielding
- Supports both "default" and custom database backends
- Properly tears down resources in reverse order
376-403: LGTM! Helper fixtures are correctly implemented.The three helper fixtures (
model_registry_rest_headers,sa_namespace,service_account) provide clean abstractions for:
- REST API authentication headers
- Temporary namespace creation with proper lifecycle management
- Service account provisioning
All use appropriate context managers and waiting mechanisms.
|
Status of building tag latest: success. |
…opendatahub-io#976) * test: reorganize test structure in preparation of top folder renaming Restructure model_registry tests into a clearer hierarchy: - Split into model_registry/ and model_catalog/ top-level domains - Group model_catalog tests by function: catalog_config, metadata, upgrade
4 participants
Restructure model_registry tests into a clearer hierarchy:
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
Release Notes
Tests
Note: This release contains internal test infrastructure improvements with no direct end-user impact.
✏️ Tip: You can customize this high-level summary in your review settings.