test(maas): add multiple subscription selection tests for TinyLlama#1165
Merged
threcc merged 4 commits intoopendatahub-io:mainfrom Mar 4, 2026
Merged
test(maas): add multiple subscription selection tests for TinyLlama#1165threcc merged 4 commits intoopendatahub-io:mainfrom
threcc merged 4 commits intoopendatahub-io:mainfrom
Conversation
SB159
requested review from
Raghul-M,
brettmthompson,
mwaykole and
threcc
as code owners
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/hold', '/cherry-pick', '/lgtm', '/verified', '/build-push-pr-image'} |
for more information, see https://pre-commit.ci
Contributor
📝 WalkthroughWalkthroughAdds a new test module exercising multiple MAAS subscriptions per model and accompanying utilities to create subscriptions and poll endpoints; tests cover explicit subscription selection, priority selection, and service-account access restrictions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py (1)
56-66: Use unique resource names to reduce e2e flakiness in shared environments.Hardcoded names increase collision risk across retries/parallel runs and can cause intermittent
AlreadyExistsfailures. Prefer a per-test unique suffix for subscription/policy/service-account names.Suggested pattern
+import uuid ... +suffix = uuid.uuid4().hex[:6] ... - subscription_name="extra-subscription", + subscription_name=f"extra-subscription-{suffix}", ... - subscription_name="high-tier-subscription", + subscription_name=f"high-tier-subscription-{suffix}", ... - name="service-account-access-policy", + name=f"service-account-access-policy-{suffix}", ... - subscription_name="premium-subscription", + subscription_name=f"premium-subscription-{suffix}", ... - name=service_account_name, + name=f"{service_account_name}-{suffix}",Also applies to: 111-121, 167-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py` around lines 56 - 66, The test uses hardcoded resource names in calls to create_maas_subscription (e.g., subscription_name="extra-subscription", owner_group_name="nonexistent-group-xyz") which can collide in shared/e2e runs; update these calls to generate per-test unique names (for example append a short uuid, timestamp, or pytest node id) when constructing subscription_name, owner_group_name, and any policy/service-account names referenced in the same test (also update the other occurrences around the ranges noted). Locate the create_maas_subscription invocations and replace static literals with a name builder (e.g., f"extra-subscription-{unique_suffix}") so each test run produces distinct resources while keeping the rest of the call arguments and teardown logic 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_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py`:
- Around line 205-215: The test currently allows either 403 or 429 from
poll_expected_status which masks authorization failures; update the call to
poll_expected_status (in test_multiple_subscriptions_per_model.py) to expect
only {403} and change the subsequent assertion to assert response.status_code ==
403 (and keep the same error message context) so the test deterministically
verifies unauthorized subscription use rather than allowing throttling (429) to
satisfy the check.
---
Nitpick comments:
In
`@tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py`:
- Around line 56-66: The test uses hardcoded resource names in calls to
create_maas_subscription (e.g., subscription_name="extra-subscription",
owner_group_name="nonexistent-group-xyz") which can collide in shared/e2e runs;
update these calls to generate per-test unique names (for example append a short
uuid, timestamp, or pytest node id) when constructing subscription_name,
owner_group_name, and any policy/service-account names referenced in the same
test (also update the other occurrences around the ranges noted). Locate the
create_maas_subscription invocations and replace static literals with a name
builder (e.g., f"extra-subscription-{unique_suffix}") so each test run produces
distinct resources while keeping the rest of the call arguments and teardown
logic unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.pytests/model_serving/model_server/maas_billing/maas_subscription/utils.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py (1)
211-216:⚠️ Potential issue | 🟡 MinorDocument temporary dual-status assertion with a TODO.
Given Line 211/Line 214 intentionally allow both
403and429for now, add an inline TODO (ideally with a tracking issue) so this doesn’t remain permissive after behavior is finalized.Suggested update
response = poll_expected_status( request_session_http=request_session_http, model_url=model_url_tinyllama_free, headers=headers, payload=payload, + # TODO(<tracking-ticket>): tighten to {403} once denial status is finalized. expected_statuses={403, 429}, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py` around lines 211 - 216, The test currently allows both 403 and 429 for the same assertion (the assert on response.status_code and the expected_statuses={403, 429} call); add an inline TODO comment immediately above this assertion referencing a tracking issue (e.g. TODO: tighten behavior to 403 only once ISSUE-XXXX is resolved) so the temporary dual-status permissiveness is documented and revisited—place the comment next to the assert that checks response.status_code and/or the expected_statuses argument.
🧹 Nitpick comments (1)
tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py (1)
58-59: Use unique resource names to reduce CI collision risk.Static names (e.g.,
extra-subscription,premium-subscription,test-service-account) can conflict in shared or retried environments. Prefer per-test unique suffixes.Suggested pattern
+import uuid ... +suffix = uuid.uuid4().hex[:6] with create_maas_subscription( admin_client=admin_client, - subscription_name="extra-subscription", + subscription_name=f"extra-subscription-{suffix}", ... - subscription_name="high-tier-subscription", + subscription_name=f"high-tier-subscription-{suffix}", ... - name="service-account-access-policy", + name=f"service-account-access-policy-{suffix}", ... - subscription_name="premium-subscription", + subscription_name=f"premium-subscription-{suffix}", ... - service_account_name = "test-service-account" + service_account_name = f"test-service-account-{suffix}"Also applies to: 114-115, 171-172, 180-180, 192-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py` around lines 58 - 59, The test uses static resource names (e.g., the subscription_name values "extra-subscription", "premium-subscription" and service account or group names like "test-service-account") which cause CI collisions; update the test helper calls in this test to generate unique names by appending a per-test suffix (timestamp, short UUID or pytest nodeid) when creating subscriptions/owner groups/service accounts (where subscription_name and owner_group_name are passed) and apply the same pattern to the other occurrences mentioned (the other subscription_name and service/account strings in the file). Locate the places constructing subscriptions (calls that pass subscription_name="...") and replace the literal with a generated name variable (e.g., f"{base_name}-{unique_suffix}") so each test run uses unique resource names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py`:
- Around line 211-216: The test currently allows both 403 and 429 for the same
assertion (the assert on response.status_code and the expected_statuses={403,
429} call); add an inline TODO comment immediately above this assertion
referencing a tracking issue (e.g. TODO: tighten behavior to 403 only once
ISSUE-XXXX is resolved) so the temporary dual-status permissiveness is
documented and revisited—place the comment next to the assert that checks
response.status_code and/or the expected_statuses argument.
---
Nitpick comments:
In
`@tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py`:
- Around line 58-59: The test uses static resource names (e.g., the
subscription_name values "extra-subscription", "premium-subscription" and
service account or group names like "test-service-account") which cause CI
collisions; update the test helper calls in this test to generate unique names
by appending a per-test suffix (timestamp, short UUID or pytest nodeid) when
creating subscriptions/owner groups/service accounts (where subscription_name
and owner_group_name are passed) and apply the same pattern to the other
occurrences mentioned (the other subscription_name and service/account strings
in the file). Locate the places constructing subscriptions (calls that pass
subscription_name="...") and replace the literal with a generated name variable
(e.g., f"{base_name}-{unique_suffix}") so each test run uses unique resource
names.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Run ID: 634142ce-548f-4c57-955a-4a60832dea07
📒 Files selected for processing (1)
tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py
mwaykole
approved these changes
Mar 4, 2026
threcc
approved these changes
Mar 4, 2026
|
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
Adds tests to validate behavior when multiple MaaS subscriptions exist for the same model.
Tests verify that users can access the model when explicitly selecting a valid subscription and are denied when selecting a subscription they do not belong to.
How it has been tested
Additional Requirements
Summary by CodeRabbit