tests(maas): update test with maas_subscription_controller_enabled_la…#1219
Merged
dbasunag merged 3 commits intoopendatahub-io:mainfrom Mar 13, 2026
Merged
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/verified', '/wip', '/lgtm', '/build-push-pr-image', '/hold'} |
Contributor
📝 WalkthroughWalkthroughReplaced a test-class fixture reference across multiple MaaS subscription tests and introduced new session-scoped PostgreSQL-related fixtures and helper utilities to provision and wait for a MaaS Postgres deployment used by those tests. No test logic within individual test methods was changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ 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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py (1)
15-24: Add rate-limit policy fixtures to preserve 429-path test coverageThe fixture swap at line 17 removes
maas_request_ratelimit_policyandmaas_token_ratelimit_policythat were present in the previous setup. The test still expects 429 status codes (line 93), but without these fixtures, the throttling path may not be exercised—only 403 would be triggered.Suggested adjustment
`@pytest.mark.usefixtures`( "maas_unprivileged_model_namespace", "maas_subscription_controller_enabled_latest", + "maas_request_ratelimit_policy", + "maas_token_ratelimit_policy", "maas_gateway_api", "maas_api_gateway_reachable",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py` around lines 15 - 24, The test lost the rate-limit fixtures so the 429 throttling path isn't exercised; re-add the fixtures maas_request_ratelimit_policy and maas_token_ratelimit_policy to the pytest.mark.usefixtures decorator on the failing test (the one using maas_unprivileged_model_namespace, maas_subscription_controller_enabled_latest, maas_gateway_api, maas_api_gateway_reachable, maas_inference_service_tinyllama_premium, maas_model_tinyllama_premium, maas_auth_policy_tinyllama_premium, maas_subscription_tinyllama_premium) so the test triggers the expected 429 responses instead of only 403. Ensure the fixture names match exactly (maas_request_ratelimit_policy, maas_token_ratelimit_policy) and keep them active for the test run.
🤖 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_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py`:
- Around line 15-24: The test lost the rate-limit fixtures so the 429 throttling
path isn't exercised; re-add the fixtures maas_request_ratelimit_policy and
maas_token_ratelimit_policy to the pytest.mark.usefixtures decorator on the
failing test (the one using maas_unprivileged_model_namespace,
maas_subscription_controller_enabled_latest, maas_gateway_api,
maas_api_gateway_reachable, maas_inference_service_tinyllama_premium,
maas_model_tinyllama_premium, maas_auth_policy_tinyllama_premium,
maas_subscription_tinyllama_premium) so the test triggers the expected 429
responses instead of only 403. Ensure the fixture names match exactly
(maas_request_ratelimit_policy, maas_token_ratelimit_policy) and keep them
active for the test run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 625234f3-a651-45f6-8d59-e77d3cd76d15
📒 Files selected for processing (5)
tests/model_serving/maas_billing/maas_subscription/component_health/test_maas_api_health.pytests/model_serving/maas_billing/maas_subscription/component_health/test_maas_controller_health.pytests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.pytests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.pytests/model_serving/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py
dbasunag
previously approved these changes
Mar 13, 2026
…ahub-io#1213) * test: add maas postgres prereqs setup for subscription tests * address review comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * address review comment --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Thomas Recchiuto <34453570+threcc@users.noreply.github.com> Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
…test Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
SB159
force-pushed
the
feature/update-test-maas-subscription-controller
branch
from
297323b to
08030d4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/model_serving/maas_billing/maas_subscription/utils.py (1)
388-395: Weak return type annotation.
dict[Any, Any]loses type information. Consider a more descriptive type alias.Example
PostgresResources = dict[type[Secret] | type[Service] | type[Deployment], list[Secret | Service | Deployment]]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/utils.py` around lines 388 - 395, The return type for get_maas_postgres_resources is too generic (dict[Any, Any]); introduce a clear type alias (e.g., PostgresResources) that maps the specific Kubernetes resource classes to lists of their instances (for example using Union/type[...] of Secret, Service, Deployment) and update the function signature to return that alias; update any imports or typing (from typing import Type, Union, Dict, List) as needed and replace dict[Any, Any] with the new PostgresResources alias in get_maas_postgres_resources.tests/model_serving/maas_billing/maas_subscription/conftest.py (1)
46-72: Fixture duplicates existingmaas_controller_enabled_latestlogic.The DSC patching and wait logic is nearly identical to
tests/model_serving/maas_billing/conftest.py:705-735. The only difference is the dependency list. Consider whether this can be composed from the existing fixture or extracted to a shared helper (seeutilities/data_science_cluster_utils.py:update_components_in_dsc).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 46 - 72, The new fixture maas_subscription_controller_enabled_latest duplicates the patching and wait logic from maas_controller_enabled_latest; refactor by reusing or extracting the shared logic: either have maas_subscription_controller_enabled_latest call or depend on the existing maas_controller_enabled_latest fixture (remove duplicated ResourceEditor/DscComponents patch/wait blocks) or move the patch+wait sequence into utilities/data_science_cluster_utils.py:update_components_in_dsc and call that helper from both fixtures; ensure references to DscComponents.KSERVE, ResourceEditor, and DataScienceCluster.wait_for_condition remain correct and remove the redundant code block in maas_subscription_controller_enabled_latest.
🤖 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_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 377-385: Update the type hint for the
maas_subscription_controller_enabled_latest parameter in the
maas_api_key_for_actor fixture: it is currently annotated as None but actually
yields a DataScienceCluster, so change the annotation to the correct type
(DataScienceCluster) or to typing.Any if the value is intentionally unused;
ensure the parameter signature in maas_api_key_for_actor is updated accordingly.
- Around line 360-372: The code redundantly waits for the Postgres deployment
twice: the loop over resources_instances[Deployment] calling
deployment.wait_for_condition(condition="Available", ...) duplicates the logic
in wait_for_postgres_deployment_ready; remove the explicit deployment loop and
keep wait_for_postgres_deployment_ready (and wait_for_postgres_connection_log)
so readiness is checked once via wait_for_postgres_deployment_ready.
In `@tests/model_serving/maas_billing/maas_subscription/utils.py`:
- Around line 448-460: The current wait_for_postgres_connection_log uses
TimeoutSampler(func=lambda: True) which never signals completion, making the
POSTGRES_READY_LOG_TEXT check unreachable; change the sampler to poll a
condition function instead (e.g., func=lambda: POSTGRES_READY_LOG_TEXT in
get_postgres_pod_in_namespace(admin_client,
namespace).log(container="postgres")) or move the pod_log check into the
sampler's func so the sampler returns True when the ready text is present, then
keep the LOGGER.info and return when found; also handle the sampler timing out
by raising or logging a clear timeout error. Use the existing symbols
wait_for_postgres_connection_log, get_postgres_pod_in_namespace, and
POSTGRES_READY_LOG_TEXT to locate and update the logic.
---
Nitpick comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 46-72: The new fixture maas_subscription_controller_enabled_latest
duplicates the patching and wait logic from maas_controller_enabled_latest;
refactor by reusing or extracting the shared logic: either have
maas_subscription_controller_enabled_latest call or depend on the existing
maas_controller_enabled_latest fixture (remove duplicated
ResourceEditor/DscComponents patch/wait blocks) or move the patch+wait sequence
into utilities/data_science_cluster_utils.py:update_components_in_dsc and call
that helper from both fixtures; ensure references to DscComponents.KSERVE,
ResourceEditor, and DataScienceCluster.wait_for_condition remain correct and
remove the redundant code block in maas_subscription_controller_enabled_latest.
In `@tests/model_serving/maas_billing/maas_subscription/utils.py`:
- Around line 388-395: The return type for get_maas_postgres_resources is too
generic (dict[Any, Any]); introduce a clear type alias (e.g., PostgresResources)
that maps the specific Kubernetes resource classes to lists of their instances
(for example using Union/type[...] of Secret, Service, Deployment) and update
the function signature to return that alias; update any imports or typing (from
typing import Type, Union, Dict, List) as needed and replace dict[Any, Any] with
the new PostgresResources alias in get_maas_postgres_resources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1deb5c2a-71bf-4a29-9964-74f16135f587
📒 Files selected for processing (7)
tests/model_serving/maas_billing/maas_subscription/component_health/test_maas_api_health.pytests/model_serving/maas_billing/maas_subscription/component_health/test_maas_controller_health.pytests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.pytests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.pytests/model_serving/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.pytests/model_serving/maas_billing/maas_subscription/utils.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.py
- tests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py
- tests/model_serving/maas_billing/maas_subscription/component_health/test_maas_api_health.py
- tests/model_serving/maas_billing/maas_subscription/component_health/test_maas_controller_health.py
dbasunag
approved these changes
Mar 13, 2026
Collaborator
|
Merging based on @SB159 's request. |
|
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
3 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.
…test
Pull Request
update test with maas_subscription_controller_enabled_latest
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit