LCORE-1360: Integration tests for /root endpoint#1194
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a new integration test for the /root endpoint that mocks the AsyncLlamaStackClientHolder, configures a mocked version response, and verifies the handler returns an HTML response with HTTP 200 and expected title/header. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integration/endpoints/test_root_endpoint.py (1)
15-40: Remove the unusedmock_llama_stack_clientfixture — it's dead code that patches the wrong module.Two problems here:
- The patch target is
"app.endpoints.info.AsyncLlamaStackClientHolder", which belongs to the info endpoint module, not the root endpoint. This is almost certainly a copy-paste fromtest_info_endpoint.py.- The fixture is never injected into
test_root_endpoint(it's not listed as a parameter), so the patch is never applied when the test runs.Looking at the relevant snippet of
root_endpoint_handlerinsrc/app/endpoints/root.py(lines 788–807), the handler makes no external service calls — it simply returnsHTMLResponse(INDEX_PAGE). The fixture is therefore entirely unnecessary for this test file and should be removed along with its now-orphanedVersionInfoimport.♻️ Proposed cleanup
-from typing import Generator, Any +from typing import Any import pytest from pytest_mock import MockerFixture from fastapi import Request, status -from llama_stack_client.types import VersionInfo from authentication.interface import AuthTuple from configuration import AppConfig from app.endpoints.root import root_endpoint_handler - - -@pytest.fixture(name="mock_llama_stack_client") -def mock_llama_stack_client_fixture( - mocker: MockerFixture, -) -> Generator[Any, None, None]: - """Mock only the external Llama Stack client. - ... - """ - 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_root_endpoint.py` around lines 15 - 40, Remove the dead fixture mock_llama_stack_client and the unused VersionInfo import from the test file: delete the pytest fixture named mock_llama_stack_client (which patches AsyncLlamaStackClientHolder) and any references to VersionInfo, since root_endpoint_handler simply returns HTMLResponse(INDEX_PAGE) and the fixture is never injected into tests; this eliminates the incorrect patch target ("app.endpoints.info.AsyncLlamaStackClientHolder") and unused code in tests/integration/endpoints/test_root_endpoint.py.
🤖 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/integration/endpoints/test_root_endpoint.py`:
- Around line 68-70: Replace the incorrect bytes-to-string conversion using
str(response.body) with an explicit UTF-8 decode: change the assignment that
sets body (from response.body) to call response.body.decode("utf-8") so the test
works with the actual decoded HTML string; update the assertions that reference
body (the two checks for "<title>Lightspeed core service</title>" and
"<h1>Lightspeed core service</h1>") to use this decoded value.
- Around line 56-59: Update the stale docstring parameter name: replace the
documented parameter "current_config (AppConfig)" with the actual parameter name
"test_config (AppConfig)" in the test_root_endpoint.py docstring so the
Parameters section matches the function signature and the other documented
params (test_request, test_auth); locate the docstring that lists Parameters and
adjust the identifier only (leave the type and description unchanged).
---
Nitpick comments:
In `@tests/integration/endpoints/test_root_endpoint.py`:
- Around line 15-40: Remove the dead fixture mock_llama_stack_client and the
unused VersionInfo import from the test file: delete the pytest fixture named
mock_llama_stack_client (which patches AsyncLlamaStackClientHolder) and any
references to VersionInfo, since root_endpoint_handler simply returns
HTMLResponse(INDEX_PAGE) and the fixture is never injected into tests; this
eliminates the incorrect patch target
("app.endpoints.info.AsyncLlamaStackClientHolder") and unused code in
tests/integration/endpoints/test_root_endpoint.py.
3cb88dc to
e0bcf35
Compare
Description
LCORE-1360: Integration tests for
/rootendpointType of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit