LCORE-1206: add new integration tests for handling the attachments#1228
Merged
radofuchs merged 1 commit intolightspeed-core:mainfrom Feb 26, 2026
Merged
Conversation
Contributor
WalkthroughThis PR adds comprehensive integration tests for the /v1/query and /v1/streaming_query endpoints, including validation for attachments, error handling, and various payload scenarios. A new pytest fixture provides test HTTP client setup for integration tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/integration/conftest.py (1)
164-191: Good fixture implementation with proper cleanup.The environment variable restoration in the
finallyblock correctly handles both scenarios (existing value and no previous value). The fixture appropriately depends ontest_configto ensure configuration is loaded before app initialization.Consider extracting the config path construction to a module-level constant or helper since it's duplicated in
test_config_fixture(line 47) and here (lines 174-178).♻️ Optional: Extract config path to constant
+TEST_CONFIG_PATH = ( + Path(__file__).resolve().parent.parent + / "configuration" + / "lightspeed-stack.yaml" +) + + `@pytest.fixture`(name="test_config", scope="function") def test_config_fixture() -> Generator: - config_path = ( - Path(__file__).parent.parent / "configuration" / "lightspeed-stack.yaml" - ) + config_path = TEST_CONFIG_PATH assert config_path.exists(), f"Config file not found: {config_path}"And similarly update
integration_http_client_fixture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 164 - 191, Extract the duplicated config path construction into a shared module-level constant or helper function (e.g., CONFIG_PATH or get_config_path()) and use it inside integration_http_client_fixture and test_config_fixture to avoid duplication; update integration_http_client_fixture to replace the inline Path(...) assembly with the shared symbol and keep the existing environment setup/teardown logic intact.
🤖 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_query_integration.py`:
- Around line 389-391: The assertion using getattr(response, "status_code",
status.HTTP_200_OK) is a no-op because QueryResponse has no status_code; remove
that assertion and instead validate the real success condition—either assert the
actual HTTP response object’s status_code (e.g., response_http.status_code ==
status.HTTP_200_OK) if you have access to it, or simply assert properties on the
QueryResponse itself (e.g., isinstance(response, QueryResponse) and
response.conversation_id is not None and response.response is not None); update
the test to reference QueryResponse and the correct HTTP response variable (or
remove the status check) rather than using getattr on response.
- Around line 357-359: The status_code assertion is a no-op because
query_endpoint_handler returns a QueryResponse (which lacks status_code); remove
the line asserting getattr(response, "status_code", status.HTTP_200_OK) ==
status.HTTP_200_OK and instead assert the response is the expected model and
populated, e.g., assert isinstance(response, QueryResponse) (or check
response.__class__.__name__ == "QueryResponse") and keep the existing assertions
that response.conversation_id and response.response are not None to validate
success.
- Around line 432-434: The first assertion is a no-op because getattr(response,
"status_code", status.HTTP_200_OK) will default to status.HTTP_200_OK and always
pass; update the test in tests/integration/endpoints/test_query_integration.py
to assert the real status code instead (e.g., assert response.status_code ==
status.HTTP_200_OK) or explicitly expect None on missing attribute (e.g.,
getattr(response, "status_code", None) == status.HTTP_200_OK), and otherwise
remove the meaningless assertion; refer to the response object and
status.HTTP_200_OK in the change.
---
Nitpick comments:
In `@tests/integration/conftest.py`:
- Around line 164-191: Extract the duplicated config path construction into a
shared module-level constant or helper function (e.g., CONFIG_PATH or
get_config_path()) and use it inside integration_http_client_fixture and
test_config_fixture to avoid duplication; update integration_http_client_fixture
to replace the inline Path(...) assembly with the shared symbol and keep the
existing environment setup/teardown logic intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/integration/conftest.pytests/integration/endpoints/test_query_integration.pytests/integration/endpoints/test_streaming_query_integration.pytests/integration/test_openapi_json.py
Comment on lines
+357
to
+359
| assert getattr(response, "status_code", status.HTTP_200_OK) == status.HTTP_200_OK | ||
| assert response.conversation_id is not None | ||
| assert response.response is not None |
Contributor
There was a problem hiding this comment.
Status code assertion is always true (no-op).
query_endpoint_handler returns a QueryResponse object, not a FastAPI Response. The QueryResponse model doesn't have a status_code attribute, so getattr(response, "status_code", status.HTTP_200_OK) will always return the default 200, making this assertion meaningless.
Either remove the status_code assertion (since the handler doesn't raise means success), or verify the response type directly.
🔧 Proposed fix
- assert getattr(response, "status_code", status.HTTP_200_OK) == status.HTTP_200_OK
+ # Handler returning without exception indicates success (HTTP 200)
assert response.conversation_id is not None
assert response.response is not None📝 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.
Suggested change
| assert getattr(response, "status_code", status.HTTP_200_OK) == status.HTTP_200_OK | |
| assert response.conversation_id is not None | |
| assert response.response is not None | |
| # Handler returning without exception indicates success (HTTP 200) | |
| assert response.conversation_id is not None | |
| assert response.response is not None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/endpoints/test_query_integration.py` around lines 357 -
359, The status_code assertion is a no-op because query_endpoint_handler returns
a QueryResponse (which lacks status_code); remove the line asserting
getattr(response, "status_code", status.HTTP_200_OK) == status.HTTP_200_OK and
instead assert the response is the expected model and populated, e.g., assert
isinstance(response, QueryResponse) (or check response.__class__.__name__ ==
"QueryResponse") and keep the existing assertions that response.conversation_id
and response.response are not None to validate success.
Comment on lines
+389
to
+391
| assert getattr(response, "status_code", status.HTTP_200_OK) == status.HTTP_200_OK | ||
| assert response.conversation_id is not None | ||
| assert response.response is not None |
Contributor
There was a problem hiding this comment.
Same no-op status code assertion.
Same issue as above - QueryResponse doesn't have status_code, so the assertion is always true.
🔧 Proposed fix
- assert getattr(response, "status_code", status.HTTP_200_OK) == status.HTTP_200_OK
assert response.conversation_id is not None
assert response.response is not None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/endpoints/test_query_integration.py` around lines 389 -
391, The assertion using getattr(response, "status_code", status.HTTP_200_OK) is
a no-op because QueryResponse has no status_code; remove that assertion and
instead validate the real success condition—either assert the actual HTTP
response object’s status_code (e.g., response_http.status_code ==
status.HTTP_200_OK) if you have access to it, or simply assert properties on the
QueryResponse itself (e.g., isinstance(response, QueryResponse) and
response.conversation_id is not None and response.response is not None); update
the test to reference QueryResponse and the correct HTTP response variable (or
remove the status check) rather than using getattr on response.
Comment on lines
+432
to
+434
| assert getattr(response, "status_code", status.HTTP_200_OK) == status.HTTP_200_OK | ||
| assert response.conversation_id is not None | ||
| assert response.response is not None |
Contributor
There was a problem hiding this comment.
Same no-op status code assertion.
Same issue as above - remove or fix the meaningless assertion.
🔧 Proposed fix
- assert getattr(response, "status_code", status.HTTP_200_OK) == status.HTTP_200_OK
assert response.conversation_id is not None
assert response.response is not None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/endpoints/test_query_integration.py` around lines 432 -
434, The first assertion is a no-op because getattr(response, "status_code",
status.HTTP_200_OK) will default to status.HTTP_200_OK and always pass; update
the test in tests/integration/endpoints/test_query_integration.py to assert the
real status code instead (e.g., assert response.status_code ==
status.HTTP_200_OK) or explicitly expect None on missing attribute (e.g.,
getattr(response, "status_code", None) == status.HTTP_200_OK), and otherwise
remove the meaningless assertion; refer to the response object and
status.HTTP_200_OK in the change.
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
2 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.
Description
Added new integration tests for handling the attachments
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
Checklist before requesting a review
Testing
Summary by CodeRabbit