LCORE-1126: updated docstrings in unit tests #930
Conversation
WalkthroughThis PR consists primarily of docstring expansions and minor documentation updates across test files. The functional changes are minimal: a test fixture adds authorized actions setup, test_rags.py expands mock data from 2 to 3 entries, a new test function is added for quota limiters, and test endpoint README is updated with new test file entry. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Notes for reviewers:
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/unit/app/endpoints/test_tools.py (2)
127-127: Fix incomplete comments.Several comments appear truncated and incomplete:
- Lines 127, 194, 230, 311, 367, 434, 514, 544, 581, 615: "Mock authorization decorator to bypass i" (should be "bypass auth" or similar)
- Lines 130, 197, 233, 314, 370, 437, 517, 547: "Mock client holder and clien" (should be "client holder and client")
These should be completed for clarity.
Also applies to: 130-130, 194-194, 197-197, 230-230, 233-233, 311-311, 314-314, 367-367, 370-370, 434-434, 437-437, 514-514, 517-517, 544-544, 547-547, 581-581, 615-615
529-529: Fix typos in comments.Four comments contain typos: "endpointt" should be "endpoint".
- Line 529: "Call the endpointt"
- Line 560: "Call the endpointt"
- Line 593: "Call the endpointt"
- Line 628: "Call the endpointt"
Also applies to: 560-560, 593-593, 628-628
tests/unit/app/endpoints/test_shields.py (2)
39-42: Assertions inside pytest.raises will never execute.The assertions on lines 41-42 are inside the
with pytest.raises()context manager, so they will never run. Whenshields_endpoint_handlerraisesHTTPException, the context manager immediately exits, skipping lines 41-42. Move the assertions outside thewithblock.Apply this diff to fix:
with pytest.raises(HTTPException) as e: await shields_endpoint_handler(request=request, auth=auth) - assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR - assert e.value.detail["response"] == "Configuration is not loaded" # type: ignore + assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert e.value.detail["response"] == "Configuration is not loaded" # type: ignore
302-306: Assertions inside pytest.raises will never execute.Lines 304-306 are indented inside the
with pytest.raises()block, so they never run. Onceshields_endpoint_handlerraises the exception on line 303, the context manager exits immediately. Move these assertions outside thewithblock.Apply this diff:
with pytest.raises(HTTPException) as e: await shields_endpoint_handler(request=request, auth=auth) - assert e.value.status_code == status.HTTP_503_SERVICE_UNAVAILABLE - assert e.value.detail["response"] == "Service unavailable" # type: ignore - assert "Unable to connect to Llama Stack" in e.value.detail["cause"] # type: ignore + assert e.value.status_code == status.HTTP_503_SERVICE_UNAVAILABLE + assert e.value.detail["response"] == "Service unavailable" # type: ignore + assert "Unable to connect to Llama Stack" in e.value.detail["cause"] # type: ignore
🧹 Nitpick comments (1)
tests/unit/models/config/test_dump_configuration.py (1)
3-4: Consider addressing the file length rather than disabling the warning.The
too-many-linespylint warning is disabled, likely due to the test duplication below. Consider refactoring duplicated test logic into parameterized tests or shared helper functions instead of suppressing the warning.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
tests/unit/app/endpoints/README.md(1 hunks)tests/unit/app/endpoints/test_authorized.py(1 hunks)tests/unit/app/endpoints/test_conversations.py(8 hunks)tests/unit/app/endpoints/test_conversations_v2.py(4 hunks)tests/unit/app/endpoints/test_health.py(1 hunks)tests/unit/app/endpoints/test_info.py(1 hunks)tests/unit/app/endpoints/test_models.py(1 hunks)tests/unit/app/endpoints/test_query.py(10 hunks)tests/unit/app/endpoints/test_query_v2.py(1 hunks)tests/unit/app/endpoints/test_rags.py(1 hunks)tests/unit/app/endpoints/test_shields.py(3 hunks)tests/unit/app/endpoints/test_streaming_query.py(20 hunks)tests/unit/app/endpoints/test_streaming_query_v2.py(5 hunks)tests/unit/app/endpoints/test_tools.py(1 hunks)tests/unit/app/test_database.py(4 hunks)tests/unit/app/test_routers.py(3 hunks)tests/unit/models/config/test_dump_configuration.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/app/endpoints/test_health.pytests/unit/app/endpoints/test_query_v2.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_info.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/endpoints/test_conversations_v2.pytests/unit/app/endpoints/test_conversations.pytests/unit/app/endpoints/test_tools.pytests/unit/app/endpoints/test_models.pytests/unit/app/test_database.pytests/unit/app/test_routers.pytests/unit/app/endpoints/test_rags.pytests/unit/app/endpoints/test_shields.pytests/unit/app/endpoints/test_authorized.pytests/unit/app/endpoints/test_query.pytests/unit/models/config/test_dump_configuration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/app/endpoints/test_health.pytests/unit/app/endpoints/test_query_v2.pytests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_info.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/endpoints/test_conversations_v2.pytests/unit/app/endpoints/test_conversations.pytests/unit/app/endpoints/test_tools.pytests/unit/app/endpoints/test_models.pytests/unit/app/test_database.pytests/unit/app/test_routers.pytests/unit/app/endpoints/test_rags.pytests/unit/app/endpoints/test_shields.pytests/unit/app/endpoints/test_authorized.pytests/unit/app/endpoints/test_query.pytests/unit/models/config/test_dump_configuration.py
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
tests/unit/app/endpoints/test_streaming_query_v2.pytests/unit/app/endpoints/test_info.pytests/unit/app/endpoints/test_streaming_query.pytests/unit/app/endpoints/test_shields.pytests/unit/app/endpoints/test_query.py
🧬 Code graph analysis (2)
tests/unit/app/endpoints/test_streaming_query.py (1)
tests/unit/app/endpoints/test_query.py (3)
MockShield(763-792)MockShield(838-866)MockShield(915-944)
tests/unit/models/config/test_dump_configuration.py (1)
src/models/config.py (2)
Configuration(1385-1499)dump(1487-1499)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / vertexai
- GitHub Check: E2E: server mode / vertexai
- GitHub Check: E2E: library mode / azure
🔇 Additional comments (31)
tests/unit/app/endpoints/test_tools.py (1)
52-60: Expanded docstring is clear and accurate.The updated docstring effectively documents the mock objects' mapping-like behavior and lists the specific fields they contain, which provides helpful context for understanding how these mocks are used throughout the tests.
tests/unit/app/endpoints/test_conversations_v2.py (4)
138-147: Docstring expansion improves fixture documentation.The expanded docstring with structured Parameters and Returns sections provides clear guidance on the fixture's purpose and behavior. Well-documented.
315-323: Expanded docstring clarifies flag propagation testing.The enhanced docstring now explicitly documents the test's setup (mocked configuration and auth tuple) and what is being asserted (cache.list called with skip flag). This provides better context for future maintainers.
395-406: Expanded docstring documents error scenario thoroughly.The enhanced docstring now clearly documents the configuration state (cache type = None), the validation behavior (check_suid returns True), and the expected error response. This level of detail aids in understanding the error-path testing strategy.
663-673: Expanded docstring documents flag behavior and cache invocation.The enhanced docstring clearly explains that the test verifies the skip-userid flag is propagated from the auth tuple through to the cache delete method call. The explanation of the auth tuple structure and expected call signature adds valuable context.
tests/unit/app/test_database.py (4)
19-24: Well-documented fixture behavior.The expanded docstring clearly describes the generator fixture pattern and cleanup behavior, improving test maintainability.
41-47: Clear documentation of fixture configuration.The detailed Returns section documents all configuration fields, making it easy to understand the test fixture's provided values.
277-294: Comprehensive documentation of test helper.The expanded docstring provides detailed information about parameters and return values, making the test helper's purpose and usage crystal clear.
308-321: Excellent documentation of verification helper.The detailed parameter descriptions clarify the expected mock behavior and assertions, enhancing test readability.
tests/unit/app/endpoints/test_shields.py (3)
49-58: Well-documented test scenario.The expanded docstring clearly describes the test's purpose, setup, and expected behavior. The documentation accurately reflects the test implementation.
118-131: Excellent documentation with parameter details.The expanded docstring provides comprehensive coverage of the test scenario, including error conditions, patches, and expected outcomes. The Parameters section is a nice addition.
244-253: Clear and detailed docstring.The expanded docstring effectively communicates the test's purpose and the specific error handling behavior being verified.
tests/unit/app/endpoints/README.md (1)
45-47: LGTM!The documentation entry for
test_rlsapi_v1.pyis clear and follows the established format of other test file entries in this README.tests/unit/app/endpoints/test_health.py (1)
129-140: LGTM!The expanded docstring clearly describes the test scenario, including the mock setup and expected assertions. This enhances test readability and maintainability.
tests/unit/app/endpoints/test_models.py (1)
47-57: LGTM!The expanded docstring provides a clear narrative of the test scenario: configuration is loaded but the Llama Stack client cannot connect, resulting in HTTP 503. This improves test documentation.
tests/unit/app/endpoints/test_conversations.py (8)
65-104: LGTM!The expanded docstring for
create_mock_conversationthoroughly documents all parameters and return values, making the helper function easier to understand and maintain.
110-139: LGTM!The expanded docstring for
mock_database_sessionclearly describes the helper's purpose and the session mocking behavior, improving test readability.
144-177: LGTM!The fixture docstring clearly describes the test configuration setup and the returned AppConfig structure.
181-241: LGTM!The comprehensive docstring for
mock_session_data_fixturedescribes the structure and purpose of the mock data, including all nested fields. This enhances understanding of the test data shape.
246-274: LGTM!The docstring clearly describes the expected simplified chat history format returned by tests.
279-294: LGTM!The docstring accurately describes the mock UserConversation fixture attributes.
464-472: LGTM!The expanded docstring for the NotFoundError test case clearly describes the expected behavior when Llama Stack reports a session as not found.
37-52: Authorization checks are being properly validated despite the permissive fixture. Thedummy_requestserves as a base request object, while individual tests that need to validate forbidden access override the authorization resolver to restrict permissions, then assert onHTTP_403_FORBIDDENresponses. No downstream tests are inadvertently skipping authorization validation.Likely an incorrect or invalid review comment.
tests/unit/app/endpoints/test_query_v2.py (1)
32-41: LGTM!The expanded docstring clearly describes the
dummy_requestfixture, including its purpose and return value structure.tests/unit/app/endpoints/test_rags.py (2)
74-96: Test data expanded from 2 to 3 RAG entries.The mock
RagListdata now includes a third entry with IDvs_7b52a8cf-0fa3-489c-cafe-27e061d102f3, and the docstring documents all three mock entries. The corresponding assertion at line 110 is updated to expect 3 RAGs. This change increases test coverage for the list endpoint.
110-110: LGTM!The assertion correctly expects 3 RAGs, matching the updated mock data.
tests/unit/app/endpoints/test_info.py (1)
80-91: LGTM!The expanded docstring clearly describes the test scenario: verifying HTTP 503 response when the Llama Stack client cannot connect. This aligns with the documented learning about handling
APIConnectionErrorfrom Llama Stack in integration code.Based on learnings, this test correctly validates the expected error handling behavior.
tests/unit/app/endpoints/test_streaming_query_v2.py (1)
24-35: LGTM! Clear documentation for the test fixture.The docstring accurately describes the fixture's purpose and setup, including the permissive RBAC configuration for testing.
tests/unit/app/endpoints/test_streaming_query.py (1)
252-272: LGTM! Helpful documentation for test helper functions.The docstrings for
mock_database_operationsandmock_metricsclearly explain what each helper patches and why, making the test setup more maintainable.tests/unit/app/test_routers.py (1)
33-97: LGTM! Well-documented mock implementation.The docstrings for the MockFastAPI class and its methods clearly explain the mock's behavior and the purpose of each method, which aids in understanding the test infrastructure.
tests/unit/app/endpoints/test_query.py (1)
60-79: LGTM! Clear fixture and helper documentation.The docstrings for
dummy_requestand the test helper functions provide clear explanations of their purpose and behavior, improving test maintainability.
| """Test that auth dependency raises HTTPException with 403 for unauthorized access. | ||
| Verify extract_user_token raises HTTPException with status code 401 and the | ||
| expected detail for missing or malformed Authorization headers. | ||
| Checks two scenarios: | ||
| - Missing Authorization header: HTTPException.status_code == 401, | ||
| detail["response"] == "Missing or invalid credentials provided by | ||
| client", detail["cause"] == "No Authorization header found". | ||
| - Invalid Authorization format: HTTPException.status_code == 401, | ||
| detail["response"] == "Missing or invalid credentials provided by | ||
| client", detail["cause"] == "No token found in Authorization header". | ||
| """ |
There was a problem hiding this comment.
Correct the HTTP status code in the docstring subject line.
The docstring subject line (Line 50) states "HTTPException with 403" but the test body correctly verifies status code 401 (Lines 69, 80). The detailed docstring description and assertions are correct—this is an authentication failure scenario (missing or malformed Authorization headers) which should return 401 Unauthorized, not 403 Forbidden.
Apply this diff to fix the docstring:
- """Test that auth dependency raises HTTPException with 403 for unauthorized access.
+ """Test that auth dependency raises HTTPException with 401 for unauthorized access.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Test that auth dependency raises HTTPException with 403 for unauthorized access. | |
| Verify extract_user_token raises HTTPException with status code 401 and the | |
| expected detail for missing or malformed Authorization headers. | |
| Checks two scenarios: | |
| - Missing Authorization header: HTTPException.status_code == 401, | |
| detail["response"] == "Missing or invalid credentials provided by | |
| client", detail["cause"] == "No Authorization header found". | |
| - Invalid Authorization format: HTTPException.status_code == 401, | |
| detail["response"] == "Missing or invalid credentials provided by | |
| client", detail["cause"] == "No token found in Authorization header". | |
| """ | |
| """Test that auth dependency raises HTTPException with 401 for unauthorized access. | |
| Verify extract_user_token raises HTTPException with status code 401 and the | |
| expected detail for missing or malformed Authorization headers. | |
| Checks two scenarios: | |
| - Missing Authorization header: HTTPException.status_code == 401, | |
| detail["response"] == "Missing or invalid credentials provided by | |
| client", detail["cause"] == "No Authorization header found". | |
| - Invalid Authorization format: HTTPException.status_code == 401, | |
| detail["response"] == "Missing or invalid credentials provided by | |
| client", detail["cause"] == "No token found in Authorization header". | |
| """ |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_authorized.py around lines 50 to 62, the
docstring's first line incorrectly states "HTTPException with 403" while the
test assertions expect 401; update the docstring subject line to state
"HTTPException with 401" (or "401 Unauthorized") so it matches the rest of the
description and assertions about missing or malformed Authorization headers.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the docstring error
gh issue create \
--repo lightspeed-core/lightspeed-stack \
--title "Fix incorrect HTTP status code in test_authorized_dependency_unauthorized docstring" \
--body "## Description
The docstring for the \`test_authorized_dependency_unauthorized\` test function in \`tests/unit/app/endpoints/test_authorized.py\` (line 50) incorrectly states that the test verifies \"HTTPException with 403\" when it actually verifies status code 401.
## Details
- **File**: \`tests/unit/app/endpoints/test_authorized.py\`
- **Line**: 50
- **Current**: \"Test that auth dependency raises HTTPException with 403 for unauthorized access.\"
- **Expected**: \"Test that auth dependency raises HTTPException with 401 for unauthorized access.\"
The test body correctly asserts status code 401 (lines 69 and 80), which is appropriate for authentication failures (missing or malformed Authorization headers). The docstring summary line needs to be updated to match.
## Context
- **PR**: #930
- **Comment**: https://github.com/lightspeed-core/lightspeed-stack/pull/930#discussion_r2626057727
- **Requested by**: @tisnik" \
--assignee tisnik
Length of output: 62
@tisnik I've created issue #931 for the incorrect HTTP status code in the docstring: #931
The issue has been assigned to you and includes backlinks to this PR and the review comment.
🧬 Code Graph Analysis Results
src/app/endpoints/authorized.py
- Lines: 25-41
- Full snippet:
async def authorized_endpoint_handler(
auth: Annotated[AuthTuple, Depends(get_auth_dependency())],
) -> AuthorizedResponse:
"""
Handle request to the /authorized endpoint.
Process POST requests to the /authorized endpoint, returning
the authenticated user's ID and username.
Returns:
AuthorizedResponse: Contains the user ID and username of the authenticated user.
"""
# Ignore the user token, we should not return it in the response
user_id, user_name, skip_userid_check, _ = auth
return AuthorizedResponse(
user_id=user_id, username=user_name, skip_userid_check=skip_userid_check
)
src/authentication/utils.py
- Lines: 8-30
- Full snippet:
def extract_user_token(headers: Headers) -> str:
"""Extract the bearer token from an HTTP Authorization header.
Args:
headers: The request headers containing the Authorization value.
Returns:
The extracted bearer token.
Raises:
HTTPException: If the Authorization header is missing or malformed.
"""
authorization_header = headers.get("Authorization")
if not authorization_header:
response = UnauthorizedResponse(cause="No Authorization header found")
raise HTTPException(**response.model_dump())
scheme_and_token = authorization_header.strip().split()
if len(scheme_and_token) != 2 or scheme_and_token[0].lower() != "bearer":
response = UnauthorizedResponse(cause="No token found in Authorization header")
raise HTTPException(**response.model_dump())
return scheme_and_token[1]
tests/unit/app/endpoints/test_authorized.py
- Size/Scope: Full test file contents (long)
- Summary description:
- Tests for the /authorized endpoint behavior, including:
- Successful handling returning user_id, username, and skip_userid_check.
- Unauthorized scenarios where the handler is invoked with missing or malformed auth data, triggering TypeError or ValueError.
- A test verifying that extract_user_token raises HTTPException with 401 status and specific detail content for missing header or invalid Authorization format.
- Tests for the /authorized endpoint behavior, including:
🧠 Learnings used
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/app/endpoints/**/*.py : Use FastAPI `HTTPException` with appropriate status codes for API endpoint error handling
| def test_dump_configuration_with_quota_limiters_different_values( | ||
| tmp_path: Path, | ||
| ) -> None: | ||
| """ | ||
| Test that the Configuration object can be serialized to a JSON file and | ||
| that the resulting file contains all expected sections and values. | ||
| Please note that redaction process is not in place. | ||
| """ | ||
| cfg = Configuration( | ||
| name="test_name", | ||
| service=ServiceConfiguration( | ||
| tls_config=TLSConfiguration( | ||
| tls_certificate_path=Path("tests/configuration/server.crt"), | ||
| tls_key_path=Path("tests/configuration/server.key"), | ||
| tls_key_password=Path("tests/configuration/password"), | ||
| ), | ||
| cors=CORSConfiguration( | ||
| allow_origins=["foo_origin", "bar_origin", "baz_origin"], | ||
| allow_credentials=False, | ||
| allow_methods=["foo_method", "bar_method", "baz_method"], | ||
| allow_headers=["foo_header", "bar_header", "baz_header"], | ||
| ), | ||
| ), | ||
| llama_stack=LlamaStackConfiguration( | ||
| use_as_library_client=True, | ||
| library_client_config_path="tests/configuration/run.yaml", | ||
| api_key=SecretStr("whatever"), | ||
| ), | ||
| user_data_collection=UserDataCollection( | ||
| feedback_enabled=False, feedback_storage=None | ||
| ), | ||
| database=DatabaseConfiguration( | ||
| sqlite=None, | ||
| postgres=PostgreSQLDatabaseConfiguration( | ||
| db="lightspeed_stack", | ||
| user="ls_user", | ||
| password=SecretStr("ls_password"), | ||
| port=5432, | ||
| ca_cert_path=None, | ||
| ssl_mode="require", | ||
| gss_encmode="disable", | ||
| ), | ||
| ), | ||
| mcp_servers=[], | ||
| customization=None, | ||
| inference=InferenceConfiguration( | ||
| default_provider="default_provider", | ||
| default_model="default_model", | ||
| ), | ||
| quota_handlers=QuotaHandlersConfiguration( | ||
| limiters=[ | ||
| QuotaLimiterConfiguration( | ||
| type="user_limiter", | ||
| name="user_monthly_limits", | ||
| initial_quota=1, | ||
| quota_increase=10, | ||
| period="2 seconds", | ||
| ), | ||
| QuotaLimiterConfiguration( | ||
| type="cluster_limiter", | ||
| name="cluster_monthly_limits", | ||
| initial_quota=2, | ||
| quota_increase=20, | ||
| period="1 month", | ||
| ), | ||
| ], | ||
| scheduler=QuotaSchedulerConfiguration( | ||
| period=10, | ||
| database_reconnection_count=123, | ||
| database_reconnection_delay=456, | ||
| ), | ||
| enable_token_history=True, | ||
| ), | ||
| ) | ||
| assert cfg is not None | ||
| dump_file = tmp_path / "test.json" | ||
| cfg.dump(dump_file) | ||
|
|
||
| with open(dump_file, "r", encoding="utf-8") as fin: | ||
| content = json.load(fin) | ||
| # content should be loaded | ||
| assert content is not None | ||
|
|
||
| # all sections must exists | ||
| assert "name" in content | ||
| assert "service" in content | ||
| assert "llama_stack" in content | ||
| assert "user_data_collection" in content | ||
| assert "mcp_servers" in content | ||
| assert "authentication" in content | ||
| assert "authorization" in content | ||
| assert "customization" in content | ||
| assert "inference" in content | ||
| assert "database" in content | ||
| assert "byok_rag" in content | ||
| assert "quota_handlers" in content | ||
|
|
||
| # check the whole deserialized JSON file content | ||
| assert content == { | ||
| "name": "test_name", | ||
| "service": { | ||
| "host": "localhost", | ||
| "port": 8080, | ||
| "auth_enabled": False, | ||
| "workers": 1, | ||
| "color_log": True, | ||
| "access_log": True, | ||
| "tls_config": { | ||
| "tls_certificate_path": "tests/configuration/server.crt", | ||
| "tls_key_password": "tests/configuration/password", | ||
| "tls_key_path": "tests/configuration/server.key", | ||
| }, | ||
| "cors": { | ||
| "allow_credentials": False, | ||
| "allow_headers": [ | ||
| "foo_header", | ||
| "bar_header", | ||
| "baz_header", | ||
| ], | ||
| "allow_methods": [ | ||
| "foo_method", | ||
| "bar_method", | ||
| "baz_method", | ||
| ], | ||
| "allow_origins": [ | ||
| "foo_origin", | ||
| "bar_origin", | ||
| "baz_origin", | ||
| ], | ||
| }, | ||
| }, | ||
| "llama_stack": { | ||
| "url": None, | ||
| "use_as_library_client": True, | ||
| "api_key": "**********", | ||
| "library_client_config_path": "tests/configuration/run.yaml", | ||
| }, | ||
| "user_data_collection": { | ||
| "feedback_enabled": False, | ||
| "feedback_storage": None, | ||
| "transcripts_enabled": False, | ||
| "transcripts_storage": None, | ||
| }, | ||
| "mcp_servers": [], | ||
| "authentication": { | ||
| "module": "noop", | ||
| "skip_tls_verification": False, | ||
| "k8s_ca_cert_path": None, | ||
| "k8s_cluster_api": None, | ||
| "jwk_config": None, | ||
| "api_key_config": None, | ||
| "rh_identity_config": None, | ||
| }, | ||
| "customization": None, | ||
| "inference": { | ||
| "default_provider": "default_provider", | ||
| "default_model": "default_model", | ||
| }, | ||
| "database": { | ||
| "sqlite": None, | ||
| "postgres": { | ||
| "host": "localhost", | ||
| "port": 5432, | ||
| "db": "lightspeed_stack", | ||
| "user": "ls_user", | ||
| "password": "**********", | ||
| "ssl_mode": "require", | ||
| "gss_encmode": "disable", | ||
| "namespace": "public", | ||
| "ca_cert_path": None, | ||
| }, | ||
| }, | ||
| "authorization": None, | ||
| "conversation_cache": { | ||
| "memory": None, | ||
| "postgres": None, | ||
| "sqlite": None, | ||
| "type": None, | ||
| }, | ||
| "byok_rag": [], | ||
| "quota_handlers": { | ||
| "sqlite": None, | ||
| "postgres": None, | ||
| "limiters": [ | ||
| { | ||
| "initial_quota": 1, | ||
| "name": "user_monthly_limits", | ||
| "period": "2 seconds", | ||
| "quota_increase": 10, | ||
| "type": "user_limiter", | ||
| }, | ||
| { | ||
| "initial_quota": 2, | ||
| "name": "cluster_monthly_limits", | ||
| "period": "1 month", | ||
| "quota_increase": 20, | ||
| "type": "cluster_limiter", | ||
| }, | ||
| ], | ||
| "scheduler": { | ||
| "period": 10, | ||
| "database_reconnection_count": 123, | ||
| "database_reconnection_delay": 456, | ||
| }, | ||
| "enable_token_history": True, | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate test duplication with parametrization.
The new test test_dump_configuration_with_quota_limiters_different_values duplicates ~210 lines from test_dump_configuration_with_quota_limiters (lines 313-514), differing only in three scheduler parameters (lines 584-588, assertions at 719-720). This violates the DRY principle and increases maintenance burden.
Refactor both tests into a single parametrized test:
+@pytest.mark.parametrize(
+ "scheduler_params,expected_values",
+ [
+ (
+ {"period": 10},
+ {"period": 10, "database_reconnection_count": 10, "database_reconnection_delay": 1},
+ ),
+ (
+ {"period": 10, "database_reconnection_count": 123, "database_reconnection_delay": 456},
+ {"period": 10, "database_reconnection_count": 123, "database_reconnection_delay": 456},
+ ),
+ ],
+)
-def test_dump_configuration_with_quota_limiters(tmp_path: Path) -> None:
+def test_dump_configuration_with_quota_limiters(
+ tmp_path: Path, scheduler_params: dict, expected_values: dict
+) -> None:
"""
Test that the Configuration object can be serialized to a JSON file and
that the resulting file contains all expected sections and values.
Please note that redaction process is not in place.
"""
cfg = Configuration(
# ... existing config ...
quota_handlers=QuotaHandlersConfiguration(
limiters=[
# ... existing limiters ...
],
- scheduler=QuotaSchedulerConfiguration(period=10),
+ scheduler=QuotaSchedulerConfiguration(**scheduler_params),
enable_token_history=True,
),
)
# ... rest of test ...
# Update assertion to use expected_values
assert content["quota_handlers"]["scheduler"] == expected_values
-
-def test_dump_configuration_with_quota_limiters_different_values(
- tmp_path: Path,
-) -> None:
- # Remove entire duplicate functionThis reduces the file from ~1078 lines to ~870 lines while maintaining the same test coverage.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/unit/models/config/test_dump_configuration.py around lines 313-725, the
two nearly identical tests should be combined into a single
pytest.mark.parametrize test: extract the common Configuration construction into
a local helper (or inline shared code) and parametrize the differing scheduler
fields (period, database_reconnection_count, database_reconnection_delay) and
the corresponding expected JSON entries; replace the two test functions with one
@pytest.mark.parametrize decorator that iterates over tuples of the three
scheduler values and asserts the dumped JSON contains the expected scheduler
object for each case, removing duplicated assertions and preserving all other
checks.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for test duplication refactoring
gh issue create \
--repo lightspeed-core/lightspeed-stack \
--title "Refactor duplicated quota limiter tests in test_dump_configuration.py" \
--body "## Description
Test duplication has been identified in \`tests/unit/models/config/test_dump_configuration.py\`. The tests \`test_dump_configuration_with_quota_limiters\` and \`test_dump_configuration_with_quota_limiters_different_values\` duplicate approximately 210 lines of code, differing only in three scheduler parameters.
## Details
- **File**: \`tests/unit/models/config/test_dump_configuration.py\`
- **Tests affected**:
- \`test_dump_configuration_with_quota_limiters\`
- \`test_dump_configuration_with_quota_limiters_different_values\`
- **Issue**: These tests share identical structure and assertions, varying only in the scheduler configuration parameters (\`database_reconnection_count\` and \`database_reconnection_delay\`)
## Recommendation
Consider refactoring these tests using pytest parametrization to eliminate duplication and improve maintainability.
## References
- Pull Request: https://github.com/lightspeed-core/lightspeed-stack/pull/930
- Review Comment: https://github.com/lightspeed-core/lightspeed-stack/pull/930#discussion_r2626057735
- Reported by: @tisnik" \
--assignee tisnik
Length of output: 62
@tisnik, I've created issue #932 to track the refactoring of the duplicated quota limiter tests in test_dump_configuration.py. The issue has been assigned to you and includes references back to this PR and comment.
/\_/\
( o.o )
> ^ < Issue created!
/| |\
| |
🧬 Code Graph Analysis Results
filename: src/models/config.py
lines 324-341
def config(self) -> SQLiteDatabaseConfiguration | PostgreSQLDatabaseConfiguration:
"""
Get the active database backend configuration.
Returns:
SQLiteDatabaseConfiguration or PostgreSQLDatabaseConfiguration: The
configured SQLite configuration if `sqlite` is set, otherwise the
configured PostgreSQL configuration if `postgres` is set.
Raises:
ValueError: If neither `sqlite` nor `postgres` is
configured.
"""
if self.sqlite is not None:
return self.sqlite
if self.postgres is not None:
return self.postgres
raise ValueError("No database configuration found")
filename: src/configuration.py
lines 424-456
class ModelContextProtocolServer(ConfigurationBase):
"""Model context protocol server configuration.
MCP (Model Context Protocol) servers provide tools and capabilities to the
AI agents. These are configured by this structure. Only MCP servers
defined in the lightspeed-stack.yaml configuration are available to the
agents. Tools configured in the llama-stack run.yaml are not accessible to
lightspeed-core agents.
Useful resources:
- [Model Context Protocol](https://modelcontextprotocol.io/docs/getting-started/intro)
- [MCP FAQs](https://modelcontextprotocol.io/faqs)
- [Wikipedia article](https://en.wikipedia.org/wiki/Model_Context_Protocol)
"""
name: str = Field(
...,
title="MCP name",
description="MCP server name that must be unique",
)
provider_id: str = Field(
"model-context-protocol",
title="Provider ID",
description="MCP provider identification",
)
url: str = Field(
...,
title="MCP server URL",
description="URL of the MCP server",
)
filename: src/configuration.py
lines 1487-1499
def dump(self, filename: str = "configuration.json") -> None:
"""
Write the current Configuration model to a JSON file.
The configuration is serialized with an indentation of 4 spaces using
the model's JSON representation and written with UTF-8 encoding. If the
file exists it will be overwritten.
Parameters:
filename (str): Path to the output file (defaults to "configuration.json").
"""
with open(filename, "w", encoding="utf-8") as fout:
fout.write(self.model_dump_json(indent=4))
1 participant
Description
LCORE-1126: updated docstrings in unit tests
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Summary by CodeRabbit
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.