minor changes to model registry rests and fixtures#614
Merged
dbasunag merged 1 commit intoopendatahub-io:mainfrom Sep 16, 2025
Merged
minor changes to model registry rests and fixtures#614dbasunag merged 1 commit intoopendatahub-io:mainfrom
dbasunag merged 1 commit intoopendatahub-io:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughTest-suite test files updated: one fixture signature reduced, RBAC test parameterization and context construction adjusted, REST API fixtures add pod readiness wait and secret teardown, and secure-DB tests update class/test decorators and fixture usage. No production code changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/wip', '/verified', '/lgtm', '/cherry-pick', '/build-push-pr-image'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/model_registry/rest_api/conftest.py (1)
260-264: Reorder waits to include rollout pods and reduce namespace flakinessCall wait_for_condition(Available) first, then wait_for_pods_running; the current call snapshots pods pre‑rollout and is namespace‑wide. Reordering aligns with deploy_secure_mysql_and_mr and ensures new pods are checked.
- wait_for_pods_running( - admin_client=admin_client, namespace_name=model_registry_namespace, number_of_consecutive_checks=3 - ) - model_registry_db_deployments[0].wait_for_condition(condition="Available", status="True") + model_registry_db_deployments[0].wait_for_condition(condition="Available", status="True") + wait_for_pods_running( + admin_client=admin_client, namespace_name=model_registry_namespace, number_of_consecutive_checks=3 + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/model_registry/conftest.py(0 hunks)tests/model_registry/rbac/conftest.py(1 hunks)tests/model_registry/rbac/multiple_instance_utils.py(0 hunks)tests/model_registry/rbac/test_mr_rbac.py(0 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(2 hunks)
💤 Files with no reviewable changes (3)
- tests/model_registry/rbac/test_mr_rbac.py
- tests/model_registry/conftest.py
- tests/model_registry/rbac/multiple_instance_utils.py
🧰 Additional context used
🧠 Learnings (12)
📚 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/conftest.pytests/model_registry/rest_api/test_model_registry_secure_db.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/conftest.pytests/model_registry/rest_api/test_model_registry_secure_db.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/conftest.pytests/model_registry/rest_api/test_model_registry_secure_db.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/rbac/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/rest_api/test_model_registry_secure_db.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/rest_api/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/test_model_registry_secure_db.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/test_model_registry_secure_db.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_secure_db.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/rest_api/test_model_registry_secure_db.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/test_model_registry_secure_db.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_secure_db.py
🧬 Code graph analysis (1)
tests/model_registry/rest_api/conftest.py (3)
tests/model_registry/utils.py (1)
wait_for_pods_running(248-281)tests/conftest.py (1)
admin_client(68-69)tests/model_registry/conftest.py (1)
model_registry_namespace(62-63)
🔇 Additional comments (3)
tests/model_registry/rest_api/conftest.py (1)
329-334: LGTM: explicit cleanup of SSL secretsPost‑yield deletion looks good and prevents leaked secrets.
tests/model_registry/rest_api/test_model_registry_secure_db.py (2)
16-16: Confirm dropping model_registry_instance was intentionalPrior learnings indicated class‑level model_registry_instance and deploy_secure_mysql_and_mr serve different purposes. Please confirm no hidden dependencies rely on the former. I’m referencing our past note explicitly.
90-94: LGTM: fixture ordering for valid CA pathOrder ensures CA ConfigMap exists before patching and deploy. Matches the secure‑DB flow.
Comment on lines
+345
to
347
| mr_instances = [stack.enter_context(ModelRegistry(**param)) for param in request.param] | ||
| for mr_instance in mr_instances: | ||
| # Common parameters for both ModelRegistry classes |
Contributor
There was a problem hiding this comment.
Add teardown=teardown_resources to ModelRegistry contexts to avoid leaks
Other parametrized fixtures pass teardown; missing it here may leave MR instances behind.
- mr_instances = [stack.enter_context(ModelRegistry(**param)) for param in request.param]
+ mr_instances = [
+ stack.enter_context(ModelRegistry(**param, teardown=teardown_resources))
+ for param in request.param
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mr_instances = [stack.enter_context(ModelRegistry(**param)) for param in request.param] | |
| for mr_instance in mr_instances: | |
| # Common parameters for both ModelRegistry classes | |
| mr_instances = [ | |
| stack.enter_context(ModelRegistry(**param, teardown=teardown_resources)) | |
| for param in request.param | |
| ] | |
| for mr_instance in mr_instances: | |
| # Common parameters for both ModelRegistry classes |
🤖 Prompt for AI Agents
In tests/model_registry/rbac/conftest.py around lines 345 to 347, the
ModelRegistry instances are created via
stack.enter_context(ModelRegistry(**param)) but omit the
teardown=teardown_resources argument which can leak resources; update the
context creation to pass teardown=teardown_resources to each ModelRegistry
invocation (e.g. stack.enter_context(ModelRegistry(...,
teardown=teardown_resources))) so the fixture tears down MR instances properly.
lugi0
requested changes
Sep 16, 2025
Collaborator
Author
|
/build-push-pr-image |
|
Status of building tag pr-614: success. |
dbasunag
force-pushed
the
minor_adjustments
branch
from
87bef37 to
6681300
Compare
lugi0
approved these changes
Sep 16, 2025
lugi0
reviewed
Sep 16, 2025
| "patch_mysql_deployment_with_ssl_ca", | ||
| "patch_invalid_ca", | ||
| ) | ||
| @pytest.mark.sanity |
Collaborator
Author
There was a problem hiding this comment.
Will add it back.
adolfo-ab
approved these changes
Sep 16, 2025
Collaborator
Author
|
/build-push-pr-image |
|
Status of building tag pr-614: success. |
|
Status of building tag latest: success. |
sheltoncyril
pushed a commit
to sheltoncyril/opendatahub-tests
that referenced
this pull request
Oct 2, 2025
mwaykole
pushed a commit
to mwaykole/opendatahub-tests
that referenced
this pull request
Jan 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit