LCORE-1352: integration tests for /authorized endpoint#1195
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughIntroduces a new integration test module for the /authorized endpoint. Adds a fixture to mock AsyncLlamaStackClientHolder with a configured version, and an async test that validates the authorized endpoint returns correct user identity and configuration response fields. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
tisnik
force-pushed
the
lcore-1352-integration-tests-for-authorized-endpoint
branch
from
a835252 to
49c2370
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/endpoints/test_authorized_endpoint.py (2)
15-40: Remove the deadmock_llama_stack_clientfixture — wrong module patched, never invokedThree separate problems make this fixture dead code:
- Wrong patch target — Line 30 patches
"app.endpoints.info.AsyncLlamaStackClientHolder"(the info endpoint's module). The test is for the authorized endpoint.- Handler has no such dependency —
authorized_endpoint_handler(seesrc/app/endpoints/authorized.py:24-42) never referencesAsyncLlamaStackClientHolder; it simply unpacks the injectedauthtuple.- Fixture is not injected —
mock_llama_stack_clientis absent fromtest_authorized_endpoint's parameter list, so pytest never invokes it.This looks like an unmodified copy-paste from the info-endpoint test. Remove the fixture entirely; the test is already self-contained.
Removing the fixture also cleans up the orphaned imports on lines 3 and 7 (
Generator,Any,VersionInfo).🗑️ Proposed cleanup
-from typing import Generator, Any -import pytest -from pytest_mock import MockerFixture - -from llama_stack_client.types import VersionInfo -from authentication.interface import AuthTuple +import pytest +from authentication.interface import AuthTuple from configuration import AppConfig from app.endpoints.authorized import authorized_endpoint_handler from constants import DEFAULT_USER_UID, DEFAULT_USER_NAME, DEFAULT_SKIP_USER_ID_CHECK - - -@pytest.fixture(name="mock_llama_stack_client") -def mock_llama_stack_client_fixture( - mocker: MockerFixture, -) -> Generator[Any, None, None]: - ... - mock_holder_class = mocker.patch("app.endpoints.info.AsyncLlamaStackClientHolder") - mock_client = mocker.AsyncMock() - mock_client.inspect.version.return_value = VersionInfo(version="0.2.22") - mock_holder_instance = mock_holder_class.return_value - mock_holder_instance.get_client.return_value = mock_client - yield mock_client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/endpoints/test_authorized_endpoint.py` around lines 15 - 40, Delete the unused pytest fixture named mock_llama_stack_client from tests/integration/endpoints/test_authorized_endpoint.py because it patches the wrong target ("app.endpoints.info.AsyncLlamaStackClientHolder"), is never injected into the test (not present in test_authorized_endpoint parameters), and the authorized handler (authorized_endpoint_handler) does not depend on AsyncLlamaStackClientHolder; also remove the now-orphaned imports Generator, Any, and VersionInfo at the top of the file. Ensure no other tests in this file reference mock_llama_stack_client before removing it.
59-60: Minor: prefer discarding the fixture value directly in the signatureThe
_ = test_configworkaround exists because linters flag unused function parameters. A more idiomatic approach is to indicate the parameter is intentionally unused at the signature level:✨ Alternative
async def test_authorized_endpoint( - test_config: AppConfig, + test_config: AppConfig, # noqa: ARG001 (side-effect only: loads app config) test_auth: AuthTuple, ) -> None: ... - # Fixtures with side effects (needed but not directly used) - _ = test_config - response = await authorized_endpoint_handler(auth=test_auth)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/endpoints/test_authorized_endpoint.py` around lines 59 - 60, Replace the runtime discard "_ = test_config" with an unused-parameter indicator in the test signature: remove the assignment and rename the fixture parameter to start with an underscore (e.g., change parameter test_config to _test_config) or use pytest.mark.usefixtures("test_config") on the test function; update the test function signature where test_config is declared and remove the "_ = test_config" line so linters no longer flag an unused variable.
🤖 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/integration/endpoints/test_authorized_endpoint.py`:
- Around line 15-40: Delete the unused pytest fixture named
mock_llama_stack_client from
tests/integration/endpoints/test_authorized_endpoint.py because it patches the
wrong target ("app.endpoints.info.AsyncLlamaStackClientHolder"), is never
injected into the test (not present in test_authorized_endpoint parameters), and
the authorized handler (authorized_endpoint_handler) does not depend on
AsyncLlamaStackClientHolder; also remove the now-orphaned imports Generator,
Any, and VersionInfo at the top of the file. Ensure no other tests in this file
reference mock_llama_stack_client before removing it.
- Around line 59-60: Replace the runtime discard "_ = test_config" with an
unused-parameter indicator in the test signature: remove the assignment and
rename the fixture parameter to start with an underscore (e.g., change parameter
test_config to _test_config) or use pytest.mark.usefixtures("test_config") on
the test function; update the test function signature where test_config is
declared and remove the "_ = test_config" line so linters no longer flag an
unused variable.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Description
LCORE-1352: integration tests for
/authorizedendpointType of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit