test(maas): update auth enforcement tests for API key flow#1197
Merged
dbasunag merged 2 commits intoopendatahub-io:mainfrom Mar 10, 2026
Merged
test(maas): update auth enforcement tests for API key flow#1197dbasunag merged 2 commits intoopendatahub-io:mainfrom
dbasunag merged 2 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{'/hold', '/cherry-pick', '/wip', '/build-push-pr-image', '/verified', '/lgtm'} |
dbasunag
approved these changes
Mar 10, 2026
Contributor
📝 WalkthroughWalkthroughThis pull request refactors authentication enforcement tests to standardize HTTP response polling using a common utility function, replaces authentication parameter construction with a dedicated helper, updates test markers from sanity to smoke, and modifies the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (3)
tests/model_serving/model_server/maas_billing/maas_subscription/test_maas_auth_enforcement.py (3)
78-86: Inconsistent HTTP call pattern.This test uses direct
request_session_http.post()while the other three tests usepoll_expected_status. If immediate failure is expected for invalid tokens (no retry needed), this is fine. Otherwise, consider usingpoll_expected_statusfor consistency with the rest of the class.♻️ Proposed change for consistency
- resp = request_session_http.post( - url=model_url_tinyllama_free, - headers=headers, - json=payload, - timeout=60, - ) - - LOGGER.info(f"test_invalid_token_gets_401 -> POST {model_url_tinyllama_free} returned {resp.status_code}") - assert resp.status_code in (401, 403), f"Expected 401 or 403, got {resp.status_code}: {(resp.text or '')[:200]}" + resp = poll_expected_status( + request_session_http=request_session_http, + model_url=model_url_tinyllama_free, + headers=headers, + payload=payload, + expected_statuses={401, 403}, + ) + + LOGGER.info(f"test_invalid_token_gets_401 -> POST {model_url_tinyllama_free} returned {resp.status_code}")🤖 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_maas_auth_enforcement.py` around lines 78 - 86, The test explicitly calls request_session_http.post(...) for model_url_tinyllama_free while other tests use poll_expected_status; make the call pattern consistent by replacing the direct request with poll_expected_status(...) (or vice versa if immediate no-retry behavior is intended) so all tests use the same helper; locate the call to request_session_http.post in this test and use the poll_expected_status helper with the same url, headers, payload, timeout and expected status set to (401, 403) (or document why a direct post is required and leave as-is).
93-93: Type annotation inconsistency.
maas_headers_for_wrong_group_sa: dictat line 93 vsmaas_headers_for_actor_api_key: dict[str, str]at line 36. Use consistent parameterized type hints.♻️ Fix type annotation
- maas_headers_for_wrong_group_sa: dict, + maas_headers_for_wrong_group_sa: dict[str, str],🤖 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_maas_auth_enforcement.py` at line 93, The test fixture parameter maas_headers_for_wrong_group_sa uses a bare dict type while maas_headers_for_actor_api_key uses dict[str, str]; update the annotation for maas_headers_for_wrong_group_sa to dict[str, str] to make type hints consistent across the test (locate the parameter in the test function or fixture definition and change its annotation to match maas_headers_for_actor_api_key).
40-49: Redundant assertion afterpoll_expected_status.
poll_expected_statusalready guarantees the returned response has a status code inexpected_statuses, otherwise it callspytest.fail(). The assertion at line 49 is therefore redundant.Same applies to tests at lines 67, 86, and 108.
♻️ Optional: Remove redundant assertion
resp = poll_expected_status( request_session_http=request_session_http, model_url=model_url_tinyllama_free, headers=maas_headers_for_actor_api_key, payload=payload, expected_statuses={200}, ) LOGGER.info(f"test_authorized_user_gets_200 -> POST {model_url_tinyllama_free} returned {resp.status_code}") - assert resp.status_code == 200, f"Expected 200, got {resp.status_code}: {resp.text[:200]}"🤖 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_maas_auth_enforcement.py` around lines 40 - 49, Remove the redundant explicit status-code assertions that follow poll_expected_status in the test functions (e.g., the assert resp.status_code == 200 after calling poll_expected_status). poll_expected_status already fails the test if the response status is not in expected_statuses, so delete those asserts in the tests where poll_expected_status is used (refer to uses of poll_expected_status and variables like resp, model_url_tinyllama_free, maas_headers_for_actor_api_key, and LOGGER); keep the logging lines and any other checks that are not duplicative of poll_expected_status's behavior.
🤖 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/model_server/maas_billing/maas_subscription/test_maas_auth_enforcement.py`:
- Around line 78-86: The test explicitly calls request_session_http.post(...)
for model_url_tinyllama_free while other tests use poll_expected_status; make
the call pattern consistent by replacing the direct request with
poll_expected_status(...) (or vice versa if immediate no-retry behavior is
intended) so all tests use the same helper; locate the call to
request_session_http.post in this test and use the poll_expected_status helper
with the same url, headers, payload, timeout and expected status set to (401,
403) (or document why a direct post is required and leave as-is).
- Line 93: The test fixture parameter maas_headers_for_wrong_group_sa uses a
bare dict type while maas_headers_for_actor_api_key uses dict[str, str]; update
the annotation for maas_headers_for_wrong_group_sa to dict[str, str] to make
type hints consistent across the test (locate the parameter in the test function
or fixture definition and change its annotation to match
maas_headers_for_actor_api_key).
- Around line 40-49: Remove the redundant explicit status-code assertions that
follow poll_expected_status in the test functions (e.g., the assert
resp.status_code == 200 after calling poll_expected_status).
poll_expected_status already fails the test if the response status is not in
expected_statuses, so delete those asserts in the tests where
poll_expected_status is used (refer to uses of poll_expected_status and
variables like resp, model_url_tinyllama_free, maas_headers_for_actor_api_key,
and LOGGER); keep the logging lines and any other checks that are not
duplicative of poll_expected_status's behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d1a6c3a7-3dcb-4b96-9362-d712bebb8bfb
📒 Files selected for processing (2)
tests/model_serving/model_server/maas_billing/maas_subscription/test_maas_auth_enforcement.pytests/model_serving/model_server/maas_billing/maas_subscription/utils.py
💤 Files with no reviewable changes (1)
- tests/model_serving/model_server/maas_billing/maas_subscription/utils.py
|
Status of building tag latest: success. |
mwaykole
approved these changes
Mar 11, 2026
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
5 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
Update MaaS auth enforcement tests to use API key flow
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit