LCORE-1955: Fixed issues in BYOK integration tests#1559
Conversation
WalkthroughTest code in the BYOK query integration suite was refactored to explicitly pass all Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 1
🤖 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_byok_integration.py`:
- Around line 325-338: Tests are overly verbose because each QueryRequest is
constructed with all parameters instead of using defaults; update tests to only
pass relevant fields (e.g., query and when needed vector_store_ids) by removing
redundant arguments from QueryRequest calls, or add a small test factory like
make_query_request(query, vector_store_ids=None, **overrides) and use it across
tests; locate constructions of QueryRequest in test_query_byok_integration.py
(e.g., the query_request variable instances) and replace them with minimal calls
or the factory to reduce duplication and improve clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97f60739-ef7f-49d0-8179-7350ede60228
📒 Files selected for processing (1)
tests/integration/endpoints/test_query_byok_integration.py
📜 Review details
⏰ 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). (15)
- GitHub Check: mypy
- GitHub Check: integration_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Import FastAPI dependencies with:from fastapi import APIRouter, HTTPException, Request, status, Depends
Import Llama Stack client with:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
Type aliases defined at module level for clarity
Use Final[type] as type hint for all constants
All functions require docstrings with brief descriptions
Complete type annotations for parameters and return types in functions
Usetyping_extensions.Selffor model validators in Pydantic models
Use modern union type syntaxstr | intinstead ofUnion[str, int]
UseOptional[Type]for optional type hints
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Use standard log levels with clear purposes: debug, info, warning, error
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes: Configuration, Error/Exception, Resolver, Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Use@model_validatorand@field_validatorfor Pydantic model validation
Complete type annotations for all class attributes; use specific types, notAny
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections
Files:
tests/integration/endpoints/test_query_byok_integration.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest; pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Use markerpytest.mark.asynciofor async tests
Unit tests require 60% coverage, integration tests 10%
Files:
tests/integration/endpoints/test_query_byok_integration.py
🧠 Learnings (1)
📚 Learning: 2026-02-23T14:11:46.950Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1198
File: src/utils/responses.py:184-188
Timestamp: 2026-02-23T14:11:46.950Z
Learning: The query request validator in the Responses API flow requires that `query_request.model` and `query_request.provider` must either both be specified or both be absent. The concatenated format (e.g., `model="provider/model"` with `provider=None`) is not permitted by the validator.
Applied to files:
tests/integration/endpoints/test_query_byok_integration.py
| query_request = QueryRequest( | ||
| query="What is OpenShift?", | ||
| conversation_id=None, | ||
| provider=None, | ||
| model=None, | ||
| system_prompt=None, | ||
| attachments=None, | ||
| no_tools=False, | ||
| generate_topic_summary=None, | ||
| media_type=None, | ||
| vector_store_ids=None, | ||
| shield_ids=None, | ||
| solr=None, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reduce test verbosity by leveraging QueryRequest defaults or introducing a test helper.
All 13 test cases now explicitly pass every QueryRequest parameter, even when most are set to their default values. This creates significant code duplication (~150 lines of repetitive parameter assignments) and obscures test intent by burying the meaningful parameters (query and occasionally vector_store_ids) among boilerplate.
Testing best practice is to specify only parameters relevant to each test, allowing the model's defaults to handle the rest. This improves maintainability and makes each test's purpose clearer.
♻️ Recommended approaches
Option 1 (Preferred): Rely on QueryRequest defaults
Most parameters can be omitted entirely:
query_request = QueryRequest(
query="What is OpenShift?",
- conversation_id=None,
- provider=None,
- model=None,
- system_prompt=None,
- attachments=None,
- no_tools=False,
- generate_topic_summary=None,
- media_type=None,
- vector_store_ids=None,
- shield_ids=None,
- solr=None,
)For tests that need specific vector_store_ids:
query_request = QueryRequest(
query="What is OpenShift?",
- conversation_id=None,
- provider=None,
- model=None,
- system_prompt=None,
- attachments=None,
- no_tools=False,
- generate_topic_summary=None,
- media_type=None,
vector_store_ids=["source-a"],
- shield_ids=None,
- solr=None,
)Option 2: Introduce a test helper factory
If explicit control is needed across all tests, create a helper function in the test file:
def make_query_request(
query: str,
vector_store_ids: list[str] | None = None,
**overrides: Any,
) -> QueryRequest:
"""Build a QueryRequest with common test defaults."""
return QueryRequest(
query=query,
conversation_id=overrides.get("conversation_id"),
provider=overrides.get("provider"),
model=overrides.get("model"),
system_prompt=overrides.get("system_prompt"),
attachments=overrides.get("attachments"),
no_tools=overrides.get("no_tools", False),
generate_topic_summary=overrides.get("generate_topic_summary"),
media_type=overrides.get("media_type"),
vector_store_ids=vector_store_ids,
shield_ids=overrides.get("shield_ids"),
solr=overrides.get("solr"),
)Then use it:
query_request = make_query_request("What is OpenShift?")
# Or with filtering:
query_request = make_query_request("What is OpenShift?", vector_store_ids=["source-a"])Also applies to: 385-398, 471-484, 542-555, 593-606, 637-650, 685-698, 728-741, 849-862, 930-943, 1033-1046, 1118-1131, 1222-1235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/endpoints/test_query_byok_integration.py` around lines 325
- 338, Tests are overly verbose because each QueryRequest is constructed with
all parameters instead of using defaults; update tests to only pass relevant
fields (e.g., query and when needed vector_store_ids) by removing redundant
arguments from QueryRequest calls, or add a small test factory like
make_query_request(query, vector_store_ids=None, **overrides) and use it across
tests; locate constructions of QueryRequest in test_query_byok_integration.py
(e.g., the query_request variable instances) and replace them with minimal calls
or the factory to reduce duplication and improve clarity.
1 participant
Description
LCORE-1955: Fixed issues in BYOK integration tests
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit
Note: This release contains no user-facing changes.