LCORE-1206: add extra e2e tests to align with OLS#1103
LCORE-1206: add extra e2e tests to align with OLS#1103tisnik merged 1 commit intolightspeed-core:mainfrom
Conversation
add new tests for token quotas
WalkthroughThis pull request adds end-to-end test coverage for token usage metrics validation. It introduces new step definitions for capturing and asserting token metrics from API responses and Prometheus endpoints, and updates existing test scenarios to verify token metrics increase after successful queries and remain unchanged during error conditions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/features/query.feature (1)
96-129:⚠️ Potential issue | 🟠 MajorDuplicate scenario names and incorrect scenario content.
There are two issues here:
Lines 106-114: This scenario is titled "Check if LLM responds for query request with error for missing query" but the request body contains a valid
queryfield and tests for "Improper conversation ID" error. This appears to be a copy of the scenario above (lines 96-105).Lines 116-129: Another scenario with the same title "Check if LLM responds for query request with error for missing query" which correctly tests the missing query case.
The scenario at lines 106-114 should be removed as it duplicates the scenario at lines 96-105.
🐛 Proposed fix - remove duplicate scenario
Then The status code of the response is 422 And The body of the response contains Value error, Improper conversation ID '123e4567' -Scenario: Check if LLM responds for query request with error for missing query - Given The system is in default state - And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva - When I use "query" to ask question with authorization header - """ - {"conversation_id": "123e4567", "query": "Write a simple code for reversing string", "model": "{MODEL}", "provider": "{PROVIDER}"} - """ - Then The status code of the response is 422 - And The body of the response contains Value error, Improper conversation ID '123e4567' - Scenario: Check if LLM responds for query request with error for missing query Given The system is in default state And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva
🤖 Fix all issues with AI agents
In `@tests/e2e/features/steps/token_counters.py`:
- Around line 98-106: The precondition in check_streamed_token_counter_fields
incorrectly asserts context.response_data but then uses context.response.text;
update the precondition to assert the actual dependency, e.g., verify
context.response is not None and that context.response.text is present before
calling _get_end_event_data, so replace the assert on context.response_data with
an assertion that context.response and context.response.text (or equivalent) are
available to avoid a misleading check and potential AttributeError when
extracting end_event_data.
🧹 Nitpick comments (3)
tests/e2e/features/steps/token_counters.py (1)
166-169: Consider creating a new dict to avoid mutating the parsed event data.Line 167 retrieves the
datadict from the event, and line 168 mutates it. This modifies the original parsed JSON object. While acceptable in test code, creating a shallow copy would be safer.♻️ Proposed fix
if event.get("event") == "end": # Merge data contents with available_quotas from parent level - result = event.get("data", {}) + result = {**event.get("data", {})} result["available_quotas"] = event.get("available_quotas", {}) return resulttests/e2e/features/streaming_query.feature (2)
107-115: Consider adding token metrics validation for consistency.This scenario doesn't capture or validate token metrics, unlike similar 422 error scenarios (missing query, missing model). Consider adding token metrics capture and unchanged validation for consistency across error scenarios.
117-137: Inconsistent token metrics validation between similar error scenarios.The "unknown provider" scenario (lines 127-137) captures and validates token metrics unchanged, but the "unknown model" scenario (lines 117-125) does not. Both are 404 errors - consider adding token metrics validation to the unknown model scenario for consistency.
♻️ Proposed fix
Scenario: Check if LLM responds for streaming_query request with error for unknown model Given The system is in default state And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva + And I capture the current token metrics When I use "streaming_query" to ask question with authorization header """ {"query": "Say hello", "provider": "{PROVIDER}", "model":"unknown"} """ Then The status code of the response is 404 And The body of the response contains Model with ID unknown does not exist + And The token metrics should not have changed
| @then("The streamed response should contain token counter fields") | ||
| def check_streamed_token_counter_fields(context: Context) -> None: | ||
| """Check that streamed response end event contains token fields.""" | ||
| assert context.response_data is not None, "Response data needs to be parsed first" | ||
|
|
||
| # Parse the end event from the streaming response to get token info | ||
| end_event_data = _get_end_event_data(context.response.text) | ||
| assert end_event_data is not None, "End event not found in streaming response" | ||
|
|
There was a problem hiding this comment.
Misleading precondition assertion.
Line 101 asserts context.response_data is not None, but line 104 accesses context.response.text. The assertion doesn't validate what's actually used. Consider aligning the precondition check with the actual dependency.
🔧 Proposed fix
def check_streamed_token_counter_fields(context: Context) -> None:
"""Check that streamed response end event contains token fields."""
- assert context.response_data is not None, "Response data needs to be parsed first"
+ assert context.response is not None, "Response needs to be available first"
# Parse the end event from the streaming response to get token info
end_event_data = _get_end_event_data(context.response.text)🤖 Prompt for AI Agents
In `@tests/e2e/features/steps/token_counters.py` around lines 98 - 106, The
precondition in check_streamed_token_counter_fields incorrectly asserts
context.response_data but then uses context.response.text; update the
precondition to assert the actual dependency, e.g., verify context.response is
not None and that context.response.text is present before calling
_get_end_event_data, so replace the assert on context.response_data with an
assertion that context.response and context.response.text (or equivalent) are
available to avoid a misleading check and potential AttributeError when
extracting end_event_data.
2 participants
add new tests for token quotas
Description
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