feat(evalhub): Update EvalHub provider tests and add RBAC fixtures#1178
Closed
ruivieira wants to merge 24 commits intoopendatahub-io:mainfrom
Closed
feat(evalhub): Update EvalHub provider tests and add RBAC fixtures#1178ruivieira wants to merge 24 commits intoopendatahub-io:mainfrom
ruivieira wants to merge 24 commits intoopendatahub-io:mainfrom
Conversation
- Introduced new fixtures for ServiceAccounts and RoleBindings to facilitate testing of EvalHub multi-tenancy. - Added tests to verify the listing and retrieval of providers, ensuring proper response structure and required fields. - Implemented health check and utility functions for interacting with the EvalHub API. - Defined constants for RBAC roles for access management in tests.
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/wip', '/cherry-pick', '/lgtm', '/hold', '/verified'} |
# Conflicts: # tests/model_explainability/evalhub/conftest.py
…troller for secret retrieval - Updated `create_ca_bundle_file` to fetch the router certificate secret name from the default IngressController. - Removed the hardcoded secret name. - Changed the `evalhub_ca_bundle_file` function to work with the new CA bundle creation logic.
for more information, see https://pre-commit.ci
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds EvalHub provider API end-to-end tests, RBAC ServiceAccount fixtures with short-lived tokens, tenant-aware EvalHub client helpers, and ensures IngressController exists when resolving router TLS secret. Token creation via shell introduces command‑injection and secret‑leak risks (CWE-78, CWE-532). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/model_explainability/evalhub/utils.py (1)
129-129: Consider URL-encodingprovider_idfor defense in depth.Direct string interpolation of
provider_idinto the URL path could allow path traversal if input is not sanitized. While test code typically uses controlled inputs, URL-encoding provides defense in depth.Proposed fix
+from urllib.parse import quote + ... - url = f"https://{host}{EVALHUB_PROVIDERS_PATH}/{provider_id}" + url = f"https://{host}{EVALHUB_PROVIDERS_PATH}/{quote(provider_id, safe='')}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_explainability/evalhub/utils.py` at line 129, The URL currently interpolates provider_id directly into url (url = f"https://{host}{EVALHUB_PROVIDERS_PATH}/{provider_id}"); to harden this, URL-encode provider_id before composing the URL (use urllib.parse.quote with safe='' or similar) and then build the url using the encoded value so path-traversal characters are percent-encoded; update the code that sets url to reference the encoded_provider_id variable instead of raw provider_id.
🤖 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_explainability/evalhub/test_evalhub_providers.py`:
- Line 162: The test accesses providers["items"][0"] without checking for an
empty list, which can raise IndexError; update the test around the
first_provider_id assignment to first verify providers["items"] is non-empty
(e.g., assert providers.get("items") and len(providers["items"]) > 0) or call
pytest.skip when no providers are present, then only set first_provider_id =
providers["items"][0]["resource"]["id"]; reference the variables providers,
providers["items"], and first_provider_id when making the guard.
In `@utilities/certificates_utils.py`:
- Around line 27-33: The code assumes IngressController("default") exists and
accesses ingress_controller.instance.spec which can raise AttributeError; update
the creation or access to guard existence by either instantiating
IngressController with ensure_exists=True (or the equivalent constructor flag)
or check ingress_controller.instance is not None before accessing .spec, and
handle the missing resource by logging or raising a clear error; modify the
lines that create ingress_controller and the subsequent access to default_cert
(variables: IngressController, ingress_controller, ingress_controller.instance,
default_cert) to include this existence check or ensure_exists parameter and an
explicit fallback/error path.
---
Nitpick comments:
In `@tests/model_explainability/evalhub/utils.py`:
- Line 129: The URL currently interpolates provider_id directly into url (url =
f"https://{host}{EVALHUB_PROVIDERS_PATH}/{provider_id}"); to harden this,
URL-encode provider_id before composing the URL (use urllib.parse.quote with
safe='' or similar) and then build the url using the encoded value so
path-traversal characters are percent-encoded; update the code that sets url to
reference the encoded_provider_id variable instead of raw provider_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0e04a8f0-65e7-456b-9204-82414e8f0be6
📒 Files selected for processing (5)
tests/model_explainability/evalhub/conftest.pytests/model_explainability/evalhub/constants.pytests/model_explainability/evalhub/test_evalhub_providers.pytests/model_explainability/evalhub/utils.pyutilities/certificates_utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_explainability/evalhub/test_evalhub_providers.py (1)
148-155:⚠️ Potential issue | 🟡 MinorGuard
itemsbefore Line 154.If the environment returns no providers, Line 154 raises
IndexErrorand the test never exercisesget_evalhub_provider. Add a local precondition assertion orpytest.skipinstead of relying on another test to establish it.Suggested fix
providers = list_evalhub_providers( host=evalhub_route.host, token=evalhub_scoped_token, ca_bundle_file=evalhub_ca_bundle_file, tenant=model_namespace.name, ) + assert providers.get("items"), "No providers available to test get_single_provider" first_provider_id = providers["items"][0]["resource"]["id"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_explainability/evalhub/test_evalhub_providers.py` around lines 148 - 155, The test currently assumes list_evalhub_providers() returns items and then indexes providers["items"][0] which will IndexError if no providers exist; modify the test around the call to list_evalhub_providers (and before computing first_provider_id) to guard providers["items"] by asserting its truthiness or calling pytest.skip() when empty so the test short-circuits instead of failing—apply this check right after list_evalhub_providers(...) and before using first_provider_id and get_evalhub_provider to ensure the rest of the test only runs when providers are present.
🧹 Nitpick comments (1)
tests/model_explainability/evalhub/test_evalhub_providers.py (1)
22-24: Apply the RBAC fixture once at class scope.
evalhub_providers_role_bindingis never read inside these tests; it is only a setup side effect. Put@pytest.mark.usefixtures("evalhub_providers_role_binding")onTestEvalHubProvidersand drop the repeated parameters to remove the ARG002 noise and make the dependency explicit.Refactor sketch
import pytest import requests from ocp_resources.namespace import Namespace -from ocp_resources.role_binding import RoleBinding from ocp_resources.route import Route @@ `@pytest.mark.sanity` `@pytest.mark.model_explainability` +@pytest.mark.usefixtures("evalhub_providers_role_binding") class TestEvalHubProviders: @@ - evalhub_providers_role_binding: RoleBinding, evalhub_ca_bundle_file: str, evalhub_route: Route,Also applies to: 31-31, 52-52, 71-71, 94-94, 117-117, 143-143, 172-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_explainability/evalhub/test_evalhub_providers.py` around lines 22 - 24, The tests currently accept the evalhub_providers_role_binding fixture as a parameter but never use it; instead, add `@pytest.mark.usefixtures`("evalhub_providers_role_binding") to the TestEvalHubProviders class to apply the RBAC setup once at class scope and remove the repeated evalhub_providers_role_binding parameters from the individual test functions (this removes ARG002 noise); apply the same change for other test classes/functions that currently accept evalhub_providers_role_binding as an unused parameter.
🤖 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_explainability/evalhub/test_evalhub_providers.py`:
- Around line 177-184: Update the test that calls get_evalhub_provider to
explicitly assert a 404 response: wrap the call in a try/except (or use
pytest.raises as excinfo) capturing requests.exceptions.HTTPError raised by
utils.py:138, then inspect excinfo.value.response.status_code (or the caught
exception.response.status_code) and assert it equals 404; keep the existing
parameters and provider_id="nonexistent-provider-id" and
tenant=model_namespace.name so the test verifies the 404 contract instead of any
non-2xx error.
---
Duplicate comments:
In `@tests/model_explainability/evalhub/test_evalhub_providers.py`:
- Around line 148-155: The test currently assumes list_evalhub_providers()
returns items and then indexes providers["items"][0] which will IndexError if no
providers exist; modify the test around the call to list_evalhub_providers (and
before computing first_provider_id) to guard providers["items"] by asserting its
truthiness or calling pytest.skip() when empty so the test short-circuits
instead of failing—apply this check right after list_evalhub_providers(...) and
before using first_provider_id and get_evalhub_provider to ensure the rest of
the test only runs when providers are present.
---
Nitpick comments:
In `@tests/model_explainability/evalhub/test_evalhub_providers.py`:
- Around line 22-24: The tests currently accept the
evalhub_providers_role_binding fixture as a parameter but never use it; instead,
add `@pytest.mark.usefixtures`("evalhub_providers_role_binding") to the
TestEvalHubProviders class to apply the RBAC setup once at class scope and
remove the repeated evalhub_providers_role_binding parameters from the
individual test functions (this removes ARG002 noise); apply the same change for
other test classes/functions that currently accept
evalhub_providers_role_binding as an unused parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: fdff9954-ba41-4a20-b156-dd9584955232
📒 Files selected for processing (1)
tests/model_explainability/evalhub/test_evalhub_providers.py
Member
Author
Contributor
@ruivieira Yes, #1181 got merged, We can rebase this one and get it merge, Thanks! |
… refactor tests to use it
…b-collections # Conflicts: # tests/model_explainability/evalhub/test_evalhub_providers.py
for more information, see https://pre-commit.ci
…isting permissions
mariusdanciu
approved these changes
Mar 9, 2026
mariusdanciu
left a comment
mariusdanciu
left a comment
There was a problem hiding this comment.
lgtm.
A few suggestions if I may (for the future):
- Have a separate test file for authn and authz. Including trying to tamper with the /events endpoint while protected by status-events access.
- Have a separate test files for evaluation jobs
- Have a separate test files for providers
- Have a separate test files for collections
dbasunag
reviewed
Mar 9, 2026
| provider | ||
| for provider in evalhub_providers_response["items"] | ||
| if provider["resource"]["id"] == "lm_evaluation_harness" | ||
| ) |
Collaborator
There was a problem hiding this comment.
Any next() call should be used with an explicit default. We are trying to move to python 3.14 and they tightened this in 3.14
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_explainability/evalhub/test_evalhub_providers.py (1)
153-159:⚠️ Potential issue | 🟡 MinorAssert the 403 contract on
response.status_code, not the exception text.
match="403"only checks the rendered message. These tests should capture theHTTPErrorand assertexcinfo.value.response.status_code == 403, the same way the 404 path is already pinned.Suggested change
- with pytest.raises(requests.exceptions.HTTPError, match="403"): + with pytest.raises(requests.exceptions.HTTPError) as excinfo: list_evalhub_providers( host=evalhub_route.host, token=evalhub_unauthorised_token, ca_bundle_file=evalhub_ca_bundle_file, tenant=model_namespace.name, ) + assert excinfo.value.response is not None + assert excinfo.value.response.status_code == 403 @@ - with pytest.raises(requests.exceptions.HTTPError, match="403"): + with pytest.raises(requests.exceptions.HTTPError) as excinfo: get_evalhub_provider( host=evalhub_route.host, token=evalhub_unauthorised_token, ca_bundle_file=evalhub_ca_bundle_file, provider_id=provider_id, tenant=model_namespace.name, ) + assert excinfo.value.response is not None + assert excinfo.value.response.status_code == 403In pytest 9, does `pytest.raises(..., match="403")` only match the exception message, and when `requests.Response.raise_for_status()` raises `requests.exceptions.HTTPError`, is the originating `response.status_code` available on `exception.response`?Also applies to: 171-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_explainability/evalhub/test_evalhub_providers.py` around lines 153 - 159, The test should assert the HTTP status via the exception's response, not by matching the exception text: replace the pytest.raises(..., match="403") usage around the call to list_evalhub_providers (and the similar block at lines 171-178) with a context manager that captures the exception (e.g., using "with pytest.raises(requests.exceptions.HTTPError) as excinfo:") and then assert excinfo.value.response.status_code == 403; keep the same call parameters (host, token, ca_bundle_file, tenant) so the test verifies the HTTP contract rather than the rendered message.
🧹 Nitpick comments (1)
tests/model_explainability/evalhub/conftest.py (1)
78-90: Drop the unusedevalhub_deploymentdependency from these ServiceAccount fixtures.These fixtures do not read
evalhub_deployment, so they force SA/token setup to wait on EvalHub rollout for no effect. That adds avoidable setup latency and couples RBAC fixtures to deployment readiness.Suggested change
def evalhub_scoped_sa( admin_client: DynamicClient, model_namespace: Namespace, - evalhub_deployment: Deployment, ) -> Generator[ServiceAccount, Any, Any]: @@ def evalhub_unauthorised_sa( admin_client: DynamicClient, model_namespace: Namespace, - evalhub_deployment: Deployment, ) -> Generator[ServiceAccount, Any, Any]:Also applies to: 140-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_explainability/evalhub/conftest.py` around lines 78 - 90, The fixture evalhub_scoped_sa currently depends on evalhub_deployment but never uses it, causing unnecessary test setup latency; remove the unused parameter from the fixture signature (delete evalhub_deployment from the argument list of the evalhub_scoped_sa function) and update any other ServiceAccount fixtures that similarly list evalhub_deployment (e.g., the other fixture around lines 140-152) to drop that dependency so SA/token setup no longer waits on EvalHub rollout; keep the ServiceAccount context manager code (ServiceAccount(...)) and yield behavior unchanged.
🤖 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_explainability/evalhub/test_evalhub_providers.py`:
- Around line 167-170: Guard the provider lookup in the test by verifying
evalhub_providers_response["items"] is non-empty before indexing; if empty, skip
the test (or assert a meaningful failure) so an IndexError cannot occur.
Specifically update the section that computes provider_id from
evalhub_providers_response (the line using
evalhub_providers_response["items"][0]["resource"]["id"]) to first check
len(evalhub_providers_response.get("items", [])) > 0 (or use a test skip) and
only then extract provider_id and continue to the 403 assertion.
---
Duplicate comments:
In `@tests/model_explainability/evalhub/test_evalhub_providers.py`:
- Around line 153-159: The test should assert the HTTP status via the
exception's response, not by matching the exception text: replace the
pytest.raises(..., match="403") usage around the call to list_evalhub_providers
(and the similar block at lines 171-178) with a context manager that captures
the exception (e.g., using "with pytest.raises(requests.exceptions.HTTPError) as
excinfo:") and then assert excinfo.value.response.status_code == 403; keep the
same call parameters (host, token, ca_bundle_file, tenant) so the test verifies
the HTTP contract rather than the rendered message.
---
Nitpick comments:
In `@tests/model_explainability/evalhub/conftest.py`:
- Around line 78-90: The fixture evalhub_scoped_sa currently depends on
evalhub_deployment but never uses it, causing unnecessary test setup latency;
remove the unused parameter from the fixture signature (delete
evalhub_deployment from the argument list of the evalhub_scoped_sa function) and
update any other ServiceAccount fixtures that similarly list evalhub_deployment
(e.g., the other fixture around lines 140-152) to drop that dependency so
SA/token setup no longer waits on EvalHub rollout; keep the ServiceAccount
context manager code (ServiceAccount(...)) and yield behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0dbee5e2-c14e-48f2-9d8a-978a3650306f
📒 Files selected for processing (2)
tests/model_explainability/evalhub/conftest.pytests/model_explainability/evalhub/test_evalhub_providers.py
Member
Author
|
Closing this as there is a new set of tests for multi-tenant EvalHub that will replace the ones in this PR. |
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
6 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
How it has been tested
Additional Requirements
Summary by CodeRabbit
New Features
Tests