tests(maas): add multiple auth policy and cascade deletion tests#1220
Merged
dbasunag merged 6 commits intoopendatahub-io:mainfrom Mar 17, 2026
Merged
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/build-push-pr-image', '/wip', '/lgtm', '/verified', '/hold'} |
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 two new pytest modules and fixtures that test MaaS subscription and auth-policy behavior: cascade deletion/rebuild and OR-logic across multiple auth policies. Tests perform resource creation/deletion, readiness polling, and HTTP access assertions (expecting 200, 403, or 429). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (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)
📝 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 CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![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 (5)
tests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py (2)
209-215: Missing assertion after polling for 200.This
poll_expected_statuscall returns a response, but unlike other assertions in this file (lines 72-75, 129-132, 164-167, 227-230), there's no explicitassertverifying the status code. Add an assertion for consistency and clearer test failure messages.♻️ Proposed fix
- poll_expected_status( + response = poll_expected_status( request_session_http=request_session_http, model_url=model_url_tinyllama_premium, headers=explicit_headers, payload=payload, expected_statuses={200}, ) + assert response.status_code == 200, ( + f"Expected 200 with extra AuthPolicy, got {response.status_code}: " + f"{(response.text or '')[:200]}" + )🤖 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_multiple_auth_policies_per_model.py` around lines 209 - 215, The call to poll_expected_status using request_session_http, model_url_tinyllama_premium, explicit_headers, and payload currently ignores the returned response; capture its return (e.g., response = poll_expected_status(...)) and add an assertion checking the status code (e.g., assert response.status_code == 200 or assert response.status_code in {200}) to match the other tests (see earlier uses of poll_expected_status).
168-169: Extra blank line.PEP 8: Two blank lines between top-level definitions, one blank line between method-level logical sections. This is extraneous.
🧹 Proposed fix
f"{(baseline_resp.text or '')[:200]}" ) - suffix = generate_random_name()🤖 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_multiple_auth_policies_per_model.py` around lines 168 - 169, Remove the extraneous blank line flagged by PEP8 in tests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py: delete the extra empty row so there are exactly two blank lines between top-level definitions (or one blank line between logical sections inside functions/tests); locate the extra blank line between the surrounding top-level definitions or inside the affected test function and collapse it to the proper single or double blank-line spacing to satisfy PEP8.tests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.py (3)
146-166: Fixture restoration relies on accessing attributes of a deleted resource.Line 150 accesses
maas_subscription_tinyllama_free.namespaceafter the resource was deleted on line 120. While the Python object still holds the cached attribute values, this pattern is fragile. Consider capturingoriginal_namespacealongsideoriginal_nameandoriginal_prioritybefore deletion for clarity and defensive coding.♻️ Proposed fix
original_name = maas_subscription_tinyllama_free.name + original_namespace = maas_subscription_tinyllama_free.namespace original_priority = getattr(maas_subscription_tinyllama_free, "priority", 0) LOGGER.info("Deleting original subscription %s", original_name)Then use
original_namespacein the restoration block:with MaaSSubscription( client=admin_client, name=original_name, - namespace=maas_subscription_tinyllama_free.namespace, + namespace=original_namespace,🤖 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_cascade_deletion.py` around lines 146 - 166, The restore block is referencing maas_subscription_tinyllama_free.namespace after that resource was deleted; capture the namespace into a local variable (e.g., original_namespace) alongside original_name and original_priority before deletion and then use that captured original_namespace when constructing the MaaSSubscription in the finally block (referencing maas_subscription_tinyllama_free, original_name, original_priority, and restored_subscription to locate the code).
100-100: Remove commented-out fixture reference.Dead code. Remove to reduce noise.
🧹 Proposed fix
maas_free_group: str, maas_model_tinyllama_free, - # maas_auth_policy_tinyllama_free, model_url_tinyllama_free: str,🤖 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_cascade_deletion.py` at line 100, Remove the dead commented-out fixture reference "maas_auth_policy_tinyllama_free" from the test setup in test_cascade_deletion.py; locate the commented line (e.g., "# maas_auth_policy_tinyllama_free,") in the fixture list or parameterization and delete it so the test file no longer contains the unused commented-out fixture reference.
48-48: Remove commented-out code.Dead code artifact. Either use
maas_subscription_tinyllama_freefor something meaningful or remove the comment entirely.🧹 Proposed fix
- # _ = maas_subscription_tinyllama_free - with create_maas_subscription(🤖 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_cascade_deletion.py` at line 48, Remove the commented-out dead code line referencing maas_subscription_tinyllama_free in test_cascade_deletion.py; either delete the comment entirely or replace it with a meaningful assertion or fixture usage that exercises maas_subscription_tinyllama_free (e.g., assign it to a variable or call a helper/assertion). Locate the commented line containing "_ = maas_subscription_tinyllama_free" and remove it if unused, or implement the intended use of the maas_subscription_tinyllama_free fixture/function within the test.
🤖 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/test_cascade_deletion.py`:
- Around line 116-120: The test deletes the class-scoped fixture
maas_subscription_tinyllama_free by calling
maas_subscription_tinyllama_free.clean_up(wait=True) and if an exception occurs
before the restoration block the fixture remains deleted and other tests fail;
wrap the deletion and restoration with explicit try/except/finally around the
clean_up and restoration logic in TestCascadeDeletion so any exception during
deletion is caught and logged (use LOGGER.error with exception info) and ensure
the restoration step for maas_subscription_tinyllama_free is executed in the
finally block with its own try/except that logs failures to restore (so failures
to restore are visible but don’t silently break subsequent tests).
In
`@tests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py`:
- Line 217: The test double-deletes extra_auth_policy because it was created via
the context manager with teardown=True (extra_auth_policy) and then manually
calls extra_auth_policy.delete(wait=True); remove the explicit delete() call so
the context manager's __exit__ handles cleanup, or alternatively create
extra_auth_policy with teardown=False if you intend to delete manually—update
the code around the creation/usage of extra_auth_policy to use one cleanup
strategy consistently.
---
Nitpick comments:
In `@tests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.py`:
- Around line 146-166: The restore block is referencing
maas_subscription_tinyllama_free.namespace after that resource was deleted;
capture the namespace into a local variable (e.g., original_namespace) alongside
original_name and original_priority before deletion and then use that captured
original_namespace when constructing the MaaSSubscription in the finally block
(referencing maas_subscription_tinyllama_free, original_name, original_priority,
and restored_subscription to locate the code).
- Line 100: Remove the dead commented-out fixture reference
"maas_auth_policy_tinyllama_free" from the test setup in
test_cascade_deletion.py; locate the commented line (e.g., "#
maas_auth_policy_tinyllama_free,") in the fixture list or parameterization and
delete it so the test file no longer contains the unused commented-out fixture
reference.
- Line 48: Remove the commented-out dead code line referencing
maas_subscription_tinyllama_free in test_cascade_deletion.py; either delete the
comment entirely or replace it with a meaningful assertion or fixture usage that
exercises maas_subscription_tinyllama_free (e.g., assign it to a variable or
call a helper/assertion). Locate the commented line containing "_ =
maas_subscription_tinyllama_free" and remove it if unused, or implement the
intended use of the maas_subscription_tinyllama_free fixture/function within the
test.
In
`@tests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py`:
- Around line 209-215: The call to poll_expected_status using
request_session_http, model_url_tinyllama_premium, explicit_headers, and payload
currently ignores the returned response; capture its return (e.g., response =
poll_expected_status(...)) and add an assertion checking the status code (e.g.,
assert response.status_code == 200 or assert response.status_code in {200}) to
match the other tests (see earlier uses of poll_expected_status).
- Around line 168-169: Remove the extraneous blank line flagged by PEP8 in
tests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py:
delete the extra empty row so there are exactly two blank lines between
top-level definitions (or one blank line between logical sections inside
functions/tests); locate the extra blank line between the surrounding top-level
definitions or inside the affected test function and collapse it to the proper
single or double blank-line spacing to satisfy PEP8.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4747bdeb-f6ce-413c-88c1-6fbde185b81d
📒 Files selected for processing (2)
tests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.pytests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py (1)
208-214: Missing assertion after polling for 200.Unlike
test_two_auth_policies_or_logic_allows_access(lines 129-132), this test does not assert on the response after polling for status 200. Add an explicit assertion for consistency and clearer failure diagnostics if the intermediate state check fails.♻️ Proposed fix
- poll_expected_status( + response_200 = poll_expected_status( request_session_http=request_session_http, model_url=model_url_tinyllama_premium, headers=explicit_headers, payload=payload, expected_statuses={200}, ) + assert response_200.status_code == 200, ( + f"Expected 200 with extra AuthPolicy, got {response_200.status_code}: " + f"{(response_200.text or '')[:200]}" + )🤖 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_multiple_auth_policies_per_model.py` around lines 208 - 214, The poll_expected_status call in test_multiple_auth_policies_per_model does not assert the actual response after it returns 200; update the test to capture the return value from poll_expected_status (e.g., response = poll_expected_status(...)) and add an explicit assertion that response.status_code == 200 (and optionally assert response is not None or inspect response.json() for expected keys), so failures produce clear diagnostics; reference the poll_expected_status invocation in the test to locate and modify the call.
🤖 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_multiple_auth_policies_per_model.py`:
- Around line 208-214: The poll_expected_status call in
test_multiple_auth_policies_per_model does not assert the actual response after
it returns 200; update the test to capture the return value from
poll_expected_status (e.g., response = poll_expected_status(...)) and add an
explicit assertion that response.status_code == 200 (and optionally assert
response is not None or inspect response.json() for expected keys), so failures
produce clear diagnostics; reference the poll_expected_status invocation in the
test to locate and modify the call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 66e814fc-87f0-4925-b05b-da0edb4a168b
📒 Files selected for processing (2)
tests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.pytests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py (1)
208-214: Assert the pre-delete 200 state explicitly.Line 208 currently drops the response object, which weakens diagnostics if this step regresses. Capture and assert it before deletion.
Proposed fix
- poll_expected_status( + pre_delete_response = poll_expected_status( request_session_http=request_session_http, model_url=model_url_tinyllama_premium, headers=explicit_headers, payload=payload, expected_statuses={200}, ) + assert pre_delete_response.status_code == 200, ( + f"Expected 200 before deleting extra AuthPolicy, got {pre_delete_response.status_code}: " + f"{(pre_delete_response.text or '')[:200]}" + )As per coding guidelines, "REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".
🤖 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_multiple_auth_policies_per_model.py` around lines 208 - 214, The call to poll_expected_status currently discards the response making regressions hard to debug; assign its return to a variable (e.g., resp = poll_expected_status(...)) when calling poll_expected_status with request_session_http, model_url_tinyllama_premium, headers=explicit_headers, payload and expected_statuses={200}, then explicitly assert the response indicates success (for example check resp.status_code == 200 or resp.status in your test harness) before proceeding to deletion so the pre-delete 200 state is captured and reported.
🤖 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_multiple_auth_policies_per_model.py`:
- Around line 208-214: The call to poll_expected_status currently discards the
response making regressions hard to debug; assign its return to a variable
(e.g., resp = poll_expected_status(...)) when calling poll_expected_status with
request_session_http, model_url_tinyllama_premium, headers=explicit_headers,
payload and expected_statuses={200}, then explicitly assert the response
indicates success (for example check resp.status_code == 200 or resp.status in
your test harness) before proceeding to deletion so the pre-delete 200 state is
captured and reported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 47d95756-82a9-4399-93c8-8e0d145d74f6
📒 Files selected for processing (1)
tests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py
mwaykole
previously approved these changes
Mar 14, 2026
dbasunag
requested changes
Mar 14, 2026
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
for more information, see https://pre-commit.ci Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
SB159
force-pushed
the
feature/test-multiple-auth-policies-per-model
branch
from
41bdadb to
1eaae51
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.py (1)
94-95:⚠️ Potential issue | 🟠 MajorPut the destructive delete inside the restoration guard.
maas_subscription_tinyllama_free.clean_up(wait=True)at Line 95 runs beforetry/finally; if it throws, restoration never runs. That can leave class-scoped state broken for subsequent tests.Proposed fix
- LOGGER.info("Deleting original subscription %s", original_name) - maas_subscription_tinyllama_free.clean_up(wait=True) - payload = chat_payload_for_url(model_url=model_url_tinyllama_free) try: + LOGGER.info("Deleting original subscription %s", original_name) + maas_subscription_tinyllama_free.clean_up(wait=True) + response = poll_expected_status( request_session_http=request_session_http, model_url=model_url_tinyllama_free, @@ - finally: - with MaaSSubscription( - client=admin_client, - name=original_name, - namespace=maas_subscription_tinyllama_free.namespace, - owner={ - "groups": [{"name": maas_free_group}], - }, - model_refs=[ - { - "name": maas_model_tinyllama_free.name, - "namespace": maas_model_tinyllama_free.namespace, - "tokenRateLimits": [{"limit": 100, "window": "1m"}], - } - ], - priority=original_priority, - teardown=False, - wait_for_resource=True, - ) as restored_subscription: - restored_subscription.wait_for_condition( - condition="Ready", - status="True", - timeout=300, - ) - LOGGER.info("Restored original subscription %s", restored_subscription.name) + finally: + try: + with MaaSSubscription( + client=admin_client, + name=original_name, + namespace=maas_subscription_tinyllama_free.namespace, + owner={ + "groups": [{"name": maas_free_group}], + }, + model_refs=[ + { + "name": maas_model_tinyllama_free.name, + "namespace": maas_model_tinyllama_free.namespace, + "tokenRateLimits": [{"limit": 100, "window": "1m"}], + } + ], + priority=original_priority, + teardown=False, + wait_for_resource=True, + ) as restored_subscription: + restored_subscription.wait_for_condition( + condition="Ready", + status="True", + timeout=300, + ) + LOGGER.info("Restored original subscription %s", restored_subscription.name) + except Exception: + LOGGER.exception("Failed restoring original subscription %s", original_name) + raiseAs per coding guidelines,
**: REVIEW PRIORITIES:3. Bug-prone patterns and error handling gaps.Also applies to: 99-145
🤖 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_cascade_deletion.py` around lines 94 - 95, The call to maas_subscription_tinyllama_free.clean_up(wait=True) is executed outside the restoration guard and can raise, preventing the finally/restoration code from running; move the destructive delete into the try/finally block (i.e., perform maas_subscription_tinyllama_free.clean_up(wait=True) inside the try or inside the finally after any safe checks) so that the restoration/teardown code always executes, and ensure any other destructive cleanup calls in the same test (lines ~99-145) follow the same pattern and reference the same objects (maas_subscription_tinyllama_free, original_name) to keep class-scoped state consistent for subsequent tests.
🤖 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 509-558: The setup creates MaaSAuthPolicy (extra_auth_policy) and
a subscription (system_authenticated_subscription) and then calls
wait_for_condition; wrap the post-creation logic in a try/finally so cleanup
always runs on failure: after creating extra_auth_policy and
system_authenticated_subscription (via MaaSAuthPolicy and
create_maas_subscription) enter try: call extra_auth_policy.wait_for_condition
and system_authenticated_subscription.wait_for_condition and yield the dict, and
in finally: if extra_auth_policy.exists: call
extra_auth_policy.clean_up(wait=True) (and similarly clean up
system_authenticated_subscription if needed), ensuring you reference the
existing symbols extra_auth_policy, system_authenticated_subscription,
wait_for_condition, and clean_up so resources are removed even when waits throw.
- Around line 466-493: The fixture's cleanup must be idempotent and always run:
wrap the setup/yield logic around create_maas_subscription and
temporary_subscription.wait_for_condition in a try/finally so cleanup runs even
if setup fails, and before calling temporary_subscription.clean_up(check) verify
the resource still exists by calling temporary_subscription.exists(); only call
clean_up(wait=True) when exists() returns True (mirroring the pattern used
elsewhere in this file). Ensure you keep the yield of temporary_subscription
inside the try so finally always executes and do not remove the teardown=False
argument to create_maas_subscription.
---
Duplicate comments:
In `@tests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.py`:
- Around line 94-95: The call to
maas_subscription_tinyllama_free.clean_up(wait=True) is executed outside the
restoration guard and can raise, preventing the finally/restoration code from
running; move the destructive delete into the try/finally block (i.e., perform
maas_subscription_tinyllama_free.clean_up(wait=True) inside the try or inside
the finally after any safe checks) so that the restoration/teardown code always
executes, and ensure any other destructive cleanup calls in the same test (lines
~99-145) follow the same pattern and reference the same objects
(maas_subscription_tinyllama_free, original_name) to keep class-scoped state
consistent for subsequent tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6fbf85f0-18e5-4aaa-a93b-217cf0b5ae0a
📒 Files selected for processing (3)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.pytests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.py (1)
91-95: Mutating class-scoped fixture risks test isolation.Deleting
maas_subscription_tinyllama_free(class-scoped, line 95) affects other tests inTestCascadeDeletionthat depend on it. If restoration in finally fails, subsequent tests will fail with misleading errors.Consider wrapping the restoration in its own try/except to log failures explicitly:
Proposed fix
finally: + try: with MaaSSubscription( client=admin_client, name=original_name, namespace=maas_subscription_tinyllama_free.namespace, owner={ "groups": [{"name": maas_free_group}], }, model_refs=[ { "name": maas_model_tinyllama_free.name, "namespace": maas_model_tinyllama_free.namespace, "tokenRateLimits": [{"limit": 100, "window": "1m"}], } ], priority=original_priority, teardown=False, wait_for_resource=True, ) as restored_subscription: restored_subscription.wait_for_condition( condition="Ready", status="True", timeout=300, ) LOGGER.info("Restored original subscription %s", restored_subscription.name) + except Exception as restore_err: + LOGGER.error( + "Failed to restore subscription %s: %s. Subsequent tests may fail.", + original_name, + restore_err, + ) + raise🤖 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_cascade_deletion.py` around lines 91 - 95, The test mutates the class-scoped fixture maas_subscription_tinyllama_free by calling maas_subscription_tinyllama_free.clean_up(wait=True), which can break other tests in TestCascadeDeletion; fix by creating and deleting a temporary subscription (or clone) for this test instead of operating on the class fixture, and if you must restore the fixture ensure the restoration is wrapped in its own try/except that catches exceptions from the restore logic and logs them (use LOGGER.error) so failures during restore do not propagate and break subsequent tests; refer to maas_subscription_tinyllama_free, clean_up(wait=True), original_name/original_priority and TestCascadeDeletion when making the changes.
🤖 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_cascade_deletion.py`:
- Around line 91-95: The test mutates the class-scoped fixture
maas_subscription_tinyllama_free by calling
maas_subscription_tinyllama_free.clean_up(wait=True), which can break other
tests in TestCascadeDeletion; fix by creating and deleting a temporary
subscription (or clone) for this test instead of operating on the class fixture,
and if you must restore the fixture ensure the restoration is wrapped in its own
try/except that catches exceptions from the restore logic and logs them (use
LOGGER.error) so failures during restore do not propagate and break subsequent
tests; refer to maas_subscription_tinyllama_free, clean_up(wait=True),
original_name/original_priority and TestCascadeDeletion when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3dcfa3ad-af12-44c4-8cdc-de93f72c883c
📒 Files selected for processing (3)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_cascade_deletion.pytests/model_serving/maas_billing/maas_subscription/test_multiple_auth_policies_per_model.py
dbasunag
requested changes
Mar 17, 2026
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
dbasunag
approved these changes
Mar 17, 2026
mwaykole
approved these changes
Mar 17, 2026
|
Status of building tag latest: success. |
ssaleem-rh
pushed a commit
to ssaleem-rh/opendatahub-tests
that referenced
this pull request
Mar 23, 2026
…ndatahub-io#1220) * tests(maas): add multiple auth policy and cascade deletion tests Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * tests(maas): fix address coderabit review comments Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * address review comments Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * address review comments Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> --------- Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com> Signed-off-by: Shehan Saleem <ssaleem@redhat.com>
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
add multiple auth policy and cascade deletion tests
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit