test: Add deployment tests for registered models#1174
Merged
sheltoncyril merged 6 commits intoopendatahub-io:mainfrom Mar 6, 2026
Merged
test: Add deployment tests for registered models#1174sheltoncyril merged 6 commits intoopendatahub-io:mainfrom
sheltoncyril merged 6 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/lgtm', '/build-push-pr-image', '/wip', '/cherry-pick', '/hold'} |
Contributor
📝 WalkthroughWalkthroughAdds end-to-end OpenVINO/OpenDataHub Model Registry test infrastructure and an E2E test: new exceptions, helper to fetch the OpenVINO image, fixtures for namespace, secret, ServingRuntime, InferenceService, predictor Pod lookup, and port-forwarding; updates a model artifact URI; adds a test that calls the deployed model endpoint via HTTP. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 2
🧹 Nitpick comments (2)
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py (1)
369-371: Remove unused fixture arguments from the test signature.The arguments
admin_client,model_registry_deployment_ns, andmodel_registry_serving_runtimeare not referenced in the method body. The class-level@pytest.mark.usefixtures(...)already ensures setup for the latter two, allowing them to be safely removed from the signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py` around lines 369 - 371, Remove the unused fixture arguments from the test function signature: drop admin_client, model_registry_deployment_ns, and model_registry_serving_runtime from the function parameter list so the test no longer declares unused fixtures; the class-level `@pytest.mark.usefixtures` already provides setup for model_registry_deployment_ns and model_registry_serving_runtime, so keep any other required fixtures but ensure the test function only lists fixtures it actually references.tests/model_registry/model_registry/rest_api/conftest.py (1)
739-741: UseLOGGER.exception()to preserve traceback context when logging caught exceptions.In exception handlers where the exception is caught, logged, and re-raised, use
LOGGER.exception()instead ofLOGGER.error()to automatically include the full traceback. This improves diagnostics without requiring manual exception formatting.Suggested fix
- except Exception as expt: - LOGGER.error(f"Failed to set up port forwarding for pod {model_registry_predictor_pod.name}: {expt}") + except Exception: + LOGGER.exception( + f"Failed to set up port forwarding for pod {model_registry_predictor_pod.name}" + ) raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/rest_api/conftest.py` around lines 739 - 741, Replace the LOGGER.error call in the exception handler that catches setup errors for port forwarding (the except Exception as expt: block referencing model_registry_predictor_pod.name) with LOGGER.exception so the traceback is preserved; keep the existing message text ("Failed to set up port forwarding for pod {model_registry_predictor_pod.name}: {expt}" or similar) but call LOGGER.exception(...) and then re-raise the exception as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_registry/model_registry/rest_api/conftest.py`:
- Around line 486-487: The test currently silently defaults model_uri to
"hf://jonburdo/test2" when register_model_data lacks "external_id"—change this
to fail fast instead: fetch register_model_data =
registered_model_rest_api.get("register_model", {}) and require model_uri =
register_model_data.get("external_id") (no hardcoded default), then raise an
explicit error or assertion (e.g., ValueError or assert) if model_uri is None or
empty so the test fails when upstream registration didn't provide an
external_id; apply the same change to the other occurrences referenced by the
comment (the model_uri lookups around the later block at the noted lines).
- Around line 724-727: Replace the hard-coded local_port=9998 with an ephemeral
free port allocation: open a temporary socket, bind to ('', 0) to obtain a free
port (use sock.getsockname()[1]), close the socket, assign that value to
local_port and rebuild local_url = f"http://localhost:{local_port}/v1/models";
ensure any port-forward logic (where remote_port=8888 and local_port is used)
uses this dynamically acquired local_port so parallel test runs won't collide.
---
Nitpick comments:
In `@tests/model_registry/model_registry/rest_api/conftest.py`:
- Around line 739-741: Replace the LOGGER.error call in the exception handler
that catches setup errors for port forwarding (the except Exception as expt:
block referencing model_registry_predictor_pod.name) with LOGGER.exception so
the traceback is preserved; keep the existing message text ("Failed to set up
port forwarding for pod {model_registry_predictor_pod.name}: {expt}" or similar)
but call LOGGER.exception(...) and then re-raise the exception as before.
In
`@tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py`:
- Around line 369-371: Remove the unused fixture arguments from the test
function signature: drop admin_client, model_registry_deployment_ns, and
model_registry_serving_runtime from the function parameter list so the test no
longer declares unused fixtures; the class-level `@pytest.mark.usefixtures`
already provides setup for model_registry_deployment_ns and
model_registry_serving_runtime, so keep any other required fixtures but ensure
the test function only lists fixtures it actually references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1237ea60-9849-4c3c-88cb-63a1209002db
📒 Files selected for processing (3)
tests/model_registry/model_registry/rest_api/conftest.pytests/model_registry/model_registry/rest_api/constants.pytests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py
fege
reviewed
Mar 5, 2026
fege
reviewed
Mar 5, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py (2)
390-395: Make endpoint assertion resilient to transient startup timing.A single GET can intermittently fail in e2e environments even when the deployment becomes healthy seconds later. Use a bounded poll before final assertion.
🛠️ Suggested reliability improvement
+import time import requests @@ - model_response = requests.get(model_endpoint, timeout=10) - LOGGER.info(f"Model endpoint status: {model_response.status_code}") - - assert model_response.status_code == 200, ( - f"Model endpoint returned status code:{model_response.status_code}: response text{model_response.text}" - ) + deadline = time.monotonic() + 60 + last_error: str | None = None + model_response = None + + while time.monotonic() < deadline: + try: + model_response = requests.get(model_endpoint, timeout=10) + LOGGER.info(f"Model endpoint status: {model_response.status_code}") + if model_response.status_code == 200: + break + last_error = f"status={model_response.status_code}, body={model_response.text}" + except requests.RequestException as exc: + last_error = str(exc) + time.sleep(2) + + assert model_response is not None and model_response.status_code == 200, ( + f"Model endpoint did not become ready within timeout. Last error: {last_error}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py` around lines 390 - 395, The test does a single requests.get to model_endpoint (model_response) which can transiently fail; replace the single GET with a bounded polling loop in test_model_registry_rest_api.py that retries requests.get(model_endpoint, timeout=10) until status_code == 200 or a total wait (e.g., 30–60s) is exceeded, sleeping a short interval between attempts; log each attempt via LOGGER and after the loop assert the final model_response.status_code == 200 (including model_response.text in the assertion message) so the test is resilient to startup timing.
369-371: Remove unused test parameters to clear ARG002 noise.These parameters are not referenced in the test body, so they add lint noise without improving coverage.
♻️ Proposed cleanup
def test_registered_model_deployment( self, - admin_client: DynamicClient, - model_registry_deployment_ns: Namespace, - model_registry_serving_runtime: ServingRuntime, model_registry_inference_service: InferenceService, model_registry_model_portforward: str, registered_model_rest_api: dict[str, Any], ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py` around lines 369 - 371, This test function is declaring unused parameters (admin_client, model_registry_deployment_ns, model_registry_serving_runtime) which trigger ARG002; remove these unused fixture parameters from the test function signature in tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py so the test body no longer declares them, leaving only the fixtures actually referenced in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py`:
- Around line 390-395: The test does a single requests.get to model_endpoint
(model_response) which can transiently fail; replace the single GET with a
bounded polling loop in test_model_registry_rest_api.py that retries
requests.get(model_endpoint, timeout=10) until status_code == 200 or a total
wait (e.g., 30–60s) is exceeded, sleeping a short interval between attempts; log
each attempt via LOGGER and after the loop assert the final
model_response.status_code == 200 (including model_response.text in the
assertion message) so the test is resilient to startup timing.
- Around line 369-371: This test function is declaring unused parameters
(admin_client, model_registry_deployment_ns, model_registry_serving_runtime)
which trigger ARG002; remove these unused fixture parameters from the test
function signature in
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py so
the test body no longer declares them, leaving only the fixtures actually
referenced in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d850fe3-3fa8-4df8-b072-6a7a30d127aa
📒 Files selected for processing (1)
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py
|
Status of building tag latest: success. |
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.
Pull Request
Summary
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit
Tests
Chores