parameterize mariadb tests #504
Conversation
📝 WalkthroughWalkthroughUnifies DB resource fixtures into a backend-aware metadata fixture, changes DSC fixture scope to session, adds MariaDB support and related constants, removes legacy MariaDB test scaffolding, updates many tests to the new fixtures and parameterization, and adds pod-level and upgrade helper fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (24)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (16)
🧰 Additional context used🧠 Learnings (14)📚 Learning: 2025-08-11T12:14:12.803ZApplied to files:
📚 Learning: 2025-07-30T14:15:25.605ZApplied to files:
📚 Learning: 2025-07-17T15:42:23.880ZApplied to files:
📚 Learning: 2025-07-30T14:15:25.605ZApplied to files:
📚 Learning: 2025-07-17T15:42:26.275ZApplied to files:
📚 Learning: 2025-08-12T15:40:14.417ZApplied to files:
📚 Learning: 2025-07-17T15:42:04.167ZApplied to files:
📚 Learning: 2025-06-16T11:26:53.789ZApplied to files:
📚 Learning: 2025-07-04T00:17:47.799ZApplied to files:
📚 Learning: 2025-07-17T15:41:54.284ZApplied to files:
📚 Learning: 2025-07-17T15:43:04.876ZApplied to files:
📚 Learning: 2025-08-11T12:14:34.102ZApplied to files:
📚 Learning: 2025-08-08T15:58:03.524ZApplied to files:
📚 Learning: 2025-08-11T07:34:44.352ZApplied to files:
🧬 Code Graph Analysis (4)tests/model_registry/rest_api/test_multiple_mr.py (1)
tests/model_registry/rest_api/conftest.py (2)
tests/model_registry/utils.py (1)
tests/model_registry/conftest.py (3)
🔇 Additional comments (23)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/wip', '/cherry-pick', '/hold', '/build-push-pr-image', '/lgtm'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
tests/model_registry/upgrade/conftest.py (1)
60-66: Namespace deletion unconditional &ensure_existsmissing
Namespace(name=ns, ensure_exists=True)is not used; instead a newNamespace(name=ns)is created and then
deleted. If the namespace never existed the delete() call is still executed and will raise.Either reuse the earlier
namespaceobject or instantiate withensure_exists=Truebefore callingdelete().
🧹 Nitpick comments (12)
tests/model_registry/rest_api/constants.py (1)
58-58: Consider consolidating DB-backend identifiers
MARIADB_METADATA_DBduplicates the"db_name": "mariadb"literal already conveyed elsewhere viaPORT_MAP/ backend strings. If the constant is only used to pass a single-field payload, consider re-using an existing helper (or building one function) to avoid drift when new back-ends are added.tests/model_registry/rbac/multiple_instance_utils.py (1)
70-72: Hard-coded backend string
db_backend="mysql"appears in two separate calls (deployment template & mysql config).
To avoid accidental divergence when tests are parameterised for MariaDB, consider lifting"mysql"into a constant or a local variable.tests/model_registry/constants.py (2)
57-65: Minor style nit – multi-line string literal
MARIADB_MY_CNFembeds\nescapes. Triple-quoted strings improve readability and make editing easier:-MARIADB_MY_CNF = ( - "[mysqld]\nbind-address=0.0.0.0\ndefault_storage_engine=InnoDB\n" - "binlog_format=row\ninnodb_autoinc_lock_mode=2\ninnodb_buffer_pool_size=1024M" - "\nmax_allowed_packet=256M\n" -) +MARIADB_MY_CNF = """[mysqld] +bind-address=0.0.0.0 +default_storage_engine=InnoDB +binlog_format=row +innodb_autoinc_lock_mode=2 +innodb_buffer_pool_size=1024M +max_allowed_packet=256M +"""
62-65: PORT_MAP duplication
PORT_MAPmaps both backends to the same 3306 value. If no port difference is expected, one default constant (e.g.,DEFAULT_DB_PORT = 3306) would be simpler.tests/model_registry/upgrade/test_model_registry_upgrade.py (1)
52-53: Unused injected fixture in this test
model_registry_instanceisn’t used intest_retrieving_model_post_upgrade. Prefer applying it viausefixturesto avoid unused-parameter noise.- def test_retrieving_model_post_upgrade( - self: Self, - model_registry_client: list[ModelRegistryClient], - model_registry_instance, - ): + @pytest.mark.usefixtures("model_registry_instance") + def test_retrieving_model_post_upgrade( + self: Self, + model_registry_client: list[ModelRegistryClient], + ):If another fixture already guarantees MR availability for
model_registry_client, you can omitmodel_registry_instanceentirely.tests/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
23-28: Drop the redundant fixture parameter
updated_dsc_component_state_scope_sessionis already activated through
@pytest.mark.usefixtures, so keeping it in the method signature is superfluous and
adds an unused-variable warning.- updated_dsc_component_state_scope_session: DataScienceCluster,If you need the object inside the test, keep the param and remove it from
usefixtures; otherwise, delete the param for clarity.Also applies to: 31-38
tests/model_registry/rest_api/test_multiple_mr.py (1)
32-33: Remove unusedmodel_registry_instanceparameterThe fixture is already executed through
@pytest.mark.usefixtures. Since the test body
doesn’t reference the variable, drop it from the parameter list or prefix with_
to silence lint warnings.tests/model_registry/rbac/test_mr_rbac.py (1)
41-47: Duplicate fixture listed twice
updated_dsc_component_state_scope_sessionis included twice in the@usefixtures
decorator – redundant and noisy. Remove the second entry.tests/model_registry/rest_api/test_model_registry_rest_api.py (1)
88-108: Typed as list but treated like singleton
model_registry_instance[0]assumes at least one element, but the fixture
already guarantees ≥1. Consider meaningful naming (first_mr_instance) or
iterating explicitly; otherwise OK.tests/model_registry/conftest.py (1)
90-98: Nestedif pytestconfig.option.post_upgradeblock is duplicatedThe inner
ifrepeats the same condition and can never evaluate differently.
Remove the redundant check to simplify flow.tests/model_registry/utils.py (2)
485-508: Return type annotation is incorrect
get_mr_configmap_objectsreturns a list ofConfigMapobjects but is annotated aslist[Service], which will confuse static analysis and IDEs.- ) -> list[Service]: + ) -> list[ConfigMap]:
682-688: Function name no longer reflects its purpose
get_mysql_confignow handles both MySQL and MariaDB (db_backendparam, port map). Consider renaming to something backend-agnostic (get_db_config) to avoid cognitive dissonance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
tests/model_registry/conftest.py(5 hunks)tests/model_registry/constants.py(1 hunks)tests/model_registry/image_validation/conftest.py(1 hunks)tests/model_registry/image_validation/test_verify_rhoai_images.py(1 hunks)tests/model_registry/negative_tests/conftest.py(3 hunks)tests/model_registry/negative_tests/test_db_migration.py(1 hunks)tests/model_registry/negative_tests/test_model_registry_creation_negative.py(2 hunks)tests/model_registry/python_client/test_model_registry_creation.py(1 hunks)tests/model_registry/rbac/conftest.py(2 hunks)tests/model_registry/rbac/multiple_instance_utils.py(4 hunks)tests/model_registry/rbac/test_mr_rbac.py(4 hunks)tests/model_registry/rbac/test_mr_rbac_sa.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(1 hunks)tests/model_registry/rest_api/mariadb/conftest.py(0 hunks)tests/model_registry/rest_api/mariadb/test_mr_mariadb.py(0 hunks)tests/model_registry/rest_api/mariadb/utils.py(0 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(2 hunks)tests/model_registry/rest_api/test_multiple_mr.py(2 hunks)tests/model_registry/scc/test_model_registry_scc.py(1 hunks)tests/model_registry/upgrade/conftest.py(1 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(3 hunks)tests/model_registry/utils.py(12 hunks)
💤 Files with no reviewable changes (3)
- tests/model_registry/rest_api/mariadb/utils.py
- tests/model_registry/rest_api/mariadb/test_mr_mariadb.py
- tests/model_registry/rest_api/mariadb/conftest.py
🧰 Additional context used
🧠 Learnings (30)
📓 Common learnings
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: 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: 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: 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: 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: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_endpoint list access is not needed because upstream fixture validation already ensures endpoints exist before the tests execute. The Model Registry setup process validates endpoint availability, making additional bounds checks redundant.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_rest_endpoint fixture contains a built-in assertion `assert len(mr_instances) >= 1` that ensures at least one model registry instance exists before returning the endpoint list. This validation makes bounds checking redundant when accessing the first element of the returned list in test methods.
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#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.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/image_validation/conftest.pytests/model_registry/upgrade/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:23.880Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/rest_api/constants.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/constants.pytests/model_registry/image_validation/conftest.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/rest_api/constants.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/constants.pytests/model_registry/image_validation/conftest.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/rest_api/constants.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/constants.pytests/model_registry/image_validation/conftest.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-17T15:42:04.167Z
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.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/rest_api/constants.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/constants.pytests/model_registry/image_validation/conftest.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-08-08T15:59:16.170Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/test_async_upload_e2e.py:344-356
Timestamp: 2025-08-08T15:59:16.170Z
Learning: In opendatahub-io/opendatahub-tests, tests/model_registry/async_job/test_async_upload_e2e.py’s test_end_to_end_async_upload_job intentionally expects an OCI image index (application/vnd.oci.image.index.v1+json) from pull_manifest_from_oci_registry and asserts on the "manifests" array. This format has been verified by the author (lugi0); no change to Accept headers or assertions is required.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.py
📚 Learning: 2025-07-04T00:17:47.799Z
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.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/rest_api/constants.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/constants.pytests/model_registry/image_validation/conftest.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_endpoint list access is not needed because upstream fixture validation already ensures endpoints exist before the tests execute. The Model Registry setup process validates endpoint availability, making additional bounds checks redundant.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/image_validation/conftest.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-17T15:43:04.876Z
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.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/rest_api/constants.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/constants.pytests/model_registry/image_validation/conftest.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_rest_endpoint fixture contains a built-in assertion `assert len(mr_instances) >= 1` that ensures at least one model registry instance exists before returning the endpoint list. This validation makes bounds checking redundant when accessing the first element of the returned list in test methods.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/image_validation/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-06-18T08:27:21.114Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:257-264
Timestamp: 2025-06-18T08:27:21.114Z
Learning: In tests/model_registry/rest_api/conftest.py, the user fege prefers using next() without a default value to raise StopIteration when the mysql container is not found, rather than gracefully handling it with None checks. This is a deliberate "fail fast" design choice.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/rbac/test_mr_rbac.py
📚 Learning: 2025-07-09T08:50:22.172Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
Applied to files:
tests/model_registry/image_validation/test_verify_rhoai_images.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/image_validation/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-06-16T11:25:39.599Z
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.
Applied to files:
tests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-16T12:20:29.672Z
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.
Applied to files:
tests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/upgrade/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-17T15:41:54.284Z
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.
Applied to files:
tests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/rest_api/constants.pytests/model_registry/negative_tests/test_db_migration.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/python_client/test_model_registry_creation.pytests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-08-08T15:58:03.769Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/conftest.py:210-219
Timestamp: 2025-08-08T15:58:03.769Z
Learning: In tests/model_registry/async_job/conftest.py, using https in MODEL_SYNC_REGISTRY_SERVER_ADDRESS together with MODEL_SYNC_REGISTRY_IS_SECURE="False" is intentional: IS_SECURE=False disables TLS verification for the Model Registry’s self-signed certificate in test environments. This pattern aligns with the repository’s accepted practice of skipping TLS verification in tests.
Applied to files:
tests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/utils.py
📚 Learning: 2025-05-13T17:27:26.289Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#240
File: conftest.py:328-335
Timestamp: 2025-05-13T17:27:26.289Z
Learning: In the opendatahub-tests codebase, py_config["must_gather_collector"]["must_gather_base_directory"] is never empty as it's set during pytest_sessionstart() by combining get_base_dir() with BASE_DIRECTORY_NAME, making it safe to use in directory operations.
Applied to files:
tests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/conftest.py
📚 Learning: 2025-05-13T17:27:26.289Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#240
File: conftest.py:328-335
Timestamp: 2025-05-13T17:27:26.289Z
Learning: In the opendatahub-tests codebase, py_config["must_gather_collector"]["must_gather_base_directory"] is never empty as it's set during pytest_sessionstart() by combining get_base_dir() with BASE_DIRECTORY_NAME="must-gather-collected", making it safe to use in directory operations.
Applied to files:
tests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/conftest.py
📚 Learning: 2025-06-16T11:26:53.789Z
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.
Applied to files:
tests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_secure_db.pytests/model_registry/image_validation/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-06-18T06:42:37.705Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/utils.py:40-44
Timestamp: 2025-06-18T06:42:37.705Z
Learning: In tests/model_registry/rest_api/utils.py, the user prefers to keep the default value of verify=False in execute_model_registry_post_command to maintain backward compatibility, rather than changing it to True for security reasons.
Applied to files:
tests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/rest_api/test_model_registry_rest_api.py
📚 Learning: 2025-06-16T11:25:51.461Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:355-371
Timestamp: 2025-06-16T11:25:51.461Z
Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/test_mr_rbac.py
📚 Learning: 2025-05-05T13:48:02.409Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#283
File: tests/model_registry/test_user_permission.py:31-31
Timestamp: 2025-05-05T13:48:02.409Z
Learning: In the opendatahub-tests repository, bypassing TLS verification with `--insecure-skip-tls-verify=true` is an acceptable practice in test environments, particularly in the model registry tests.
Applied to files:
tests/model_registry/rest_api/test_model_registry_secure_db.py
📚 Learning: 2025-05-22T14:14:45.746Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#283
File: tests/model_registry/rbac/utils.py:31-32
Timestamp: 2025-05-22T14:14:45.746Z
Learning: In the OpenDataHub tests codebase, the ModelRegistryClient accepts the port parameter as a string, and explicit conversion to integer is not required.
Applied to files:
tests/model_registry/constants.py
📚 Learning: 2025-07-17T15:43:04.876Z
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, 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.
Applied to files:
tests/model_registry/rbac/test_mr_rbac.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-06-06T12:22:57.057Z
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.
Applied to files:
tests/model_registry/rbac/test_mr_rbac.py
📚 Learning: 2025-05-06T23:36:04.545Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:1-4
Timestamp: 2025-05-06T23:36:04.545Z
Learning: The opendatahub-tests repository is pinned to Python 3.12, which supports `typing.Self` natively, so there's no need for backward compatibility imports from typing_extensions.
Applied to files:
tests/model_registry/rest_api/test_multiple_mr.py
📚 Learning: 2025-06-16T23:01:39.623Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: utilities/user_utils.py:56-62
Timestamp: 2025-06-16T23:01:39.623Z
Learning: In the opendatahub-tests repository, temporary test credentials used in internal-only test environments don't require the same level of security precautions as production credentials. The team prefers using shlex.split() for command construction in these scenarios as they consider it adequate protection for their testing use case.
Applied to files:
tests/model_registry/rbac/conftest.py
📚 Learning: 2025-05-07T10:40:15.681Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0
Timestamp: 2025-05-07T10:40:15.681Z
Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.
Applied to files:
tests/model_registry/conftest.py
📚 Learning: 2025-06-17T21:36:46.461Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#359
File: tests/model_registry/rbac/conftest.py:214-229
Timestamp: 2025-06-17T21:36:46.461Z
Learning: In ocp_resources library, when reading from existing OAuth resources (like `OAuth(name="cluster")`), the client parameter is not required. The resource can access `.instance.spec` to read configuration from the live cluster without needing to pass a client parameter.
Applied to files:
tests/model_registry/utils.py
🧬 Code Graph Analysis (11)
tests/model_registry/rbac/multiple_instance_utils.py (1)
tests/model_registry/utils.py (2)
get_model_registry_deployment_template_dict(73-202)get_mysql_config(682-690)
tests/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
tests/model_registry/conftest.py (1)
updated_dsc_component_state_scope_session(157-206)
tests/model_registry/image_validation/conftest.py (2)
utilities/general.py (1)
wait_for_pods_by_labels(222-253)tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/upgrade/conftest.py (3)
tests/model_registry/utils.py (1)
wait_for_pods_running(245-278)utilities/constants.py (2)
DscComponents(156-177)ManagementState(162-164)tests/conftest.py (2)
dsc_resource(415-416)admin_client(66-67)
tests/model_registry/negative_tests/conftest.py (3)
tests/model_registry/utils.py (1)
get_model_registry_deployment_template_dict(73-202)utilities/general.py (1)
wait_for_pods_by_labels(222-253)tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/rbac/test_mr_rbac.py (1)
tests/model_registry/conftest.py (2)
updated_dsc_component_state_scope_session(157-206)model_registry_namespace(50-51)
tests/model_registry/rest_api/test_model_registry_rest_api.py (1)
tests/model_registry/conftest.py (1)
model_registry_instance(55-88)
tests/model_registry/rest_api/test_multiple_mr.py (1)
tests/model_registry/conftest.py (2)
model_registry_instance(55-88)model_registry_namespace(50-51)
tests/model_registry/rbac/conftest.py (1)
tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/conftest.py (3)
tests/model_registry/utils.py (3)
get_model_registry_metadata_resources(633-679)get_mr_service_by_label(39-63)get_endpoint_from_mr_service(66-70)tests/conftest.py (2)
admin_client(66-67)teardown_resources(87-94)utilities/constants.py (1)
Protocols(91-98)
tests/model_registry/utils.py (1)
tests/conftest.py (1)
teardown_resources(87-94)
🔇 Additional comments (17)
tests/model_registry/rbac/multiple_instance_utils.py (3)
2-3: Import-time access to py_config may raise early KeyErrorAccessing
py_config["model_registry_namespace"]at import time will explode during test collection if the key is missing/typoed.
If that risk is acceptable, fine; otherwise move the lookup inside a fixture or wrap withpy_config.get(...)+ explicit assertion.
19-20: 👍 Deterministic namespaceReplacing the random namespace with the configured value improves debuggability and matches the “predictable names expose cleanup failures” guideline.
85-86: Parameter alignment looks correct
get_mysql_config(..., db_backend="mysql")now matches the updated signature in utils – good catch.tests/model_registry/negative_tests/test_db_migration.py (1)
15-17: Fixture rename checkThe new fixture names assume
conftest.pyalready exports
updated_dsc_component_state_scope_session,model_registry_metadata_db_resources, andmodel_registry_instance.
Please verify older aliases are removed to prevent silent dupes.tests/model_registry/scc/test_model_registry_scc.py (1)
62-65: Consistent fixture setSwitching to the consolidated session-scoped fixtures aligns this test with the new infra—looks good.
tests/model_registry/image_validation/test_verify_rhoai_images.py (1)
15-18: Fixture consolidation LGTMSwitching to session-scoped DSC and consolidated DB/MR fixtures aligns with the refactor and should reduce setup overhead. No issues spotted.
tests/model_registry/rest_api/test_model_registry_secure_db.py (1)
40-40: Good: use consolidated DB resource fixtureReplacing
mysql_metadata_resourceswithmodel_registry_metadata_db_resourcesmatches the new fixture model and keeps resource management centralized.tests/model_registry/python_client/test_model_registry_creation.py (1)
36-38: Fixture consolidation LGTMUsing
model_registry_metadata_db_resourcesandmodel_registry_instanceis consistent with the refactor and reduces fixture sprawl.tests/model_registry/rbac/test_mr_rbac_sa.py (1)
13-16: Fixture updates LGTMMigrating to session-scoped DSC and consolidated MR/DB fixtures is aligned with the unified setup across tests. Works well with existing
model_registry_instance_rest_endpointusage.tests/model_registry/upgrade/test_model_registry_upgrade.py (3)
31-31: Pre-upgrade fixture setup LGTMUsing the consolidated DB resources and MR instance in pre-upgrade tests matches the new pattern and keeps setup minimal.
67-68: API version check LGTMAccessing
apiVersionvia the first instance matches the fixture contract and expected CRD version post-upgrade.
74-77: Spec assertions LGTMValidating
istiodisabled and OAuth proxy route state aligns with post-upgrade expectations.tests/model_registry/image_validation/conftest.py (1)
12-20: Verify label selector robustness
label_selector=f"app={MR_INSTANCE_NAME}"assumes the MR pod always carries plain
applabel. In several other fixtures we match using
app.kubernetes.io/nameorcomponent.
Please confirm the label is stable across backends, or switch to the canonical label
to avoid flaky “pod not found” errors.tests/model_registry/rbac/conftest.py (1)
517-520: LGTM – handy utility fixtureSession-scoped
api_server_urlcleanly centralises the cluster URL retrieval.tests/model_registry/rest_api/conftest.py (1)
123-131: Based on the fixture definitions,model_registry_metadata_db_resourcesalways includes a non‐emptySecretlist for any supported backend (and defaultnum_resources=1), so the direct index is safe. The proposed guard against a missingSecretkey is therefore unnecessary. Use the code as‐is.Likely an incorrect or invalid review comment.
tests/model_registry/conftest.py (1)
49-52: Session-scoped namespace fixture leaks when DSC is cleaned upBecause
model_registry_namespaceis session-scoped, later tests running
after the DSC component is set back to Removed will still reference the
now-deleted namespace. Consider reverting to class scope or refreshing the
value after teardown.tests/model_registry/utils.py (1)
73-202: Verify MariaDB probe commands and redundant settingsThe MariaDB branch still includes MySQL-specific probes and duplicate env vars—please manually confirm and adjust as needed:
- Confirm the RHEL8 MariaDB image actually provides
mysqladmin(if not, switch tomariadb-adminor another supported tool).- Verify the default data directory in the MariaDB image matches the PVC mount (since we removed the
--datadiroverride).- Remove the extra
MYSQL_ROOT_PASSWORDifMARIADB_ROOT_PASSWORDalone suffices.
Please check directly in a MariaDB container (e.g. with
podmanordocker exec) that the probe commands and environment variables work as expected in this image.
tests/model_registry/python_client/test_model_registry_creation.py
Outdated
Show resolved
Hide resolved
dbasunag
force-pushed
the
mariadb-tests
branch
from
c981f80 to
b49f5cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/model_registry/conftest.py (1)
100-100: Duplicate issue: Same parameter key mismatch here.Same problem as in the
model_registry_instancefixture - usingdb_nameinstead ofdb_backend.Apply this fix:
- db_backend = getattr(request, "param", {}).get("db_name", "mysql") + db_backend = getattr(request, "param", {}).get("db_backend", "mysql")tests/model_registry/rest_api/test_model_registry_rest_api.py (1)
35-36: Critical: Parameter key mismatch prevents MariaDB testing.The test parameters use
"db_name": "mariadb"but the fixtures expect"db_backend". This causes the MariaDB test case to silently fall back to MySQL, defeating the purpose of parameterization.Fix the parameter keys:
pytest.param( - {"db_name": "mariadb"}, - {"db_name": "mariadb"}, + {"db_backend": "mariadb"}, + {"db_backend": "mariadb"}, MODEL_REGISTER_DATA, ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
tests/model_registry/conftest.py(5 hunks)tests/model_registry/constants.py(1 hunks)tests/model_registry/image_validation/conftest.py(1 hunks)tests/model_registry/image_validation/test_verify_rhoai_images.py(1 hunks)tests/model_registry/negative_tests/conftest.py(3 hunks)tests/model_registry/negative_tests/test_db_migration.py(1 hunks)tests/model_registry/negative_tests/test_model_registry_creation_negative.py(2 hunks)tests/model_registry/python_client/test_model_registry_creation.py(1 hunks)tests/model_registry/rbac/conftest.py(2 hunks)tests/model_registry/rbac/multiple_instance_utils.py(4 hunks)tests/model_registry/rbac/test_mr_rbac.py(4 hunks)tests/model_registry/rbac/test_mr_rbac_sa.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(1 hunks)tests/model_registry/rest_api/mariadb/conftest.py(0 hunks)tests/model_registry/rest_api/mariadb/test_mr_mariadb.py(0 hunks)tests/model_registry/rest_api/mariadb/utils.py(0 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(2 hunks)tests/model_registry/rest_api/test_multiple_mr.py(2 hunks)tests/model_registry/scc/test_model_registry_scc.py(1 hunks)tests/model_registry/upgrade/conftest.py(1 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(3 hunks)tests/model_registry/utils.py(12 hunks)
💤 Files with no reviewable changes (3)
- tests/model_registry/rest_api/mariadb/utils.py
- tests/model_registry/rest_api/mariadb/conftest.py
- tests/model_registry/rest_api/mariadb/test_mr_mariadb.py
🚧 Files skipped from review as they are similar to previous changes (16)
- tests/model_registry/negative_tests/test_model_registry_creation_negative.py
- tests/model_registry/negative_tests/test_db_migration.py
- tests/model_registry/python_client/test_model_registry_creation.py
- tests/model_registry/rest_api/constants.py
- tests/model_registry/rest_api/test_model_registry_secure_db.py
- tests/model_registry/rbac/multiple_instance_utils.py
- tests/model_registry/image_validation/test_verify_rhoai_images.py
- tests/model_registry/utils.py
- tests/model_registry/image_validation/conftest.py
- tests/model_registry/scc/test_model_registry_scc.py
- tests/model_registry/negative_tests/conftest.py
- tests/model_registry/upgrade/conftest.py
- tests/model_registry/constants.py
- tests/model_registry/rest_api/test_multiple_mr.py
- tests/model_registry/rbac/test_mr_rbac.py
- tests/model_registry/rest_api/conftest.py
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/rest_api/test_model_registry_secure_db.py:16-16
Timestamp: 2025-08-11T12:14:12.803Z
Learning: In tests/model_registry/rest_api/test_model_registry_secure_db.py, both the class-level `model_registry_instance` fixture and the method-level `deploy_secure_mysql_and_mr` fixture are intentionally required together for the secure database test setup. These fixtures serve different purposes and are not redundant.
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: 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: 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: 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.
📚 Learning: 2025-07-17T15:42:23.880Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:12.803Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/rest_api/test_model_registry_secure_db.py:16-16
Timestamp: 2025-08-11T12:14:12.803Z
Learning: In tests/model_registry/rest_api/test_model_registry_secure_db.py, both the class-level `model_registry_instance` fixture and the method-level `deploy_secure_mysql_and_mr` fixture are intentionally required together for the secure database test setup. These fixtures serve different purposes and are not redundant.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_endpoint list access is not needed because upstream fixture validation already ensures endpoints exist before the tests execute. The Model Registry setup process validates endpoint availability, making additional bounds checks redundant.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:04.167Z
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.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-04T00:17:47.799Z
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.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:43:04.876Z
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.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:41:54.284Z
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.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/conftest.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-06-16T11:25:39.599Z
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.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rbac/conftest.py
📚 Learning: 2025-06-16T11:26:53.789Z
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.
Applied to files:
tests/model_registry/rbac/test_mr_rbac_sa.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/rbac/conftest.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_rest_endpoint fixture contains a built-in assertion `assert len(mr_instances) >= 1` that ensures at least one model registry instance exists before returning the endpoint list. This validation makes bounds checking redundant when accessing the first element of the returned list in test methods.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-16T12:20:29.672Z
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.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.py
🧬 Code Graph Analysis (4)
tests/model_registry/rbac/conftest.py (1)
tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/rest_api/test_model_registry_rest_api.py (1)
tests/model_registry/conftest.py (1)
model_registry_instance(55-88)
tests/model_registry/upgrade/test_model_registry_upgrade.py (1)
tests/model_registry/conftest.py (1)
model_registry_instance(55-88)
tests/model_registry/conftest.py (2)
tests/model_registry/utils.py (3)
get_model_registry_metadata_resources(635-681)get_mr_service_by_label(41-65)get_endpoint_from_mr_service(68-72)utilities/constants.py (1)
Protocols(91-98)
🔇 Additional comments (6)
tests/model_registry/rbac/conftest.py (1)
13-13: LGTM! Clean Infrastructure import and API server URL fixture.The new
api_server_urlfixture correctly uses the Infrastructure resource to retrieve the cluster API server URL, which is a standard approach for accessing cluster information in OpenShift tests.Also applies to: 517-520
tests/model_registry/upgrade/test_model_registry_upgrade.py (1)
31-31: LGTM! Fixture consolidation properly implemented.The updates correctly migrate from the old MySQL-specific fixtures (
model_registry_instance_mysql) to the new consolidated fixtures (model_registry_instance,model_registry_metadata_db_resources). The access patternmodel_registry_instance[0]is consistent with the new fixture returning a list of instances.Also applies to: 52-52, 64-64, 67-67, 74-74, 76-76
tests/model_registry/conftest.py (2)
92-139: Excellent fixture consolidation approach.The
model_registry_metadata_db_resourcesfixture properly consolidates all database resources into a unified structure, supporting both MySQL and MariaDB backends. The handling of both normal and post-upgrade scenarios is well-designed.
49-51: Session scope for namespace makes sense.Moving the
model_registry_namespacefixture to session scope improves efficiency by reusing the namespace across multiple tests, which aligns with the session-scoped DSC management.tests/model_registry/rbac/test_mr_rbac_sa.py (1)
13-16: LGTM! Clean fixture migration for RBAC tests.The migration to consolidated fixtures (
model_registry_metadata_db_resources,model_registry_instance) is correctly implemented, maintaining test stability while benefiting from the unified resource management approach.tests/model_registry/rest_api/test_model_registry_rest_api.py (1)
43-46: LGTM! Test structure properly updated for fixture consolidation.The migration to consolidated fixtures and updated test method signatures is correctly implemented. The access pattern
model_registry_instance[0]properly handles the list-based fixture return.Also applies to: 90-94, 103-105
dbasunag
force-pushed
the
mariadb-tests
branch
from
b49f5cc to
039aded
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/model_registry/rest_api/test_model_registry_rest_api.py (1)
26-37: Fix parameter key mismatch to enable MariaDB testing.The fixtures expect
"db_backend"as the key, but the test passes{"db_name": "mariadb"}. This causes the fixtures to default to MySQL instead of testing MariaDB.Apply this diff to fix the parameter keys:
- pytest.param( - {"db_name": "mariadb"}, - {"db_name": "mariadb"}, - MODEL_REGISTER_DATA, - ), + pytest.param( + {"db_backend": "mariadb"}, + {"db_backend": "mariadb"}, + MODEL_REGISTER_DATA, + ),tests/model_registry/conftest.py (1)
111-112: MariaDB ConfigMap gating condition fixedThe condition now correctly checks db_backend == "mariadb" (was previously comparing to a list). This resolves the always-false bug and ensures ConfigMaps are handled for MariaDB runs.
🧹 Nitpick comments (4)
tests/model_registry/conftest.py (4)
55-89: Minor perf/logging tweaks in model_registry_instance
- You already ensure DB resources via the dependency — good. Also, using param.get("db_name", "mysql") is correct per team convention. Noted from retrieved learnings that tests pass db_name.
- You call wait_for_pods_running once per instance; this is redundant and increases test time when num_resources > 1. Call it once after all instances are Available/OAuthProxyAvailable.
- The log line provides no detail. Consider logging the param dict.
Apply this diff to move the pod wait outside the loop and improve the log:
- LOGGER.warning("Requested Oauth Proxy configuration:") + LOGGER.info(f"Requested Oauth Proxy configuration: {param}") @@ - for mr_instance in mr_instances: - mr_instance.wait_for_condition(condition="Available", status="True") - mr_instance.wait_for_condition(condition="OAuthProxyAvailable", status="True") - wait_for_pods_running( - admin_client=admin_client, namespace_name=model_registry_namespace, number_of_consecutive_checks=6 - ) + for mr_instance in mr_instances: + mr_instance.wait_for_condition(condition="Available", status="True") + mr_instance.wait_for_condition(condition="OAuthProxyAvailable", status="True") + wait_for_pods_running( + admin_client=admin_client, namespace_name=model_registry_namespace, number_of_consecutive_checks=6 + )
102-119: Remove redundant nested post-upgrade checkThere is a duplicated nested if pytestconfig.option.post_upgrade. It’s dead duplication and hurts readability.
Apply this diff to remove the redundant nested condition:
- if pytestconfig.option.post_upgrade: - if pytestconfig.option.post_upgrade: - resources = { + if pytestconfig.option.post_upgrade: + resources = { Secret: [Secret(name=DB_RESOURCE_NAME, namespace=model_registry_namespace, ensure_exists=True)], PersistentVolumeClaim: [ PersistentVolumeClaim(name=DB_RESOURCE_NAME, namespace=model_registry_namespace, ensure_exists=True) ], Service: [Service(name=DB_RESOURCE_NAME, namespace=model_registry_namespace, ensure_exists=True)], ConfigMap: [ConfigMap(name=DB_RESOURCE_NAME, namespace=model_registry_namespace, ensure_exists=True)] if db_backend == "mariadb" else [], Deployment: [Deployment(name=DB_RESOURCE_NAME, namespace=model_registry_namespace, ensure_exists=True)], - } - yield resources - for kind in [Deployment, ConfigMap, Service, PersistentVolumeClaim, Secret]: - for resource in resources[kind]: - resource.delete(wait=True) + } + yield resources + for kind in [Deployment, ConfigMap, Service, PersistentVolumeClaim, Secret]: + for resource in resources[kind]: + resource.delete(wait=True)
136-138: Guard against KeyError when waiting on DeploymentsIf, for any reason, no Deployment objects were created (edge-case in alternative setups), resources_instances[Deployment] would raise KeyError. Use .get(...) to be defensive.
Apply this diff:
- for deployment in resources_instances[Deployment]: + for deployment in resources_instances.get(Deployment, []): deployment.wait_for_replicas(deployed=True)
141-154: Unreachable empty-list guard due to exception flowget_mr_service_by_label raises on absence, so the list comprehension will raise before mr_services is assigned. The subsequent if not mr_services check is unreachable. Either remove the check or catch exceptions explicitly.
Simplest: remove the unreachable guard and let the original exception bubble:
- if not mr_services: - raise ResourceNotFoundError("No model registry services found") return [get_endpoint_from_mr_service(svc=svc, protocol=Protocols.REST) for svc in mr_services]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
tests/model_registry/conftest.py(5 hunks)tests/model_registry/constants.py(1 hunks)tests/model_registry/image_validation/conftest.py(1 hunks)tests/model_registry/image_validation/test_verify_rhoai_images.py(2 hunks)tests/model_registry/negative_tests/conftest.py(3 hunks)tests/model_registry/negative_tests/test_db_migration.py(1 hunks)tests/model_registry/negative_tests/test_model_registry_creation_negative.py(2 hunks)tests/model_registry/python_client/test_model_registry_creation.py(1 hunks)tests/model_registry/rbac/conftest.py(2 hunks)tests/model_registry/rbac/multiple_instance_utils.py(4 hunks)tests/model_registry/rbac/test_mr_rbac.py(4 hunks)tests/model_registry/rbac/test_mr_rbac_sa.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(1 hunks)tests/model_registry/rest_api/mariadb/conftest.py(0 hunks)tests/model_registry/rest_api/mariadb/test_mr_mariadb.py(0 hunks)tests/model_registry/rest_api/mariadb/utils.py(0 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(2 hunks)tests/model_registry/rest_api/test_multiple_mr.py(2 hunks)tests/model_registry/scc/test_model_registry_scc.py(1 hunks)tests/model_registry/upgrade/conftest.py(1 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(3 hunks)tests/model_registry/utils.py(12 hunks)
💤 Files with no reviewable changes (3)
- tests/model_registry/rest_api/mariadb/test_mr_mariadb.py
- tests/model_registry/rest_api/mariadb/conftest.py
- tests/model_registry/rest_api/mariadb/utils.py
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/model_registry/scc/test_model_registry_scc.py
- tests/model_registry/constants.py
- tests/model_registry/rbac/multiple_instance_utils.py
- tests/model_registry/python_client/test_model_registry_creation.py
- tests/model_registry/rest_api/test_model_registry_secure_db.py
- tests/model_registry/negative_tests/test_db_migration.py
- tests/model_registry/image_validation/conftest.py
- tests/model_registry/rest_api/conftest.py
- tests/model_registry/negative_tests/conftest.py
- tests/model_registry/upgrade/test_model_registry_upgrade.py
- tests/model_registry/negative_tests/test_model_registry_creation_negative.py
- tests/model_registry/rbac/test_mr_rbac_sa.py
- tests/model_registry/rbac/conftest.py
- tests/model_registry/image_validation/test_verify_rhoai_images.py
- tests/model_registry/rest_api/constants.py
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/upgrade/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:23.880Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/upgrade/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-17T15:42:04.167Z
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.
Applied to files:
tests/model_registry/upgrade/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:12.803Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/rest_api/test_model_registry_secure_db.py:16-16
Timestamp: 2025-08-11T12:14:12.803Z
Learning: In tests/model_registry/rest_api/test_model_registry_secure_db.py, both the class-level `model_registry_instance` fixture and the method-level `deploy_secure_mysql_and_mr` fixture are intentionally required together for the secure database test setup. These fixtures serve different purposes and are not redundant.
Applied to files:
tests/model_registry/upgrade/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:43:04.876Z
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.
Applied to files:
tests/model_registry/upgrade/conftest.pytests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/conftest.py
📚 Learning: 2025-07-04T00:17:47.799Z
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.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-08-12T15:40:14.381Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/conftest.py:78-78
Timestamp: 2025-08-12T15:40:14.381Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance fixture correctly uses param.get("db_name", "mysql") to read test parameters, as tests parameterize using "db_name" as the key (e.g., {"db_name": "mariadb"}). The parameter dict key does not need to match the function parameter name when passing values between fixtures.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/conftest.pytests/model_registry/utils.py
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_endpoint list access is not needed because upstream fixture validation already ensures endpoints exist before the tests execute. The Model Registry setup process validates endpoint availability, making additional bounds checks redundant.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/conftest.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:41:54.284Z
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.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/conftest.py
📚 Learning: 2025-07-16T12:20:29.672Z
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.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.py
📚 Learning: 2025-06-16T11:26:53.789Z
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.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_rest_endpoint fixture contains a built-in assertion `assert len(mr_instances) >= 1` that ensures at least one model registry instance exists before returning the endpoint list. This validation makes bounds checking redundant when accessing the first element of the returned list in test methods.
Applied to files:
tests/model_registry/rest_api/test_model_registry_rest_api.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/rbac/test_mr_rbac.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T07:34:44.352Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/utils.py:34-36
Timestamp: 2025-08-11T07:34:44.352Z
Learning: In the opendatahub-tests repository, container images should use digest references (e.g., `imagesha256:...`) instead of version tags like `:latest`, especially for disconnected environments where specific digests are required for image mirroring. This applies to test fixtures and deployment configurations.
Applied to files:
tests/model_registry/utils.py
🧬 Code Graph Analysis (6)
tests/model_registry/upgrade/conftest.py (3)
tests/model_registry/utils.py (1)
wait_for_pods_running(247-280)utilities/constants.py (2)
DscComponents(156-177)ManagementState(162-164)tests/conftest.py (2)
dsc_resource(415-416)admin_client(66-67)
tests/model_registry/rest_api/test_model_registry_rest_api.py (1)
tests/model_registry/conftest.py (1)
model_registry_instance(55-88)
tests/model_registry/rest_api/test_multiple_mr.py (1)
tests/model_registry/conftest.py (2)
model_registry_instance(55-88)model_registry_namespace(50-51)
tests/model_registry/rbac/test_mr_rbac.py (1)
tests/model_registry/conftest.py (2)
updated_dsc_component_state_scope_session(157-206)model_registry_namespace(50-51)
tests/model_registry/conftest.py (2)
tests/model_registry/utils.py (3)
get_model_registry_metadata_resources(635-681)get_mr_service_by_label(41-65)get_endpoint_from_mr_service(68-72)utilities/constants.py (1)
Protocols(91-98)
tests/model_registry/utils.py (1)
tests/conftest.py (1)
teardown_resources(87-94)
🔇 Additional comments (22)
tests/model_registry/rest_api/test_multiple_mr.py (3)
16-25: LGTM! Parameterization updated to use consolidated fixtures.The test now correctly uses the new consolidated
model_registry_metadata_db_resourcesandmodel_registry_instancefixtures instead of the individual resource fixtures. This aligns with the PR's goal of consolidating DB resource management.
26-30: LGTM! Fixtures updated to session-scoped state.The fixture update from class-scoped to session-scoped DSC state (
updated_dsc_component_state_scope_session) and the addition of consolidated resource fixtures align with the broader refactoring pattern in this PR.
32-32: LGTM! Test signature updated for new fixture.The test method signature correctly removes the typed
model_registry_instance_mysqlparameter and uses the genericmodel_registry_instancefixture, maintaining the same access pattern.tests/model_registry/upgrade/conftest.py (2)
14-39: LGTM! Pre-upgrade fixture correctly manages DSC state.The fixture properly:
- Captures original components state
- Validates pre-condition (fails if MR already MANAGED)
- Applies patch to enable Model Registry
- Waits for conditions and namespace readiness
- Returns the patched DSC resource
Based on retrieved learnings, ResourceEditor correctly handles partial component patches without overwriting other DSC components.
42-65: LGTM! Post-upgrade teardown fixture correctly implemented.The fixture follows the proper pytest teardown pattern:
- Yields immediately for test execution
- Performs cleanup in teardown phase
- Validates post-condition (fails if MR already REMOVED)
- Patches MR back to REMOVED state
- Properly deletes the namespace
The fixture correctly handles the upgrade test lifecycle.
tests/model_registry/rest_api/test_model_registry_rest_api.py (3)
42-47: LGTM! Fixtures updated to consolidated pattern.The fixture changes correctly align with the session-scoped DSC state and consolidated DB resource management pattern used throughout this PR.
88-99: LGTM! Test method updated for new fixture pattern.The method correctly uses the new
model_registry_instancefixture and accesses attributes viamodel_registry_instance[0].nameandmodel_registry_instance[0].namespace. The core validation logic remains unchanged.
101-108: LGTM! OAuth proxy validation updated correctly.The test properly accesses the instance spec via
model_registry_instance[0].instance.specand maintains the same OAuth proxy validation assertions.tests/model_registry/utils.py (7)
75-77: LGTM! Function signature updated for backend parameterization.The addition of the
db_backendparameter enables MariaDB support while maintaining backward compatibility through the function implementation.
178-204: LGTM! MariaDB-specific configuration applied correctly.The MariaDB backend configuration properly:
- Sets appropriate labels for app/component identification
- Uses the MariaDB image digest (avoiding
:latesttag per learning)- Configures MariaDB-specific environment variables
- Sets up proper volume mounts for data and config
- Removes MySQL-specific args
- Uses ConfigMap for MariaDB configuration
The implementation follows best practices for containerized database deployment.
487-509: LGTM! ConfigMap creation for MariaDB backend.The function correctly creates ConfigMap resources for MariaDB configuration using the
MARIADB_MY_CNFconstant. The backend parameter allows conditional creation based on database type.
552-589: LGTM! Deployment creation updated for backend awareness.The function properly:
- Adds backend-specific selector labels for MariaDB
- Passes the
db_backendparameter to template generation- Maintains consistent naming and resource management patterns
This enables the same deployment function to handle both MySQL and MariaDB backends.
601-632: LGTM! Model Registry object creation with backend support.The function correctly:
- Passes
db_backendtoget_mysql_configfor backend-specific configuration- Maintains the same Model Registry configuration pattern
- Properly handles SSL certificate configuration when provided
This enables MariaDB-backed Model Registry instances.
635-682: LGTM! Consolidated metadata resource aggregation.The function provides a clean interface for creating all required DB resources:
- Returns a dictionary mapping resource types to lists of resources
- Passes
db_backendto ConfigMap and Deployment creation functions- Maintains consistent parameter patterns across all resource types
This supports the PR's goal of consolidating DB resource management.
684-692: LGTM! MySQL config updated for backend port mapping.The function correctly uses
PORT_MAP[db_backend]to provide the appropriate port for different database backends (MySQL vs MariaDB). This maintains the same configuration structure while supporting multiple backends.tests/model_registry/rbac/test_mr_rbac.py (3)
25-25: LGTM! Import path updated for relocated module.The import correctly references the new location of
MR_MULTIPROJECT_TEST_SCENARIO_PARAMSin the RBAC-specific module.
137-148: LGTM! Parameterization updated for session-scoped fixture.The parameter name correctly changes from
updated_dsc_component_state_parametrizedtoupdated_dsc_component_state_scope_sessionto match the new session-scoped fixture pattern.
149-155: LGTM! Test signature updated with explicit namespace parameter.The test method signature correctly:
- Updates the DSC parameter name to match the new fixture
- Adds explicit
model_registry_namespaceparameter- Removes the inline namespace derivation logic
This aligns with the PR's pattern of providing namespace via dedicated fixture rather than deriving it within tests.
tests/model_registry/conftest.py (4)
8-8: Correct import of ConfigMapSwitching to ocp_resources.config_map.ConfigMap is correct and aligns with current ocp-resources structure.
39-40: Good consolidation: use get_model_registry_metadata_resourcesImporting get_model_registry_metadata_resources to centralize DB resource creation is the right direction for reducing duplication and aligning with backend-aware provisioning.
49-51: Session-scoped model_registry_namespace is appropriateDeriving the namespace from updated_dsc_component_state_scope_session and promoting to session scope aligns with the PR objective and reduces setup churn across classes.
156-162: No stale references found forupdated_dsc_component_state_scope_class
Verified via ripgrep search: there are no remaining usages of the old fixture name in the repository. Safe to proceed with the session‐scoped fixture change.
dbasunag
force-pushed
the
mariadb-tests
branch
from
039aded to
aaeb7ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
tests/model_registry/scc/test_model_registry_scc.py (1)
44-46: Fix: error message isn’t formatted (missing f-string)The failure message is a plain string; the expression inside braces won’t be evaluated. Convert to an f-string.
- pytest.fail( - "Expected one model registry pod. Found: {[{pod.name: pod.status} for pod in pods] if pods else None}" - ) + pytest.fail( + f"Expected one model registry pod. Found: {[{pod.name: pod.status} for pod in pods] if pods else None}" + )tests/model_registry/rbac/conftest.py (1)
196-202: Type hint and docstring mismatch: fixture yields Secret, not UserTestSessionThe created_htpasswd_secret fixture returns a Secret but is annotated (and documented) as yielding UserTestSession. Fix the type and docstring.
-@pytest.fixture(scope="module") -def created_htpasswd_secret( - original_user: str, user_credentials_rbac: dict[str, str] -) -> Generator[UserTestSession, None, None]: - """ - Session-scoped fixture that creates a test IDP user and cleans it up after all tests. - Returns a UserTestSession object that contains all necessary credentials and contexts. - """ +@pytest.fixture(scope="module") +def created_htpasswd_secret( + original_user: str, user_credentials_rbac: dict[str, str] +) -> Generator[Secret, None, None]: + """ + Module-scoped fixture that creates an HTPasswd Secret used by the OAuth IDP. + Returns the created Secret. + """tests/model_registry/utils.py (1)
552-590: Bug: deployment template references secret with a hard-coded baseUse the function’s base_name for the secret to align with how secrets are created in get_mr_secret_objects.
- secret_name = f"{DB_BASE_RESOURCES_NAME}{num_deployment}" + secret_name = f"{base_name}{num_deployment}"
♻️ Duplicate comments (1)
tests/model_registry/negative_tests/conftest.py (1)
139-166: Avoid printing DB password in process list; simplify parsing of query output
- Security: Passing the password via -p leaks secrets in the process list and logs. Prefer MYSQL_PWD env (or a shell wrapper) to avoid exposure. Note: This was raised previously and deferred; still valid.
- Robustness: Use -N -B -s to suppress headers/formatting and parse safely.
- output = model_registry_db_instance_pod.execute( - command=[ - "mysql", - "-u", - MODEL_REGISTRY_DB_SECRET_STR_DATA["database-user"], - f"-p{MODEL_REGISTRY_DB_SECRET_STR_DATA['database-password']}", - "-e", - "SELECT version FROM schema_migrations ORDER BY version DESC LIMIT 1;", - MODEL_REGISTRY_DB_SECRET_STR_DATA["database-name"], - ] - ) - latest_migration_version = int(output.strip().split()[1]) + output = model_registry_db_instance_pod.execute( + command=[ + "bash", + "-lc", + ( + f"MYSQL_PWD='{MODEL_REGISTRY_DB_SECRET_STR_DATA['database-password']}' " + f"mysql -N -B -s -u {MODEL_REGISTRY_DB_SECRET_STR_DATA['database-user']} " + f"{MODEL_REGISTRY_DB_SECRET_STR_DATA['database-name']} " + "-e \"SELECT version FROM schema_migrations ORDER BY version DESC LIMIT 1;\"" + ), + ] + ) + try: + latest_migration_version = int(output.strip().splitlines()[-1].strip()) + except (IndexError, ValueError) as exc: + raise RuntimeError(f"Unexpected mysql output while reading schema_migrations version: {output!r}") from exc @@ - model_registry_db_instance_pod.execute( - command=[ - "mysql", - "-u", - MODEL_REGISTRY_DB_SECRET_STR_DATA["database-user"], - f"-p{MODEL_REGISTRY_DB_SECRET_STR_DATA['database-password']}", - "-e", - f"UPDATE schema_migrations SET dirty = 1 WHERE version = {latest_migration_version};", - MODEL_REGISTRY_DB_SECRET_STR_DATA["database-name"], - ] - ) + model_registry_db_instance_pod.execute( + command=[ + "bash", + "-lc", + ( + f"MYSQL_PWD='{MODEL_REGISTRY_DB_SECRET_STR_DATA['database-password']}' " + f"mysql -u {MODEL_REGISTRY_DB_SECRET_STR_DATA['database-user']} " + f"{MODEL_REGISTRY_DB_SECRET_STR_DATA['database-name']} " + f"-e \"UPDATE schema_migrations SET dirty = 1 WHERE version = {latest_migration_version};\"" + ), + ] + )
🧹 Nitpick comments (9)
tests/model_registry/scc/test_model_registry_scc.py (1)
61-66: Possible redundancy: avoid listing both MR instance and its underlying DB resource fixtureIf model_registry_instance already depends on model_registry_metadata_db_resources (as per the refactor), listing both can be redundant and confusing. Consider relying only on model_registry_instance here.
tests/model_registry/rbac/conftest.py (3)
125-131: Docstring param name mismatchDocstring references test_idp_user_session but the parameter is test_idp_user. Update for accuracy.
- test_idp_user_session: The test user session containing user information + test_idp_user: The test user session containing user information
150-156: Docstring param name mismatch (second occurrence)Same mismatch here; align the name with the function parameter.
- test_idp_user_session: The test user session containing user information + test_idp_user: The test user session containing user information
517-523: Harden api_server_url fixture: handle missing status field explicitlyThe Infrastructure status should have apiServerURL, but if it doesn’t, raise a clear error.
@pytest.fixture(scope="session") def api_server_url(admin_client: DynamicClient) -> str: """ Get api server url from the cluster. """ - infrastructure = Infrastructure(client=admin_client, name="cluster", ensure_exists=True) - return infrastructure.instance.status.apiServerURL + infrastructure = Infrastructure(client=admin_client, name="cluster", ensure_exists=True) + api_url = getattr(infrastructure.instance.status, "apiServerURL", None) + if not api_url: + raise RuntimeError("Infrastructure.status.apiServerURL is missing; cannot determine cluster API URL") + return api_urltests/model_registry/negative_tests/conftest.py (1)
180-185: Minor: clarify fixture intentdelete_mr_deployment executes immediately when requested and doesn’t yield. Consider a short note in the docstring to highlight it’s an action-only fixture (no teardown).
tests/model_registry/utils.py (1)
635-681: Optional: rely on get_mr_configmap_objects to skip for MySQLGiven the change above, no extra condition is needed here; get_mr_configmap_objects will return [] for MySQL. If you prefer, you can also short-circuit here to reduce calls. No change strictly required.
tests/model_registry/conftest.py (3)
55-62: Unify type hints and narrow return type for clarityMinor consistency and typing polish:
- Use FixtureRequest consistently (already imported) instead of mixing pytest.FixtureRequest and FixtureRequest.
- Narrow the return type to the concrete ModelRegistry list.
Apply this diff:
def model_registry_instance( - request: pytest.FixtureRequest, + request: FixtureRequest, pytestconfig: Config, admin_client: DynamicClient, teardown_resources: bool, model_registry_metadata_db_resources: dict[Any, Any], model_registry_namespace: str, -) -> Generator[list[Any], Any, Any]: +) -> Generator[list[ModelRegistry], None, None]:
102-104: Remove redundant nested post-upgrade checkThe inner
if pytestconfig.option.post_upgradeduplicates the outer condition and can be removed.Apply this diff:
- if pytestconfig.option.post_upgrade: - if pytestconfig.option.post_upgrade: - resources = { + if pytestconfig.option.post_upgrade: + resources = {
141-150: Optional: add type annotation for model_registry_instance parameterClarify expected type for better readability and editor support.
Apply this diff:
-def model_registry_instance_rest_endpoint(admin_client: DynamicClient, model_registry_instance) -> list[str]: +def model_registry_instance_rest_endpoint( + admin_client: DynamicClient, model_registry_instance: list[ModelRegistry] +) -> list[str]:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
tests/model_registry/conftest.py(5 hunks)tests/model_registry/constants.py(1 hunks)tests/model_registry/image_validation/conftest.py(1 hunks)tests/model_registry/image_validation/test_verify_rhoai_images.py(2 hunks)tests/model_registry/negative_tests/conftest.py(3 hunks)tests/model_registry/negative_tests/test_db_migration.py(1 hunks)tests/model_registry/negative_tests/test_model_registry_creation_negative.py(2 hunks)tests/model_registry/python_client/test_model_registry_creation.py(1 hunks)tests/model_registry/rbac/conftest.py(2 hunks)tests/model_registry/rbac/multiple_instance_utils.py(4 hunks)tests/model_registry/rbac/test_mr_rbac.py(4 hunks)tests/model_registry/rbac/test_mr_rbac_sa.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(1 hunks)tests/model_registry/rest_api/mariadb/conftest.py(0 hunks)tests/model_registry/rest_api/mariadb/test_mr_mariadb.py(0 hunks)tests/model_registry/rest_api/mariadb/utils.py(0 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(2 hunks)tests/model_registry/rest_api/test_multiple_mr.py(2 hunks)tests/model_registry/scc/test_model_registry_scc.py(1 hunks)tests/model_registry/upgrade/conftest.py(1 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(3 hunks)tests/model_registry/utils.py(12 hunks)
💤 Files with no reviewable changes (3)
- tests/model_registry/rest_api/mariadb/conftest.py
- tests/model_registry/rest_api/mariadb/utils.py
- tests/model_registry/rest_api/mariadb/test_mr_mariadb.py
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/model_registry/negative_tests/test_db_migration.py
- tests/model_registry/image_validation/test_verify_rhoai_images.py
- tests/model_registry/rbac/multiple_instance_utils.py
- tests/model_registry/python_client/test_model_registry_creation.py
- tests/model_registry/rest_api/test_model_registry_secure_db.py
- tests/model_registry/rbac/test_mr_rbac_sa.py
- tests/model_registry/rest_api/conftest.py
- tests/model_registry/image_validation/conftest.py
- tests/model_registry/rest_api/constants.py
- tests/model_registry/constants.py
- tests/model_registry/upgrade/test_model_registry_upgrade.py
- tests/model_registry/rest_api/test_multiple_mr.py
- tests/model_registry/rest_api/test_model_registry_rest_api.py
- tests/model_registry/upgrade/conftest.py
- tests/model_registry/rbac/test_mr_rbac.py
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-07-17T15:42:23.880Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rbac/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:12.803Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/rest_api/test_model_registry_secure_db.py:16-16
Timestamp: 2025-08-11T12:14:12.803Z
Learning: In tests/model_registry/rest_api/test_model_registry_secure_db.py, both the class-level `model_registry_instance` fixture and the method-level `deploy_secure_mysql_and_mr` fixture are intentionally required together for the secure database test setup. These fixtures serve different purposes and are not redundant.
Applied to files:
tests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rbac/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rbac/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:04.167Z
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.
Applied to files:
tests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rbac/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rbac/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_endpoint list access is not needed because upstream fixture validation already ensures endpoints exist before the tests execute. The Model Registry setup process validates endpoint availability, making additional bounds checks redundant.
Applied to files:
tests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rbac/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-04T00:17:47.799Z
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.
Applied to files:
tests/model_registry/scc/test_model_registry_scc.pytests/model_registry/rbac/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/utils.pytests/model_registry/conftest.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/scc/test_model_registry_scc.pytests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/negative_tests/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-08-12T15:40:14.417Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/conftest.py:78-78
Timestamp: 2025-08-12T15:40:14.417Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance fixture correctly uses param.get("db_name", "mysql") to read test parameters, as tests parameterize using "db_name" as the key (e.g., {"db_name": "mariadb"}). The parameter dict key does not need to match the function parameter name when passing values between fixtures.
Applied to files:
tests/model_registry/negative_tests/test_model_registry_creation_negative.pytests/model_registry/rbac/conftest.pytests/model_registry/negative_tests/conftest.pytests/model_registry/utils.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:43:04.876Z
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.
Applied to files:
tests/model_registry/rbac/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-06-16T11:25:39.599Z
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.
Applied to files:
tests/model_registry/rbac/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_rest_endpoint fixture contains a built-in assertion `assert len(mr_instances) >= 1` that ensures at least one model registry instance exists before returning the endpoint list. This validation makes bounds checking redundant when accessing the first element of the returned list in test methods.
Applied to files:
tests/model_registry/negative_tests/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-09T08:50:22.172Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
Applied to files:
tests/model_registry/negative_tests/conftest.py
📚 Learning: 2025-08-11T07:34:44.352Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/utils.py:34-36
Timestamp: 2025-08-11T07:34:44.352Z
Learning: In the opendatahub-tests repository, container images should use digest references (e.g., `imagesha256:...`) instead of version tags like `:latest`, especially for disconnected environments where specific digests are required for image mirroring. This applies to test fixtures and deployment configurations.
Applied to files:
tests/model_registry/utils.py
📚 Learning: 2025-07-17T15:41:54.284Z
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.
Applied to files:
tests/model_registry/conftest.py
📚 Learning: 2025-06-16T11:26:53.789Z
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.
Applied to files:
tests/model_registry/conftest.py
🧬 Code Graph Analysis (4)
tests/model_registry/negative_tests/test_model_registry_creation_negative.py (1)
tests/model_registry/conftest.py (1)
updated_dsc_component_state_scope_session(157-206)
tests/model_registry/rbac/conftest.py (1)
tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/negative_tests/conftest.py (3)
tests/model_registry/utils.py (2)
get_model_registry_deployment_template_dict(75-204)get_model_registry_db_label_dict(207-212)utilities/general.py (1)
wait_for_pods_by_labels(222-253)tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/conftest.py (2)
tests/model_registry/utils.py (1)
get_model_registry_metadata_resources(635-681)tests/conftest.py (2)
admin_client(66-67)teardown_resources(87-94)
🔇 Additional comments (15)
tests/model_registry/scc/test_model_registry_scc.py (1)
62-64: Good move to session-scoped DSC and consolidated DB resourcesSwitching to updated_dsc_component_state_scope_session and the unified model_registry_metadata_db_resources aligns with the new, parameterized setup and reduces per-test churn.
tests/model_registry/negative_tests/conftest.py (2)
129-132: LGTM: explicit MySQL backend for negative DB deploymentThreading db_backend="mysql" into the deployment template keeps negative tests deterministic. Matches the refactor toward backend-aware templates.
168-176: Verify target DB pod selection is intentionalThis selects pods with label name={DB_RESOURCE_NAME}. Given this file also provisions a “negative” DB (DB_RESOURCES_NAME_NEGATIVE), confirm you intend to mutate the default DB pod, not the negative-test DB.
Would you like me to scan the suite for usages of set_mr_db_dirty to confirm the target DB matches test expectations?
tests/model_registry/negative_tests/test_model_registry_creation_negative.py (2)
23-28: LGTM: updated fixture scope matches the refactorUsing updated_dsc_component_state_scope_session here is consistent with the session-scoped DSC state across the suite.
35-38: LGTM: parameter rename in test signatureThe updated parameter aligns with the fixture name change; test logic remains clear.
tests/model_registry/utils.py (3)
34-37: Good: MariaDB image pinned to digestUsing a pinned digest ensures reproducibility and supports disconnected mirroring.
75-77: Template parameterization looks solid; minor consistency notes
- Container name equals db_backend, which is fine (mysql/mariadb).
- MariaDB branch adds correct labels, config volume, and drops MySQL-specific args.
No action required.
Also applies to: 178-204
684-693: Port selection via PORT_MAP is correctRouting DB port by backend is clean and future-proof.
tests/model_registry/conftest.py (7)
8-8: ConfigMap import path update looks correctSwitching to ocp_resources.config_map.ConfigMap aligns with current ocp-resources structure.
49-51: Good: session-scoped MR namespace derived from DSCUsing the session-scoped updated_dsc_component_state_scope_session to resolve registriesNamespace ensures consistency across tests and avoids redundant DSC patching.
75-79: Correct parameterization passthrough for MR instances
- num is sourced from param.get("num_resources", 1)
- db_backend is sourced from param.get("db_name", "mysql")
This matches how tests pass {"db_name": "mariadb"} and is consistent with our learnings.
100-101: Correct: db_backend derived from db_name test parameterUsing getattr(request, "param", {}).get("db_name", "mysql") is the intended and working pattern per prior guidance and parameterization.
111-113: Fix validated: MariaDB ConfigMap condition compares string to stringThe previous always-false
["mariadb"]list comparison issue is resolved by usingdb_backend == "mariadb". This ensures the ConfigMap list is populated only for MariaDB.
156-157: Session-scoped DSC component state fixture is appropriatePromoting to session scope reduces redundant cluster edits and aligns with central DSC management for the whole test run.
136-138: Avoid potential KeyError when no Deployment instances are createdIf for any reason no Deployment objects are present in resources_instances, indexing with [Deployment] raises a KeyError. Safer to use .get with a default empty list.
Apply this diff:
- for deployment in resources_instances[Deployment]: + for deployment in resources_instances.get(Deployment, []): deployment.wait_for_replicas(deployed=True)⛔ Skipped due to learnings
Learnt from: jiripetrlik PR: opendatahub-io/opendatahub-tests#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: opendatahub-io/opendatahub-tests#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.
dbasunag
force-pushed
the
mariadb-tests
branch
from
aaeb7ed to
c5c0666
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/model_registry/negative_tests/test_db_migration.py (1)
36-43: Don’t wait for Running in a negative migration test; wait for the new pod and CrashLoopBackOffFor a dirty DB migration, the MR pod should fail (CrashLoopBackOff). Waiting for “Running” via wait_for_new_running_mr_pod risks timeouts and flakiness. Instead, wait for the new pod appearance (no Running requirement), then assert CrashLoopBackOff.
Apply this diff in the test to avoid waiting for Running:
- mr_pod = wait_for_new_running_mr_pod( - admin_client=admin_client, - orig_pod_name=model_registry_pod.name, - namespace=py_config["model_registry_namespace"], - instance_name=MR_INSTANCE_NAME, - ) + # Wait for a new MR pod (name differs from the original), without requiring Running state + mr_pod = wait_for_new_mr_pod( + admin_client=admin_client, + orig_pod_name=model_registry_pod.name, + namespace=py_config["model_registry_namespace"], + instance_name=MR_INSTANCE_NAME, + )Add this helper in tests/model_registry/utils.py to support the change:
# tests/model_registry/utils.py from time import sleep from ocp_resources.pod import Pod from utilities.exceptions import ResourceNotFoundError, TimeoutExpiredError def wait_for_new_mr_pod(admin_client, orig_pod_name: str, namespace: str, instance_name: str, timeout: int = 180, sleep_s: int = 5) -> Pod: """ Wait for a new MR pod (different name) with the app label = instance_name to appear. Does not require the pod to be Running. """ label_selector = f"app={instance_name}" elapsed = 0 while elapsed < timeout: pods = list(Pod.get(dyn_client=admin_client, namespace=namespace, label_selector=label_selector)) pods = [p for p in pods if p.name != orig_pod_name] if pods: return pods[0] sleep(sleep_s) elapsed += sleep_s raise TimeoutExpiredError(f"Timed out waiting for new MR pod (label {label_selector}) different from {orig_pod_name}")
♻️ Duplicate comments (3)
tests/model_registry/upgrade/conftest.py (1)
14-29: Patch approach is correct; ResourceEditor merges component entriesUsing ResourceEditor with a partial components dict to toggle Model Registry’s managementState is correct and won’t clobber other DSC components (as established in prior discussion).
tests/model_registry/utils.py (2)
35-37: LGTM - MariaDB image with pinned digest.The use of a pinned digest for the MariaDB image follows security best practices and ensures reproducible builds, which is especially important for disconnected environments.
487-509: Fix return type annotation and add conditional ConfigMap creation.The return type should be
list[ConfigMap]instead oflist[Service], and ConfigMap creation should be conditional based on the database backend.-def get_mr_configmap_objects( +def get_mr_configmap_objects( base_name: str, namespace: str, client: DynamicClient, teardown_resources: bool, num: int, -) -> list[Service]: - config_maps = [] - - for num_config_map in range(0, num): + db_backend: str = "mysql", +) -> list[ConfigMap]: + # Only MariaDB requires ConfigMap for my.cnf configuration + if db_backend != "mariadb": + return [] + + config_maps = [] + for num_config_map in range(0, num):
🧹 Nitpick comments (2)
tests/model_registry/constants.py (1)
57-65: Readable CNF and right-sized defaultsThe additions look good. Two suggestions:
- Use a triple-quoted string for MARIADB_MY_CNF to improve readability.
- Consider lowering innodb_buffer_pool_size from 1024M to a smaller value (e.g., 256M) to reduce OOM risk in constrained test clusters.
Here’s a readability-focused change:
-MARIADB_MY_CNF = ( - "[mysqld]\nbind-address=0.0.0.0\ndefault_storage_engine=InnoDB\n" - "binlog_format=row\ninnodb_autoinc_lock_mode=2\ninnodb_buffer_pool_size=1024M" - "\nmax_allowed_packet=256M\n" -) +MARIADB_MY_CNF = """\ +[mysqld] +bind-address=0.0.0.0 +default_storage_engine=InnoDB +binlog_format=row +innodb_autoinc_lock_mode=2 +innodb_buffer_pool_size=1024M +max_allowed_packet=256M +"""tests/model_registry/image_validation/conftest.py (1)
12-20: Good addition; make the docstring reflect label selection and confirm the label keyExplicit function scope and label-based retrieval are appropriate here. Update the docstring for clarity and confirm the ‘app’ label truly equals MR_INSTANCE_NAME in your environment; many operators prefer app.kubernetes.io/instance or similar.
Apply this small docstring nit:
- """Get the model registry instance pod.""" + """Get the model registry instance pod by label."""If needed, adjust the selector after confirming the label:
- app.kubernetes.io/instance={MR_INSTANCE_NAME}
- modelregistry.opendatahub.io/instance={MR_INSTANCE_NAME}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
tests/model_registry/conftest.py(5 hunks)tests/model_registry/constants.py(1 hunks)tests/model_registry/image_validation/conftest.py(1 hunks)tests/model_registry/image_validation/test_verify_rhoai_images.py(2 hunks)tests/model_registry/negative_tests/conftest.py(3 hunks)tests/model_registry/negative_tests/test_db_migration.py(1 hunks)tests/model_registry/negative_tests/test_model_registry_creation_negative.py(2 hunks)tests/model_registry/python_client/test_model_registry_creation.py(1 hunks)tests/model_registry/rbac/conftest.py(2 hunks)tests/model_registry/rbac/multiple_instance_utils.py(4 hunks)tests/model_registry/rbac/test_mr_rbac.py(4 hunks)tests/model_registry/rbac/test_mr_rbac_sa.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(1 hunks)tests/model_registry/rest_api/mariadb/conftest.py(0 hunks)tests/model_registry/rest_api/mariadb/test_mr_mariadb.py(0 hunks)tests/model_registry/rest_api/mariadb/utils.py(0 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(2 hunks)tests/model_registry/rest_api/test_multiple_mr.py(2 hunks)tests/model_registry/scc/test_model_registry_scc.py(1 hunks)tests/model_registry/upgrade/conftest.py(1 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(3 hunks)tests/model_registry/utils.py(13 hunks)
💤 Files with no reviewable changes (3)
- tests/model_registry/rest_api/mariadb/utils.py
- tests/model_registry/rest_api/mariadb/test_mr_mariadb.py
- tests/model_registry/rest_api/mariadb/conftest.py
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/model_registry/rest_api/constants.py
- tests/model_registry/scc/test_model_registry_scc.py
- tests/model_registry/rest_api/conftest.py
- tests/model_registry/python_client/test_model_registry_creation.py
- tests/model_registry/rest_api/test_model_registry_rest_api.py
- tests/model_registry/image_validation/test_verify_rhoai_images.py
- tests/model_registry/rbac/conftest.py
- tests/model_registry/negative_tests/conftest.py
- tests/model_registry/rest_api/test_model_registry_secure_db.py
- tests/model_registry/rbac/test_mr_rbac.py
- tests/model_registry/rest_api/test_multiple_mr.py
- tests/model_registry/negative_tests/test_model_registry_creation_negative.py
- tests/model_registry/rbac/test_mr_rbac_sa.py
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-07-17T15:42:23.880Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/negative_tests/test_db_migration.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/upgrade/conftest.pytests/model_registry/image_validation/conftest.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:12.803Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/rest_api/test_model_registry_secure_db.py:16-16
Timestamp: 2025-08-11T12:14:12.803Z
Learning: In tests/model_registry/rest_api/test_model_registry_secure_db.py, both the class-level `model_registry_instance` fixture and the method-level `deploy_secure_mysql_and_mr` fixture are intentionally required together for the secure database test setup. These fixtures serve different purposes and are not redundant.
Applied to files:
tests/model_registry/negative_tests/test_db_migration.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/upgrade/conftest.pytests/model_registry/image_validation/conftest.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/conftest.py
📚 Learning: 2025-08-12T15:40:14.417Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/conftest.py:78-78
Timestamp: 2025-08-12T15:40:14.417Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance fixture correctly uses param.get("db_name", "mysql") to read test parameters, as tests parameterize using "db_name" as the key (e.g., {"db_name": "mariadb"}). The parameter dict key does not need to match the function parameter name when passing values between fixtures.
Applied to files:
tests/model_registry/negative_tests/test_db_migration.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/utils.pytests/model_registry/image_validation/conftest.pytests/model_registry/rbac/multiple_instance_utils.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:04.167Z
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.
Applied to files:
tests/model_registry/negative_tests/test_db_migration.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/upgrade/conftest.pytests/model_registry/image_validation/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/negative_tests/test_db_migration.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/upgrade/conftest.pytests/model_registry/image_validation/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-09T08:50:22.172Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
Applied to files:
tests/model_registry/negative_tests/test_db_migration.py
📚 Learning: 2025-07-04T00:17:47.799Z
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.
Applied to files:
tests/model_registry/constants.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/utils.pytests/model_registry/image_validation/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_endpoint list access is not needed because upstream fixture validation already ensures endpoints exist before the tests execute. The Model Registry setup process validates endpoint availability, making additional bounds checks redundant.
Applied to files:
tests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/image_validation/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/image_validation/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_rest_endpoint fixture contains a built-in assertion `assert len(mr_instances) >= 1` that ensures at least one model registry instance exists before returning the endpoint list. This validation makes bounds checking redundant when accessing the first element of the returned list in test methods.
Applied to files:
tests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/image_validation/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/upgrade/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:43:04.876Z
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.
Applied to files:
tests/model_registry/upgrade/conftest.pytests/model_registry/image_validation/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T07:34:44.352Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/utils.py:34-36
Timestamp: 2025-08-11T07:34:44.352Z
Learning: In the opendatahub-tests repository, container images should use digest references (e.g., `imagesha256:...`) instead of version tags like `:latest`, especially for disconnected environments where specific digests are required for image mirroring. This applies to test fixtures and deployment configurations.
Applied to files:
tests/model_registry/utils.py
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#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.
Applied to files:
tests/model_registry/image_validation/conftest.py
📚 Learning: 2025-07-17T15:41:54.284Z
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.
Applied to files:
tests/model_registry/conftest.py
📚 Learning: 2025-06-16T11:26:53.789Z
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.
Applied to files:
tests/model_registry/conftest.py
🧬 Code Graph Analysis (6)
tests/model_registry/upgrade/test_model_registry_upgrade.py (1)
tests/model_registry/conftest.py (1)
model_registry_instance(55-88)
tests/model_registry/upgrade/conftest.py (3)
tests/model_registry/utils.py (1)
wait_for_pods_running(247-280)utilities/constants.py (2)
DscComponents(156-177)ManagementState(162-164)tests/conftest.py (2)
dsc_resource(415-416)admin_client(66-67)
tests/model_registry/utils.py (1)
tests/conftest.py (1)
teardown_resources(87-94)
tests/model_registry/image_validation/conftest.py (2)
utilities/general.py (1)
wait_for_pods_by_labels(222-253)tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/rbac/multiple_instance_utils.py (1)
tests/model_registry/utils.py (2)
get_model_registry_deployment_template_dict(75-204)get_mysql_config(682-690)
tests/model_registry/conftest.py (4)
tests/model_registry/utils.py (3)
get_model_registry_metadata_resources(634-679)get_mr_service_by_label(41-65)get_endpoint_from_mr_service(68-72)tests/conftest.py (2)
admin_client(66-67)teardown_resources(87-94)tests/model_serving/conftest.py (1)
protocol(26-36)utilities/constants.py (1)
Protocols(91-98)
🔇 Additional comments (29)
tests/model_registry/negative_tests/test_db_migration.py (1)
15-17: Fixture consolidation looks goodSwitching to session-scoped DSC state and consolidated DB resources is consistent with the PR’s goals and aligns with the updated fixture graph.
tests/model_registry/rbac/multiple_instance_utils.py (1)
19-21: Namespace sourcing via config is preferableDeriving ns_name from py_config ensures deterministic behavior across runs and aligns with other refactors in this PR.
tests/model_registry/upgrade/conftest.py (1)
30-38: Post-patch readiness gates are sensibleWaiting for the ModelRegistryReady condition, namespace ACTIVE, and a stable applications namespace improves upgrade-test robustness.
tests/model_registry/upgrade/test_model_registry_upgrade.py (5)
31-31: LGTM - Fixture migration to consolidated DB resources.The change from
mysql_metadata_resourcestomodel_registry_metadata_db_resourcesaligns with the PR's objective to consolidate DB fixtures and support multiple database backends (MySQL and MariaDB).
52-52: LGTM - Consistent fixture name update.The parameter type hint removal and fixture name change from
model_registry_instance_mysqltomodel_registry_instanceis consistent with the consolidation effort.
64-64: LGTM - Consistent fixture name update.The fixture name change maintains consistency across all post-upgrade tests.
67-67: LGTM - Updated property access path.The change from
model_registry_instance_mysql[0].instance.apiVersiontomodel_registry_instance[0].instance.apiVersioncorrectly reflects the new fixture naming while maintaining the same property access pattern.
74-76: LGTM - Consistent property access updates.Both the fixture parameter name and the property access path have been updated consistently to use the new consolidated fixture.
tests/model_registry/utils.py (13)
7-7: LGTM - Required import for ConfigMap support.The addition of
ConfigMapimport is necessary for the MariaDB backend support, as MariaDB requires ConfigMap resources for configuration.
24-26: LGTM - Required imports for MariaDB support.The addition of
MARIADB_MY_CNFandPORT_MAPimports is necessary for MariaDB backend configuration and port mapping functionality.
75-77: LGTM - Function signature update for database backend support.The addition of the
db_backendparameter enables the function to handle different database types (MySQL and MariaDB) dynamically.
78-204: LGTM - Comprehensive MariaDB backend configuration.The conditional logic correctly configures MariaDB-specific settings including:
- Updated labels for app and component identification
- MariaDB image substitution
- Additional environment variable for MariaDB root password
- Volume mounts for data and configuration
- Removal of MySQL-specific args
- Volume configuration for both data (PVC) and config (ConfigMap)
The implementation follows good practices by using the backend name in volume names and maintaining consistent labeling.
448-454: LGTM - Consistent function signature standardization.The removal of default parameter values aligns with the PR's approach to make parameter passing explicit at the fixture level, improving maintainability and reducing hidden bugs.
511-513: LGTM - Consistent function signature standardization.The removal of default parameter values maintains consistency with other utility functions.
531-533: LGTM - Consistent function signature standardization.The parameter signature standardization improves consistency across the utility functions.
556-558: LGTM - Function signature update for backend support.The addition of
db_backendparameter enables deployment configuration based on the database type.
561-583: LGTM - Backend-aware deployment configuration.The conditional logic correctly configures MariaDB-specific selectors and labels for proper resource identification and service discovery.
606-607: LGTM - Function signature update for backend support.The addition of
db_backendparameter enables backend-specific configuration for Model Registry objects.
612-614: LGTM - Backend-aware MySQL configuration.The function call correctly passes the
db_backendparameter to enable backend-specific port configuration.
634-680: LGTM - Comprehensive metadata resources aggregator.This new function effectively consolidates all DB resource creation into a single interface, supporting the PR's objective to simplify fixture management. The function correctly handles all resource types needed for Model Registry database backend support.
682-690: LGTM - Backend-aware port configuration.The update to use
PORT_MAP[db_backend]enables correct port configuration for different database backends (MySQL port 3306, MariaDB port 3306, etc.).tests/model_registry/conftest.py (8)
8-8: LGTM - Required import for ConfigMap support.The addition of
ConfigMapimport is necessary for MariaDB backend support, as it requires ConfigMap resources for database configuration.
39-39: LGTM - Import update for consolidated resource management.The addition of
get_model_registry_metadata_resourcesimport supports the PR's objective to consolidate DB resource management into a single function.
49-51: LGTM - Session-scoped namespace fixture.The scope change from class to session and the fixture dependency update align with the PR's objective to make fixtures session-scoped for better resource efficiency.
55-62: LGTM - Updated fixture signature for consolidated DB resources.The replacement of individual DB resource fixtures with
model_registry_metadata_db_resourceseffectively consolidates resource management as intended by the PR.
75-78: LGTM - Backend-aware Model Registry object creation.The parameter extraction and passing to
get_model_registry_objectscorrectly enables backend-specific configuration based on test parameters.
92-139: LGTM - Comprehensive consolidated DB resources fixture.This new fixture effectively consolidates all database resource management into a single point, supporting both pre-upgrade resource creation and post-upgrade resource discovery. The implementation correctly:
- Handles both upgrade and non-upgrade scenarios
- Uses the consolidated
get_model_registry_metadata_resourcesfunction- Manages resource lifecycle with proper teardown
- Supports both MySQL and MariaDB backends through conditional ConfigMap handling
The fixture simplifies test setup while maintaining flexibility for different database backends.
142-153: LGTM - Instance-aware endpoint discovery.The updated fixture correctly derives service endpoints from the Model Registry instances rather than namespace-scoped discovery, providing better isolation and accuracy for parameterized tests.
156-207: LGTM - Session-scoped DSC state management.The scope change to session and the comprehensive DSC component management logic correctly handles:
- Session-level resource lifecycle management
- Proper namespace creation and cleanup
- Component state transitions
- Resource editor patching with appropriate component mappings
This change supports the PR's objective to make fixtures more efficient by reducing redundant DSC operations across test classes.
dbasunag
force-pushed
the
mariadb-tests
branch
from
c5c0666 to
bdb7f65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
tests/model_registry/rest_api/conftest.py (2)
122-140: Remove unused fixture: mysql_template_with_ca is dead codeThis fixture builds a template but nothing consumes it. It’s only referenced as a dependency in deploy_secure_mysql_and_mr and never used there. Keeping it introduces confusion and maintenance burden.
Apply this diff to remove the unused fixture:
@@ -@pytest.fixture(scope="class") -def mysql_template_with_ca(model_registry_metadata_db_resources: dict[Any, Any]) -> dict[str, Any]: - """ - Patches the MySQL template with the CA file path and volume mount. - """ - mysql_template = get_model_registry_deployment_template_dict( - secret_name=model_registry_metadata_db_resources[Secret][0].name, - resource_name=DB_RESOURCE_NAME, - db_backend="mysql", - ) - mysql_template["spec"]["containers"][0]["args"].append(f"--ssl-ca={CA_FILE_PATH}") - mysql_template["spec"]["containers"][0]["volumeMounts"].append({ - "mountPath": CA_MOUNT_PATH, - "name": CA_CONFIGMAP_NAME, - "readOnly": True, - }) - mysql_template["spec"]["volumes"].append({"name": CA_CONFIGMAP_NAME, "configMap": {"name": CA_CONFIGMAP_NAME}}) - return mysql_template
143-149: Drop unused mysql_template_with_ca dependency from deploy_secure_mysql_and_mrSince mysql_template_with_ca is unused, remove it from the signature to avoid triggering an unnecessary fixture.
def deploy_secure_mysql_and_mr( request: pytest.FixtureRequest, admin_client: DynamicClient, model_registry_namespace: str, - mysql_template_with_ca: dict[str, Any], patch_mysql_deployment_with_ssl_ca: Deployment, ) -> Generator[ModelRegistry, None, None]:tests/model_registry/utils.py (1)
75-205: MariaDB readiness/liveness may fail: MYSQL_ROOT_PASSWORD env not guaranteed in MariaDB imageThe probes reference ${MYSQL_ROOT_PASSWORD}. MariaDB images typically use MARIADB_* env vars. While some images alias MYSQL_* to MARIADB_*, it's not guaranteed. Use a bash fallback to support both.
Apply this diff to make the probes robust for both MySQL and MariaDB:
- "livenessProbe": { - "exec": { - "command": [ - "/bin/bash", - "-c", - "mysqladmin -u${MYSQL_USER} -p${MYSQL_ROOT_PASSWORD} ping", - ] - }, + "livenessProbe": { + "exec": { + "command": [ + "/bin/bash", + "-c", + 'mysqladmin -u"${MYSQL_USER}" -p"${MYSQL_ROOT_PASSWORD:-$MARIADB_ROOT_PASSWORD}" ping', + ] + }, "initialDelaySeconds": 15, "periodSeconds": 10, "timeoutSeconds": 5, }, @@ - "readinessProbe": { - "exec": { - "command": [ - "/bin/bash", - "-c", - 'mysql -D ${MYSQL_DATABASE} -u${MYSQL_USER} -p${MYSQL_ROOT_PASSWORD} -e "SELECT 1"', - ] - }, + "readinessProbe": { + "exec": { + "command": [ + "/bin/bash", + "-c", + 'mysql -D "${MYSQL_DATABASE}" -u"${MYSQL_USER}" -p"${MYSQL_ROOT_PASSWORD:-$MARIADB_ROOT_PASSWORD}" -e "SELECT 1"', + ] + }, "initialDelaySeconds": 10, "timeoutSeconds": 5, },
♻️ Duplicate comments (1)
tests/model_registry/utils.py (1)
487-510: Wrong return annotation for get_mr_configmap_objects; should be list[ConfigMap]The function returns ConfigMap objects (and conditionally none for non-MariaDB), but the return type is list[Service]. Update the annotation.
-def get_mr_configmap_objects( +def get_mr_configmap_objects( base_name: str, namespace: str, client: DynamicClient, teardown_resources: bool, num: int, db_backend: str, -) -> list[Service]: +) -> list[ConfigMap]:
🧹 Nitpick comments (5)
tests/model_registry/rest_api/conftest.py (4)
245-256: Avoid shadowing module-level constants; use local variable namesThe function-local CA_CONFIGMAP_NAME and CA_MOUNT_PATH shadow similarly named module-level constants, which is error-prone. Rename the locals for clarity.
- CA_CONFIGMAP_NAME = request.param.get("ca_configmap_name", "mysql-ca-configmap") - CA_MOUNT_PATH = request.param.get("ca_mount_path", "/etc/mysql/ssl") + ca_configmap_name = request.param.get("ca_configmap_name", "mysql-ca-configmap") + ca_mount_path = request.param.get("ca_mount_path", "/etc/mysql/ssl") @@ - my_sql_container = apply_mysql_args_and_volume_mounts( - my_sql_container=my_sql_container, ca_configmap_name=CA_CONFIGMAP_NAME, ca_mount_path=CA_MOUNT_PATH - ) - volumes = add_mysql_certs_volumes_to_deployment(spec=spec, ca_configmap_name=CA_CONFIGMAP_NAME) + my_sql_container = apply_mysql_args_and_volume_mounts( + my_sql_container=my_sql_container, ca_configmap_name=ca_configmap_name, ca_mount_path=ca_mount_path + ) + volumes = add_mysql_certs_volumes_to_deployment(spec=spec, ca_configmap_name=ca_configmap_name)
195-195: Harden cleanup: avoid FileNotFoundError in local_ca_bundle teardownIf the file was not created or already removed, os.remove will raise. Guard it.
- os.remove(ca_bundle_path) + if os.path.exists(ca_bundle_path): + os.remove(ca_bundle_path)
270-277: Docstring drift: mysql_ssl_artifact_paths Args section is inaccurateThis fixture doesn't accept admin_client/model_registry_namespace/mysql_ssl_artifacts..., but the docstring says it does. Update or simplify to avoid confusion.
""" Generates MySQL SSL certificate and key files in a temporary directory and provides their paths. - Args: - admin_client: The admin client to create the ConfigMap - model_registry_namespace: The namespace of the model registry - mysql_ssl_artifacts_and_secrets: The artifacts and secrets for the MySQL SSL connection - Returns: Generator[dict[str, str], None, None]: A generator that yields the CA certificate and key file paths. """
291-298: Docstring mismatch with return type in mysql_ssl_secretsThe docstring says dict[str, str] but the fixture returns dict[str, Secret]. Fix the docstring.
""" Creates Kubernetes secrets for MySQL SSL artifacts. Args: - admin_client: The admin client to create the ConfigMap + admin_client: The admin client to create the Secrets model_registry_namespace: The namespace of the model registry - mysql_ssl_artifacts_and_secrets: The artifacts and secrets for the MySQL SSL connection + mysql_ssl_artifact_paths: The artifacts (file paths) for the MySQL SSL connection Returns: - Generator[dict[str, str], None, None]: A generator that yields the CA certificate and key file paths. + Generator[dict[str, Secret], None, None]: A generator that yields the created Secrets keyed by purpose. """tests/model_registry/rest_api/test_multiple_mr.py (1)
32-36: Remove unused fixture parameter from test method signatureThe method doesn't use model_registry_instance, and it's already ensured by @usefixtures. Drop it to reduce noise.
- def test_validate_multiple_model_registry(self: Self, model_registry_instance, model_registry_namespace: str): + def test_validate_multiple_model_registry(self: Self, model_registry_namespace: str):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
tests/model_registry/conftest.py(5 hunks)tests/model_registry/constants.py(1 hunks)tests/model_registry/image_validation/conftest.py(1 hunks)tests/model_registry/image_validation/test_verify_rhoai_images.py(2 hunks)tests/model_registry/negative_tests/conftest.py(3 hunks)tests/model_registry/negative_tests/test_db_migration.py(1 hunks)tests/model_registry/negative_tests/test_model_registry_creation_negative.py(2 hunks)tests/model_registry/python_client/test_model_registry_creation.py(1 hunks)tests/model_registry/rbac/conftest.py(2 hunks)tests/model_registry/rbac/multiple_instance_utils.py(4 hunks)tests/model_registry/rbac/test_mr_rbac.py(4 hunks)tests/model_registry/rbac/test_mr_rbac_sa.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/constants.py(1 hunks)tests/model_registry/rest_api/mariadb/conftest.py(0 hunks)tests/model_registry/rest_api/mariadb/test_mr_mariadb.py(0 hunks)tests/model_registry/rest_api/mariadb/utils.py(0 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(2 hunks)tests/model_registry/rest_api/test_multiple_mr.py(2 hunks)tests/model_registry/scc/test_model_registry_scc.py(1 hunks)tests/model_registry/upgrade/conftest.py(1 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(3 hunks)tests/model_registry/utils.py(13 hunks)
💤 Files with no reviewable changes (3)
- tests/model_registry/rest_api/mariadb/test_mr_mariadb.py
- tests/model_registry/rest_api/mariadb/utils.py
- tests/model_registry/rest_api/mariadb/conftest.py
🚧 Files skipped from review as they are similar to previous changes (16)
- tests/model_registry/negative_tests/test_db_migration.py
- tests/model_registry/image_validation/conftest.py
- tests/model_registry/rbac/test_mr_rbac_sa.py
- tests/model_registry/constants.py
- tests/model_registry/rest_api/constants.py
- tests/model_registry/rest_api/test_model_registry_secure_db.py
- tests/model_registry/negative_tests/conftest.py
- tests/model_registry/rest_api/test_model_registry_rest_api.py
- tests/model_registry/rbac/conftest.py
- tests/model_registry/negative_tests/test_model_registry_creation_negative.py
- tests/model_registry/image_validation/test_verify_rhoai_images.py
- tests/model_registry/scc/test_model_registry_scc.py
- tests/model_registry/python_client/test_model_registry_creation.py
- tests/model_registry/upgrade/conftest.py
- tests/model_registry/rbac/test_mr_rbac.py
- tests/model_registry/rbac/multiple_instance_utils.py
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-07-17T15:42:23.880Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-04T00:17:47.799Z
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.
Applied to files:
tests/model_registry/rest_api/conftest.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/utils.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:04.167Z
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.
Applied to files:
tests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-08-12T15:40:14.417Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/conftest.py:78-78
Timestamp: 2025-08-12T15:40:14.417Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance fixture correctly uses param.get("db_name", "mysql") to read test parameters, as tests parameterize using "db_name" as the key (e.g., {"db_name": "mariadb"}). The parameter dict key does not need to match the function parameter name when passing values between fixtures.
Applied to files:
tests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/utils.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:12.803Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/rest_api/test_model_registry_secure_db.py:16-16
Timestamp: 2025-08-11T12:14:12.803Z
Learning: In tests/model_registry/rest_api/test_model_registry_secure_db.py, both the class-level `model_registry_instance` fixture and the method-level `deploy_secure_mysql_and_mr` fixture are intentionally required together for the secure database test setup. These fixtures serve different purposes and are not redundant.
Applied to files:
tests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:41:54.284Z
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.
Applied to files:
tests/model_registry/rest_api/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:42:26.275Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/rest_api/conftest.pytests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-17T15:43:04.876Z
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.
Applied to files:
tests/model_registry/rest_api/conftest.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_rest_endpoint fixture contains a built-in assertion `assert len(mr_instances) >= 1` that ensures at least one model registry instance exists before returning the endpoint list. This validation makes bounds checking redundant when accessing the first element of the returned list in test methods.
Applied to files:
tests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_endpoint list access is not needed because upstream fixture validation already ensures endpoints exist before the tests execute. The Model Registry setup process validates endpoint availability, making additional bounds checks redundant.
Applied to files:
tests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-06-16T11:26:53.789Z
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.
Applied to files:
tests/model_registry/rest_api/test_multiple_mr.pytests/model_registry/conftest.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/conftest.py
🧬 Code Graph Analysis (5)
tests/model_registry/rest_api/conftest.py (2)
tests/model_registry/conftest.py (2)
model_registry_metadata_db_resources(92-138)model_registry_namespace(50-51)tests/model_registry/utils.py (2)
get_model_registry_deployment_template_dict(75-204)get_mysql_config(684-692)
tests/model_registry/rest_api/test_multiple_mr.py (1)
tests/model_registry/conftest.py (2)
model_registry_instance(55-88)model_registry_namespace(50-51)
tests/model_registry/upgrade/test_model_registry_upgrade.py (1)
tests/model_registry/conftest.py (1)
model_registry_instance(55-88)
tests/model_registry/utils.py (1)
tests/conftest.py (1)
teardown_resources(87-94)
tests/model_registry/conftest.py (4)
tests/model_registry/utils.py (3)
get_model_registry_metadata_resources(635-681)get_mr_service_by_label(41-65)get_endpoint_from_mr_service(68-72)tests/conftest.py (2)
admin_client(66-67)teardown_resources(87-94)tests/model_serving/conftest.py (1)
protocol(26-36)utilities/constants.py (1)
Protocols(91-98)
🔇 Additional comments (10)
tests/model_registry/rest_api/conftest.py (1)
154-166: LGTM: Explicitly passing db_backend="mysql"Explicitly passing db_backend improves clarity and avoids accidental changes from defaults. No issues spotted here.
tests/model_registry/rest_api/test_multiple_mr.py (2)
16-25: LGTM: Consolidated parameterization with unified fixturesUsing model_registry_metadata_db_resources and model_registry_instance as indirect params simplifies setup across backends and aligns with the new fixture model.
26-30: LGTM: usefixtures aligned with session-scoped DSC and new resource fixturesThe updated usefixtures set correctly ensures DSC state, DB resources, and MR instances are provisioned ahead of tests.
tests/model_registry/upgrade/test_model_registry_upgrade.py (3)
31-31: LGTM: Pre-upgrade class uses consolidated fixturesSwitching to model_registry_metadata_db_resources and model_registry_instance fits the new backend-parameterized setup.
49-68: LGTM: Post-upgrade tests now consume model_registry_instanceUsing model_registry_instance[0] to access apiVersion/spec keeps semantics identical while aligning with the new fixture naming.
72-77: LGTM: Spec assertions adjusted to new fixture variableThe spec checks remain valid and focused on the upgrade guarantees.
tests/model_registry/utils.py (1)
683-692: LGTM: get_mysql_config now uses PORT_MAP[db_backend]Using a backend-aware port map makes the config correct for both MySQL and MariaDB.
tests/model_registry/conftest.py (3)
49-51: LGTM: Session-scoped model_registry_namespaceSession scope reduces churn and avoids repeated namespace resolution across classes.
55-79: LGTM: model_registry_instance reads db_name from test paramsUsing param.get("db_name", "mysql") is correct per current test parameterization. This matches prior guidance and avoids breaking existing tests.
Note: This aligns with the retrieved learning confirming tests pass the "db_name" key.
142-154: LGTM: Endpoint derivation based on MR instancesDeriving REST endpoints from MR services tied to each instance ensures correct fan-out across multiple registries.
rhods-ci-bot
added
commented-by-dbasunag
and removed
commented-by-dbasunag
lgtm-by-fege
labels
dbasunag
force-pushed
the
mariadb-tests
branch
from
f3f046f to
79e7e58
Compare
|
/build-push-pr-image |
|
Status of building tag pr-504: success. |
|
/verified |
|
Status of building tag latest: success. |
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Tests
Chores