test: add session fixture to enable all catalogs for tests#903
test: add session fixture to enable all catalogs for tests#903fege merged 7 commits intoopendatahub-io:mainfrom
Conversation
Introduce `enabled_model_catalog_config_map` session-scoped fixture to centrally manage catalog enablement across all model catalog tests.
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/cherry-pick', '/hold', '/build-push-pr-image', '/verified', '/lgtm'} |
📝 WalkthroughWalkthroughThis change introduces a new session-scoped pytest fixture that enables all model catalog sources by reading, updating, and writing Kubernetes ConfigMaps. The fixture is then integrated into multiple test methods across the model catalog test suite. Additionally, error handling is added to manage transient 401 authorization errors in model catalog API utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/model_registry/model_catalog/conftest.py (1)
40-79: Consider adding cleanup call tois_model_catalog_readyafter theResourceEditorcontext exits.The fixture correctly enables all catalogs via
ResourceEditor, but unlike the similarupdated_catalog_config_mapfixture (lines 91-113), it doesn't callis_model_catalog_readyafter the context manager exits to ensure proper cleanup/reset of the model catalog pods.Since this is a session-scoped fixture, missing cleanup may not affect individual test runs, but could leave the environment in an unexpected state if the fixture is used in isolation or if tests run after session teardown depend on the default state.
Consider adding cleanup for consistency:
with ResourceEditor(patches={user_sources_cm: patches}): is_model_catalog_ready(client=admin_client, model_registry_namespace=model_registry_namespace) yield user_sources_cm + is_model_catalog_ready(client=admin_client, model_registry_namespace=model_registry_namespace)tests/model_registry/model_catalog/test_model_search.py (1)
429-436: Consider parameter ordering for consistency.In
test_q_parameter_empty_query, theenabled_model_catalog_config_mapfixture parameter appears aftersearch_term(line 433), while in all other methods it appears first. Consider reordering for consistency across the test suite.@pytest.mark.parametrize("search_term", ["", None]) def test_q_parameter_empty_query( self: Self, - search_term, enabled_model_catalog_config_map: ConfigMap, + search_term, model_catalog_rest_url: list[str], model_registry_rest_headers: dict[str, str], ):tests/model_registry/model_catalog/test_default_model_catalog.py (1)
164-171: Handle intentionally unusedenabled_model_catalog_config_mapto satisfy Ruff
enabled_model_catalog_config_map: ConfigMapis added to these tests purely to trigger the session-scoped fixture’s side effects, so it’s intentionally unused in the bodies. Ruff now reports ARG002 for each of these parameters.To keep the semantics while avoiding repeated warnings, consider one of:
- Make the fixture autouse for the relevant scope and drop it from the signatures, for example in this module or per class:
pytestmark = [ ..., pytest.mark.usefixtures("enabled_model_catalog_config_map"), ]or
@pytest.mark.usefixtures("enabled_model_catalog_config_map")on the specific classes.
- If you prefer the explicit parameter, mark it as intentionally unused, e.g.:
def test_model_default_catalog_get_model_by_name( self: Self, enabled_model_catalog_config_map: ConfigMap, ... ): del enabled_model_catalog_config_map # consume fixture, silence ARG002 ...or add a
# noqa: ARG002comment on the parameter line per your linting conventions.This keeps the new catalog-enabling behavior while making the linter happy.
Also applies to: 201-248, 258-378
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/model_registry/model_catalog/conftest.py(2 hunks)tests/model_registry/model_catalog/test_custom_model_catalog.py(0 hunks)tests/model_registry/model_catalog/test_default_model_catalog.py(7 hunks)tests/model_registry/model_catalog/test_filter_options_endpoint.py(3 hunks)tests/model_registry/model_catalog/test_model_catalog_negative.py(1 hunks)tests/model_registry/model_catalog/test_model_search.py(15 hunks)tests/model_registry/utils.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/model_registry/model_catalog/test_custom_model_catalog.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/model_registry/utils.py (1)
tests/model_registry/model_catalog/conftest.py (1)
expected_catalog_values(117-118)
tests/model_registry/model_catalog/test_filter_options_endpoint.py (1)
tests/model_registry/model_catalog/conftest.py (1)
enabled_model_catalog_config_map(41-78)
tests/model_registry/model_catalog/conftest.py (3)
tests/conftest.py (1)
admin_client(78-79)tests/model_registry/conftest.py (1)
model_registry_namespace(75-76)tests/model_registry/utils.py (1)
is_model_catalog_ready(674-686)
tests/model_registry/model_catalog/test_default_model_catalog.py (1)
tests/model_registry/model_catalog/conftest.py (1)
enabled_model_catalog_config_map(41-78)
tests/model_registry/model_catalog/test_model_search.py (2)
tests/model_registry/model_catalog/conftest.py (1)
enabled_model_catalog_config_map(41-78)tests/model_registry/conftest.py (1)
model_catalog_rest_url(640-649)
🪛 Ruff (0.14.7)
tests/model_registry/model_catalog/test_filter_options_endpoint.py
48-48: Unused method argument: enabled_model_catalog_config_map
(ARG002)
101-101: Unused method argument: enabled_model_catalog_config_map
(ARG002)
tests/model_registry/model_catalog/test_default_model_catalog.py
167-167: Unused method argument: enabled_model_catalog_config_map
(ARG002)
203-203: Unused method argument: enabled_model_catalog_config_map
(ARG002)
216-216: Unused method argument: enabled_model_catalog_config_map
(ARG002)
234-234: Unused method argument: enabled_model_catalog_config_map
(ARG002)
260-260: Unused method argument: enabled_model_catalog_config_map
(ARG002)
277-277: Unused method argument: enabled_model_catalog_config_map
(ARG002)
331-331: Unused method argument: enabled_model_catalog_config_map
(ARG002)
tests/model_registry/model_catalog/test_model_search.py
36-36: Unused method argument: enabled_model_catalog_config_map
(ARG002)
68-68: Unused method argument: enabled_model_catalog_config_map
(ARG002)
111-111: Unused method argument: enabled_model_catalog_config_map
(ARG002)
155-155: Unused method argument: enabled_model_catalog_config_map
(ARG002)
221-221: Unused method argument: enabled_model_catalog_config_map
(ARG002)
260-260: Unused method argument: enabled_model_catalog_config_map
(ARG002)
312-312: Unused method argument: enabled_model_catalog_config_map
(ARG002)
352-352: Unused method argument: enabled_model_catalog_config_map
(ARG002)
403-403: Unused method argument: enabled_model_catalog_config_map
(ARG002)
433-433: Unused method argument: enabled_model_catalog_config_map
(ARG002)
451-451: Unused method argument: enabled_model_catalog_config_map
(ARG002)
497-497: Unused method argument: enabled_model_catalog_config_map
(ARG002)
543-543: Unused method argument: enabled_model_catalog_config_map
(ARG002)
583-583: Unused method argument: enabled_model_catalog_config_map
(ARG002)
🔇 Additional comments (10)
tests/model_registry/utils.py (2)
744-744: LGTM!The logging prefix improves clarity by identifying the log output context.
746-746: Verify the removal of the +2 offset is correct for all test scenarios.The assertion now expects
len(results) == len(expected_catalog_values)without the previous +2 offset. This change assumes theenabled_model_catalog_config_mapfixture now explicitly enables all catalogs, making the expected count match exactly.Ensure this change doesn't break tests where the fixture is not used or where additional default catalogs might still be present. Verify that all test paths through this assertion account for the expected catalog count consistently.
tests/model_registry/model_catalog/test_model_catalog_negative.py (1)
84-95: LGTM!Adding
enabled: trueto the catalog configuration aligns with the PR's objective of enabling catalogs for tests. This ensures the negative test scenario properly tests rejection of duplicate enabled catalogs in the custom configmap.tests/model_registry/model_catalog/conftest.py (2)
20-24: LGTM!The imports are correctly updated to include the constants needed for the new fixture.
57-65: Consider handlingyaml.safe_loadreturningNonefor empty YAML.If
default_sources_yamlis an empty string,yaml.safe_load("")returnsNone, which is correctly handled by theif parsed_yaml and "catalogs" in parsed_yamlcheck. However, logging a warning and falling back to the original (empty) sources YAML may not be the intended behavior.Verify this is the expected behavior when the default sources ConfigMap has no catalogs defined, or if an error should be raised instead.
tests/model_registry/model_catalog/test_filter_options_endpoint.py (3)
4-4: LGTM!Import added for the
ConfigMaptype hint used with the new fixture parameter.
46-52: Fixture dependency pattern is correct; static analysis warning can be ignored.The
enabled_model_catalog_config_mapparameter is intentionally added to ensure the fixture runs before the test, enabling all catalogs. The parameter appears unused because the test relies on the fixture's side effect (catalog enablement), not its return value. This is a standard pytest pattern for setup fixtures.The Ruff ARG002 warning is a false positive in this context.
99-105: Same fixture dependency pattern; approved.Consistent with the previous test method, this fixture parameter ensures catalogs are enabled before database validation tests run.
tests/model_registry/model_catalog/test_model_search.py (2)
3-3: LGTM!Import added for the
ConfigMaptype hint.
34-39: Fixture dependency pattern is intentional; static analysis warnings are false positives.The
enabled_model_catalog_config_mapparameter is added to 14 test methods across this file to ensure the session-scoped fixture runs before any model catalog tests. This is a standard pytest pattern where the fixture's side effect (enabling all catalogs in the ConfigMap) is required, not its return value.All Ruff ARG002 warnings for this fixture parameter throughout the file can be safely ignored.
|
/build-push-pr-image |
|
Status of building tag pr-903: success. |
|
/build-push-pr-image |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_registry/model_catalog/conftest.py (1)
41-79: Fixture behavior looks correct; consider preserving extra YAML keys and extracting a helperThe overall flow of
enabled_model_catalog_config_maplooks solid: it fails fast when catalogs are missing, enables all catalogs, patches the user-managed ConfigMap viaResourceEditor, and waits for readiness before yielding.Two optional cleanups you might consider:
- If
sources.yamlever gains additional top-level keys, constructingenabled_yaml_dict = {"catalogs": parsed_yaml["catalogs"]}will silently drop them. Preserving them is safer:- for catalog in parsed_yaml["catalogs"]: - catalog["enabled"] = True - enabled_yaml_dict = {"catalogs": parsed_yaml["catalogs"]} - enabled_sources_yaml = yaml.dump(enabled_yaml_dict, default_flow_style=False, sort_keys=False) + for catalog in parsed_yaml["catalogs"]: + catalog["enabled"] = True + # Preserve any additional top-level keys instead of dropping everything but "catalogs". + enabled_yaml_dict = dict(parsed_yaml) + enabled_yaml_dict["catalogs"] = parsed_yaml["catalogs"] + enabled_sources_yaml = yaml.dump(enabled_yaml_dict, default_flow_style=False, sort_keys=False)
- The YAML parsing and “enable all catalogs” logic is fairly self-contained and might be worth moving into a small utility in
tests.model_registry.utilsif you expect to reuse it elsewhere, as was suggested in earlier review feedback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/conftest.py(2 hunks)tests/model_registry/model_catalog/test_custom_model_catalog.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/model_registry/model_catalog/conftest.py (3)
tests/conftest.py (1)
admin_client(78-79)tests/model_registry/conftest.py (1)
model_registry_namespace(75-76)tests/model_registry/utils.py (1)
is_model_catalog_ready(674-686)
tests/model_registry/model_catalog/test_custom_model_catalog.py (1)
tests/model_registry/model_catalog/conftest.py (1)
enabled_model_catalog_config_map(42-78)
🪛 Ruff (0.14.7)
tests/model_registry/model_catalog/conftest.py
60-60: Avoid specifying long messages outside the exception class
(TRY003)
tests/model_registry/model_catalog/test_custom_model_catalog.py
58-58: Unused method argument: enabled_model_catalog_config_map
(ARG002)
74-74: Unused method argument: enabled_model_catalog_config_map
(ARG002)
🔇 Additional comments (1)
tests/model_registry/model_catalog/conftest.py (1)
21-25: New constants imports are consistent with usageThe additional imports (CUSTOM_CATALOG_ID1, DEFAULT_MODEL_CATALOG_CM, DEFAULT_CUSTOM_MODEL_CATALOG) are all used below and line up with the new fixture behavior and existing helpers. No issues here.
|
Status of building tag pr-903: success. |
|
/build-push-pr-image |
|
Status of building tag pr-903: success. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/model_registry/utils.py (2)
702-716: 401 handling and long exception messages inexecute_get_callThe split between retryable 401s (
TransientUnauthorizedError) and other non-2xx responses (ResourceNotFoundError) makes sense forwait_for_model_catalog_api. To reduce duplication and satisfy Ruff (TRY003), consider shortening the f-string messages or centralizing the message construction inside the exception class (e.g., have the exception accepturlandresponseand build__str__there).
755-767: Tightened length assertion andexpected_catalog_valuestypeThe stricter
assert len(results) == len(expected_catalog_values)is fine if tests always enumerate all expected catalogs; if the API can legitimately return extra system catalogs, this may over-constrain the check and could be relaxed back to a subset assertion. Also,expected_catalog_valuesis annotated asdict[str, str]but is iterated as a sequence of dicts (expected_entry["id"]), so the runtime expectation is effectivelySequence[Mapping]. Updating the type hint (and any docstring) to reflect the actual structure would avoid confusion.tests/model_registry/model_catalog/conftest.py (1)
41-79: Behaviour and scope ofenabled_model_catalog_config_mapEnabling all catalogs by reading
DEFAULT_MODEL_CATALOG_CMand patchingDEFAULT_CUSTOM_MODEL_CATALOGviaResourceEditorlooks correct and matches the test intent. A couple of points to double‑check:
- The fixture rewrites
sources.yamlto only contain acatalogslist, dropping any other top‑level keys (labels, etc.) that might exist in the operator‑managed ConfigMap. If other tests depend on additional keys fromsources.yaml, consider preserving them instead of reconstructing a minimal dict.- The readiness gate here relies on
is_model_catalog_readyonly. If you still see transient 401s in tests that depend solely on this fixture, it might be worth also callingwait_for_model_catalog_api(mirroringupdated_catalog_config_map), at the cost of adding its dependencies to this fixture’s signature.The
RuntimeError("No catalogs found in default sources ConfigMap")path is also a reasonable early‑fail, and addresses the earlier comment about failing fast when inputs are bad.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/model_catalog/conftest.py(2 hunks)tests/model_registry/utils.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/model_registry/model_catalog/conftest.py (1)
tests/model_registry/utils.py (1)
is_model_catalog_ready(679-691)
tests/model_registry/utils.py (2)
tests/model_registry/model_catalog/utils.py (1)
ResourceNotFoundError(32-33)tests/model_registry/model_catalog/conftest.py (1)
expected_catalog_values(117-118)
🪛 Ruff (0.14.7)
tests/model_registry/model_catalog/conftest.py
60-60: Avoid specifying long messages outside the exception class
(TRY003)
tests/model_registry/utils.py
713-713: Avoid specifying long messages outside the exception class
(TRY003)
715-715: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
tests/model_registry/utils.py (2)
46-50: Custom transient error type is clear and self-containedDefining
TransientUnauthorizedErrorfor retryable 401s keeps retry logic explicit and readable; no issues from a correctness standpoint.
719-729:wait_for_model_catalog_apireadiness check looks robustChecking both
/sourcesand/modelsand retrying onResourceNotFoundErrorandTransientUnauthorizedErrorshould make the catalog API readiness much less flaky under OAuth/kube‑rbac‑proxy initialization; the implementation aligns with existing URL usage patterns in this file.tests/model_registry/model_catalog/conftest.py (1)
21-25: New constants import matches downstream usageImporting
CUSTOM_CATALOG_ID1,DEFAULT_MODEL_CATALOG_CM, andDEFAULT_CUSTOM_MODEL_CATALOGaligns with how they’re referenced later in this module; nothing else to adjust here.
|
Status of building tag latest: success. |
4 participants
Introduce
enabled_model_catalog_config_mapsession-scoped fixture to centrally manage catalog enablement across all model catalog tests.Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.