Parametrized fixtures for multitenancy#461
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes refactor the test infrastructure for model registry multi-instance scenarios. Duplicated resource fixtures are replaced with parameterized fixtures capable of managing multiple resources at once. Supporting utilities and constants are introduced, and the affected RBAC test is rewritten to use the new parameterized approach. This unifies and generalizes resource setup for multi-instance testing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/cherry-pick', '/lgtm', '/build-push-pr-image', '/hold', '/verified'} |
|
/build-push-pr-image |
![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 (6)
tests/model_registry/multiple_instance_utils.py (2)
11-12: Consider making namespace generation more deterministic for debugging.While UUIDs ensure uniqueness, consider adding a timestamp or test run identifier to make debugging easier when tests fail.
-ns_name = f"{MR_INSTANCE_NAME}-ns-{str(uuid.uuid4())[:8]}" +import time +ns_name = f"{MR_INSTANCE_NAME}-ns-{int(time.time())}-{str(uuid.uuid4())[:8]}"
14-14: Remove unnecessary noqa comment.The
# noqa: F821comment appears to be unnecessary sinceNUM_MR_INSTANCESis properly imported.-db_names = [f"{DB_RESOURCES_NAME}-{i + 1}-{str(uuid.uuid4())[:8]}" for i in range(NUM_MR_INSTANCES)] # noqa: F821 +db_names = [f"{DB_RESOURCES_NAME}-{i + 1}-{str(uuid.uuid4())[:8]}" for i in range(NUM_MR_INSTANCES)]tests/model_registry/rbac/test_mr_rbac.py (1)
267-273: Consider extracting cleanup to a fixture or helper.The cleanup code at the end could be moved to a fixture teardown or a finally block to ensure it runs even if the test fails.
- # Clean up - revoke access from the second instance - revoke_mr_access( - admin_client=admin_client, - user=test_idp_user.username, - mr_instance_name=mr_instance_2.name, - model_registry_namespace=model_registry_namespace, - ) + try: + # existing test code... + finally: + # Clean up - revoke access from the second instance + revoke_mr_access( + admin_client=admin_client, + user=test_idp_user.username, + mr_instance_name=mr_instance_2.name, + model_registry_namespace=model_registry_namespace, + )tests/model_registry/conftest.py (3)
662-662: Fix inconsistent variable naming.The variable should be
pvcs(plural) to match the pattern used in other fixtures.- yield pvc + yield pvcsAlso update the variable name in the list comprehension:
- pvc = [ + pvcs = [
724-731: Consider extracting OAuth/OSSM logic to a helper function.The OAuth vs OSSM configuration logic is duplicated from the non-parametrized fixture. Consider extracting it to reduce duplication.
def _get_mr_class_and_config(is_oauth: bool) -> tuple[type, dict, dict]: """Return the appropriate MR class and configuration based on auth type.""" if is_oauth: LOGGER.warning("Requested OAuth Proxy configuration:") return ModelRegistry, None, OAUTH_PROXY_CONFIG_DICT else: LOGGER.warning("Requested OSSM configuration:") return ModelRegistryV1Alpha1, ISTIO_CONFIG_DICT, NoneThen use it in both fixtures:
- if param.get("is_model_registry_oauth"): - LOGGER.warning("Requested Ouath Proxy configuration:") - oauth_config = OAUTH_PROXY_CONFIG_DICT - mr_class = ModelRegistry - else: - LOGGER.warning("Requested OSSM configuration:") - istio_config = ISTIO_CONFIG_DICT - mr_class = ModelRegistryV1Alpha1 + mr_class, istio_config, oauth_config = _get_mr_class_and_config( + param.get("is_model_registry_oauth") + )
725-725: Fix typo in log message."Ouath" should be "OAuth".
- LOGGER.warning("Requested Ouath Proxy configuration:") + LOGGER.warning("Requested OAuth Proxy configuration:")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/conftest.py(2 hunks)tests/model_registry/constants.py(1 hunks)tests/model_registry/multiple_instance_utils.py(1 hunks)tests/model_registry/rbac/test_mr_rbac.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/rbac/test_mr_rbac.py (6)
tests/model_registry/rbac/conftest.py (2)
test_idp_user(238-275)login_as_test_user(358-372)utilities/user_utils.py (1)
UserTestSession(21-43)tests/model_registry/conftest.py (7)
updated_dsc_component_state_parametrized(542-620)db_secret_parametrized(624-640)db_pvc_parametrized(644-662)db_service_parametrized(666-682)db_deployment_parametrized(686-710)model_registry_instance_parametrized(714-755)model_registry_namespace(55-56)tests/model_registry/utils.py (2)
get_mr_service_by_label(25-49)get_endpoint_from_mr_service(52-56)tests/model_registry/rbac/utils.py (2)
grant_mr_access(112-149)revoke_mr_access(152-169)utilities/constants.py (1)
Protocols(91-98)
🔇 Additional comments (5)
tests/model_registry/constants.py (1)
63-64: LGTM!The constant is well-placed and clearly named for its purpose in multi-instance testing scenarios.
tests/model_registry/rbac/test_mr_rbac.py (2)
147-158: Well-structured parametrization!The parametrization properly includes all required fixtures and uses indirect=True for proper fixture resolution.
181-184: Good validation of expected instances.The check ensures the test receives the expected number of instances, preventing silent failures.
tests/model_registry/conftest.py (2)
541-621: Excellent DSC state management!The two-phase approach (remove then manage) properly handles namespace changes, and the cleanup logic ensures proper restoration of the original state.
626-640: Good use of ExitStack for resource management.The pattern ensures all resources are properly cleaned up even if exceptions occur during creation.
|
Status of building tag pr-461: success. |
|
/build-push-pr-image |
|
Status of building tag pr-461: success. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/model_registry/conftest.py (1)
673-715: Add pod readiness check tomodel_registry_instance_parametrizedand update its signatureYou should mirror the non-parameterized fixture’s pod-readiness wait and clarify the suppressed lint rule:
• In
tests/model_registry/conftest.pyaround line 673, extend the fixture signature to acceptadmin_client: DynamicClient.
• After yielding or appending each MR instance, callwait_for_pods_running(...)for each namespace.
• Remove or document the# noqa: FCN001on theenter_contextcall—identify which naming rule is being silenced.Suggested diff:
--- a/tests/model_registry/conftest.py +++ b/tests/model_registry/conftest.py @@ -673,7 +673,9 @@ def model_registry_instance_parametrized( - request: FixtureRequest, teardown_resources: bool +) request: FixtureRequest, + teardown_resources: bool, + admin_client: DynamicClient, ) -> Generator[List[ModelRegistry], Any, Any]: """Create Model Registry instance parametrized""" @@ -703,6 +705,17 @@ def model_registry_instance_parametrized( model_registry_instances.append(mr_instance) + # Wait for all MR pods to be running in each namespace + namespaces = { + param.get("ns_name", py_config["model_registry_namespace"]) + for param in request.param + } + for ns in namespaces: + wait_for_pods_running( + admin_client=admin_client, + namespace_name=ns, + number_of_consecutive_checks=6, + ) + LOGGER.info( f"Created {len(model_registry_instances)} MR instances: " f"{[mr.name for mr in model_registry_instances]}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_registry/conftest.py(2 hunks)tests/model_registry/multiple_instance_utils.py(1 hunks)tests/model_registry/rbac/test_mr_rbac.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/model_registry/rbac/test_mr_rbac.py
- tests/model_registry/multiple_instance_utils.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
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.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:666-676
Timestamp: 2025-07-17T15:41:54.284Z
Learning: In tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the admin_client parameter in their signatures, unlike some other Secret fixtures in the codebase. The user lugi0 confirmed this is the correct pattern for these specific fixtures.
tests/model_registry/conftest.py (12)
Learnt from: lugi0
PR: #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.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
Learnt from: lugi0
PR: #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.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:666-676
Timestamp: 2025-07-17T15:41:54.284Z
Learning: In tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the admin_client parameter in their signatures, unlike some other Secret fixtures in the codebase. The user lugi0 confirmed this is the correct pattern for these specific fixtures.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.
Learnt from: Snomaan6846
PR: #444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.
Learnt from: dbasunag
PR: #401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: #354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: adolfo-ab
PR: #334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: #338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
Learnt from: dbasunag
PR: #354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, Service resources can be created without explicitly passing the admin_client parameter when using the context manager approach with "with Service()". The client parameter is optional for Service resource creation.
🔇 Additional comments (5)
tests/model_registry/conftest.py (5)
22-23: LGTM!The addition of
ExitStackandListimports is appropriate for the parameterized fixtures implementation.
542-580: Well-structured parameterized DSC fixture implementation.The fixture properly handles dynamic namespace configuration for Model Registry with appropriate cleanup logic. The early return pattern for
teardown_resources=Falseis consistent with other fixtures in the codebase.
583-600: LGTM!The parameterized Secret fixture correctly implements the multi-resource pattern using ExitStack and follows the established conventions for Secret resources in this codebase.
603-622: LGTM!The PVC fixture follows the same clean pattern as the Secret fixture with proper resource management.
625-642: LGTM!The Service fixture correctly implements parameterization and follows the established pattern of not requiring admin_client.
rhods-ci-bot
added
changes-requested-by-adolfo-ab
and removed
changes-requested-by-adolfo-ab
labels
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/model_registry/conftest.py (2)
633-633: Consider adding a default for ports parameter.The
portsparameter lacks a default value, which could cause issues if not provided in the parameter dictionary. Consider adding a sensible default based on the existingmodel_registry_db_servicefixture.Apply this diff to add a default ports configuration:
- ports=param.get("ports"), + ports=param.get("ports", [ + { + "name": "mysql", + "nodePort": 0, + "port": 3306, + "protocol": "TCP", + "appProtocol": "tcp", + "targetPort": 3306, + } + ]),
652-665: Consider adding deployment annotations for consistency.The original
model_registry_db_deploymentfixture includes specific annotations that are missing in this parameterized version. This could affect deployment behavior.Apply this diff to add the missing annotations:
Deployment( name=param.get("db_name"), namespace=param.get("ns_name", py_config["model_registry_namespace"]), + annotations={ + "template.alpha.openshift.io/wait-for-ready": "true", + }, template=get_model_registry_deployment_template_dict( secret_name=param.get("db_name"), resource_name=param.get("db_name") ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/conftest.py(2 hunks)tests/model_registry/multiple_instance_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/multiple_instance_utils.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
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.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:666-676
Timestamp: 2025-07-17T15:41:54.284Z
Learning: In tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the admin_client parameter in their signatures, unlike some other Secret fixtures in the codebase. The user lugi0 confirmed this is the correct pattern for these specific fixtures.
tests/model_registry/conftest.py (15)
Learnt from: lugi0
PR: #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.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
Learnt from: lugi0
PR: #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.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:666-676
Timestamp: 2025-07-17T15:41:54.284Z
Learning: In tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the admin_client parameter in their signatures, unlike some other Secret fixtures in the codebase. The user lugi0 confirmed this is the correct pattern for these specific fixtures.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.
Learnt from: Snomaan6846
PR: #444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.
Learnt from: dbasunag
PR: #401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: #354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: adolfo-ab
PR: #334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: #338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
Learnt from: jiripetrlik
PR: #335
File: tests/rag/test_rag.py:0-0
Timestamp: 2025-06-19T14:32:17.658Z
Learning: The wait_for_replicas() method from ocp_resources.deployment.Deployment raises an exception when timeout is reached rather than returning a boolean value, so it doesn't need explicit return value checking in tests.
Learnt from: jiripetrlik
PR: #335
File: tests/rag/test_rag.py:0-0
Timestamp: 2025-06-19T14:32:17.658Z
Learning: The wait_for_replicas() method from ocp_resources.deployment.Deployment raises an exception when timeout is reached rather than returning a boolean value, so it doesn't need explicit return value checking in tests.
Learnt from: israel-hdez
PR: #346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper create_isvc (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: dbasunag
PR: #354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, Service resources can be created without explicitly passing the admin_client parameter when using the context manager approach with "with Service()". The client parameter is optional for Service resource creation.
🔇 Additional comments (4)
tests/model_registry/conftest.py (4)
22-23: LGTM: Appropriate imports for parameterized fixtures.The
ExitStackandListimports are correctly added to support the new parameterized fixture pattern that manages multiple resources.
583-599: LGTM: Well-implemented parameterized secret fixture.The fixture correctly uses
ExitStackto manage multiple Secret resources with proper parameter extraction and default namespace handling. The implementation aligns with the established patterns for these parameterized fixtures.
603-621: LGTM: Consistent parameterized PVC fixture implementation.The fixture properly implements the parameterized pattern with appropriate defaults for
accessmodesandsizeparameters. The resource creation and management is correctly handled.
677-718: LGTM: Comprehensive parameterized ModelRegistry fixture.The fixture correctly handles the complexity of creating multiple ModelRegistry instances with different configurations (OAuth vs OSSM). The implementation properly:
- Selects the appropriate class based on configuration
- Sets up common parameters efficiently
- Waits for required conditions
- Provides useful debugging logs
|
/build-push-pr-image |
|
Status of building tag pr-461: success. |
rhods-ci-bot
added
lgtm-by-adolfo-ab
lgtm-by-lugi0
and removed
Verified
|
Status of building tag latest: success. |
5 participants
Description
How Has This Been Tested?
Merge criteria: