Skip to content

LCORE-1472: update wording in features#1599

Merged
radofuchs merged 2 commits into
lightspeed-core:mainfrom
radofuchs:LCORE_1472_features_wording_updates
Apr 27, 2026
Merged

LCORE-1472: update wording in features#1599
radofuchs merged 2 commits into
lightspeed-core:mainfrom
radofuchs:LCORE_1472_features_wording_updates

Conversation

@radofuchs
Copy link
Copy Markdown
Contributor

@radofuchs radofuchs commented Apr 27, 2026

Description

update wording in features and remove one duplicated config

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: (e.g., Claude, CodeRabbit, Ollama, etc., N/A if not used)
  • Generated by: (e.g., tool name and version; N/A if not used)

Related Tickets & Documents

  • Related Issue #LCORE-1472
  • Closes #LCORE-1472

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

Release Notes

  • Tests
    • Removed unused end-to-end test configurations for library and server mode setups.
    • Updated multiple test assertions to use present-tense phrasing for improved readability and consistency across all E2E test scenarios.
    • Modified one test configuration to use an alternative setup and added authentication header for API testing.

@radofuchs radofuchs requested a review from tisnik April 27, 2026 14:08
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

Warning

Rate limit exceeded

@radofuchs has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 37 minutes and 59 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 81bc8b1d-0a6e-4038-adae-26259233fa1d

📥 Commits

Reviewing files that changed from the base of the PR and between fdf67ac and f643c5c.

📒 Files selected for processing (2)
  • tests/e2e/features/responses.feature
  • tests/e2e/features/steps/llm_query_response.py

Walkthrough

This PR updates E2E test assertion language from imperative ("should...") to declarative ("is...", "contains...") phrasing across feature files and step definitions. Additionally, two test configuration files are removed, and one feature file's test configuration reference is updated with an authentication header injection.

Changes

Cohort / File(s) Summary
Configuration File Deletions
tests/e2e/configuration/library-mode/lightspeed-stack-no-mcp.yaml, tests/e2e/configuration/server-mode/lightspeed-stack-no-mcp.yaml
Removed E2E test configurations for Lightspeed Core Service running with llama-stack library mode and server mode, eliminating test scenarios for these deployment patterns.
MCP Servers API Configuration Update
tests/e2e/features/mcp_servers_api_no_config.feature
Switched test configuration from lightspeed-stack-no-mcp.yaml to lightspeed-stack-no-cache.yaml and injected Authorization: Bearer mcp-e2e-no-config-token header into background setup.
Feature File Assertion Rewording
tests/e2e/features/faiss.feature, tests/e2e/features/inline_rag.feature, tests/e2e/features/mcp.feature, tests/e2e/features/models.feature, tests/e2e/features/query.feature, tests/e2e/features/responses.feature, tests/e2e/features/rlsapi_v1.feature, tests/e2e/features/rlsapi_v1_errors.feature, tests/e2e/features/smoketests.feature, tests/e2e/features/streaming_query.feature
Updated assertion step wording from imperative mood ("should contain", "should be", "should have") to declarative mood ("contains", "is", "have") across multiple scenarios without changing assertion logic.
Step Definition Decorator Updates
tests/e2e/features/steps/common_http.py, tests/e2e/features/steps/llm_query_response.py, tests/e2e/features/steps/models.py, tests/e2e/features/steps/rlsapi_v1.py, tests/e2e/features/steps/token_counters.py
Updated Behave @then step decorator strings from imperative to declarative phrasing to match corresponding feature file changes; underlying assertion logic remains unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: updating wording in feature files across the E2E test suite.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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/e2e/features/responses.feature`:
- Line 406: Update the incorrect subject–verb agreement "The responses output do
not include any tool invocation item types" to "The responses output does not
include any tool invocation item types" in both occurrences in
tests/e2e/features/responses.feature, and update the matching step decorator
string in tests/e2e/features/steps/llm_query_response.py (the decorator
currently using the same incorrect phrase) so the step text matches the
corrected grammar.

In `@tests/e2e/features/steps/llm_query_response.py`:
- Line 39: The step decorator string has a subject-verb agreement error: change
the `@then` step text from "The responses output do not include any tool
invocation item types" to "The responses output does not include any tool
invocation item types" in the step definition (the `@then`(...) decorator shown in
the diff) and update the exact same step text in the two feature-file
occurrences so the BDD matcher still finds the step.

In `@tests/e2e/features/steps/token_counters.py`:
- Line 73: The step decorator string "@then('The token metrics are not
changed')" is inconsistent with its docstring and sibling steps; update the step
definition in tests/e2e/features/steps/token_counters.py to "@then('The token
metrics have not changed')" and update the matching step text in the feature
files tests/e2e/features/streaming_query.feature (lines around the current step
occurrences) and tests/e2e/features/query.feature so their step sentences read
"The token metrics have not changed" to keep Gherkin tense consistent with the
existing "have increased" step.
🪄 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: 337a5e57-10b9-48f3-8b51-207a386f0cb0

📥 Commits

Reviewing files that changed from the base of the PR and between c985fd7 and fdf67ac.

📒 Files selected for processing (18)
  • tests/e2e/configuration/library-mode/lightspeed-stack-no-mcp.yaml
  • tests/e2e/configuration/server-mode/lightspeed-stack-no-mcp.yaml
  • tests/e2e/features/faiss.feature
  • tests/e2e/features/inline_rag.feature
  • tests/e2e/features/mcp.feature
  • tests/e2e/features/mcp_servers_api_no_config.feature
  • tests/e2e/features/models.feature
  • tests/e2e/features/query.feature
  • tests/e2e/features/responses.feature
  • tests/e2e/features/rlsapi_v1.feature
  • tests/e2e/features/rlsapi_v1_errors.feature
  • tests/e2e/features/smoketests.feature
  • tests/e2e/features/steps/common_http.py
  • tests/e2e/features/steps/llm_query_response.py
  • tests/e2e/features/steps/models.py
  • tests/e2e/features/steps/rlsapi_v1.py
  • tests/e2e/features/steps/token_counters.py
  • tests/e2e/features/streaming_query.feature
💤 Files with no reviewable changes (2)
  • tests/e2e/configuration/library-mode/lightspeed-stack-no-mcp.yaml
  • tests/e2e/configuration/server-mode/lightspeed-stack-no-mcp.yaml
📜 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). (16)
  • GitHub Check: bandit
  • GitHub Check: pydocstyle
  • GitHub Check: build-pr
  • GitHub Check: Pyright
  • GitHub Check: black
  • GitHub Check: Pylinter
  • GitHub Check: unit_tests (3.12)
  • GitHub Check: unit_tests (3.13)
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: E2E: server mode / ci / group 2
  • GitHub Check: E2E: server mode / ci / group 3
  • GitHub Check: E2E: library mode / ci / group 2
  • GitHub Check: E2E: library mode / ci / group 3
  • GitHub Check: E2E: server mode / ci / group 1
  • GitHub Check: E2E: library mode / ci / group 1
  • GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/**/*.{py,feature}

📄 CodeRabbit inference engine (AGENTS.md)

Use behave (BDD) framework for end-to-end testing

Files:

  • tests/e2e/features/faiss.feature
  • tests/e2e/features/smoketests.feature
  • tests/e2e/features/rlsapi_v1_errors.feature
  • tests/e2e/features/mcp_servers_api_no_config.feature
  • tests/e2e/features/steps/common_http.py
  • tests/e2e/features/inline_rag.feature
  • tests/e2e/features/steps/rlsapi_v1.py
  • tests/e2e/features/models.feature
  • tests/e2e/features/steps/models.py
  • tests/e2e/features/rlsapi_v1.feature
  • tests/e2e/features/query.feature
  • tests/e2e/features/steps/llm_query_response.py
  • tests/e2e/features/steps/token_counters.py
  • tests/e2e/features/streaming_query.feature
  • tests/e2e/features/responses.feature
  • tests/e2e/features/mcp.feature
**/*.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
Check constants.py for shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Use logger = get_logger(__name__) from log.py for 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
Use typing_extensions.Self for model validators in Pydantic models
Use modern union type syntax str | int instead of Union[str, int]
Use Optional[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
Use async def for I/O operations and external API calls
Handle APIConnectionError from 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 @abstractmethod decorators
Use @model_validator and @field_validator for Pydantic model validation
Complete type annotations for all class attributes; use specific types, not Any
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections

Files:

  • tests/e2e/features/steps/common_http.py
  • tests/e2e/features/steps/rlsapi_v1.py
  • tests/e2e/features/steps/models.py
  • tests/e2e/features/steps/llm_query_response.py
  • tests/e2e/features/steps/token_counters.py
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to tests/e2e/**/*.{py,feature} : Use behave (BDD) framework for end-to-end testing
📚 Learning: 2026-04-19T15:40:25.624Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to tests/e2e/test_list.txt : Maintain test list in `tests/e2e/test_list.txt`

Applied to files:

  • tests/e2e/features/smoketests.feature
  • tests/e2e/features/models.feature
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.

Applied to files:

  • tests/e2e/features/mcp_servers_api_no_config.feature
📚 Learning: 2026-04-13T13:39:59.316Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1490
File: tests/e2e/features/environment.py:206-211
Timestamp: 2026-04-13T13:39:59.316Z
Learning: In lightspeed-stack e2e tests (tests/e2e/features/), `context.feature_config` is intentionally set inside Background/step functions (scenario-scoped Behave layer). The `after_scenario` restore logic in `environment.py` only restores config when `context.scenario_lightspeed_override_active` is True, which is only set by `configure_service` when an actual config switch occurs. The module-level `_active_lightspeed_stack_config_basename` in `tests/e2e/features/steps/common.py` prevents re-applying the same config in subsequent scenarios (making `scenario_lightspeed_override_active` stay False). This means the ephemeral nature of step-set context attributes is intentional — the design ensures config restore happens exactly once per actual switch, not redundantly on every scenario.

Applied to files:

  • tests/e2e/features/mcp_servers_api_no_config.feature
📚 Learning: 2025-09-02T11:15:02.411Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.

Applied to files:

  • tests/e2e/features/mcp_servers_api_no_config.feature
📚 Learning: 2026-04-13T13:34:51.052Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1490
File: tests/e2e/features/environment.py:377-381
Timestamp: 2026-04-13T13:34:51.052Z
Learning: In `tests/e2e/features/environment.py` (lightspeed-core/lightspeed-stack), the hardcoded token `eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva` is intentionally a dummy/truncated JWT (the canonical jwt.io documentation example, missing its signature segment). It is not a real credential and does not need to be replaced or loaded from an environment variable. Only a scanner-suppression comment may be warranted if CI noise becomes an issue.

Applied to files:

  • tests/e2e/features/mcp_servers_api_no_config.feature
📚 Learning: 2026-02-19T10:06:58.708Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1181
File: tests/e2e-prow/rhoai/manifests/lightspeed/mock-jwks.yaml:32-34
Timestamp: 2026-02-19T10:06:58.708Z
Learning: In tests/e2e-prow/rhoai/, ConfigMaps like mock-jwks-script and mcp-mock-server-script are created dynamically by the pipeline.sh deployment script using `oc create configmap` commands, rather than being defined as static ConfigMap resources in the manifest YAML files.

Applied to files:

  • tests/e2e/features/mcp_servers_api_no_config.feature
📚 Learning: 2025-08-25T09:05:18.350Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 445
File: tests/e2e/features/health.feature:50-53
Timestamp: 2025-08-25T09:05:18.350Z
Learning: The step definition for ignoring specific fields in response body comparisons is implemented in tests/e2e/features/steps/common_http.py at line 175 as `then('The body of the response, ignoring the "{field}" field, is the following')`.

Applied to files:

  • tests/e2e/features/steps/common_http.py
  • tests/e2e/features/steps/rlsapi_v1.py
  • tests/e2e/features/responses.feature
📚 Learning: 2026-04-07T09:20:26.590Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1467
File: tests/e2e/features/steps/common.py:36-49
Timestamp: 2026-04-07T09:20:26.590Z
Learning: For Behave-based Python tests, rely on Behave’s Context layered stack for attribute lifecycle: Behave pushes a new Context layer when entering feature scope (before_feature) and again for scenario scope (before_scenario). Attributes assigned inside given/when/then steps live on the current scenario layer and are automatically removed when the scenario ends. As a result, step-set attributes should not be expected to persist across scenarios or features, and manual cleanup in after_scenario/after_feature is generally unnecessary for attributes set in step functions. Only perform manual cleanup for attributes that you set explicitly in before_feature/before_scenario, since those live on the respective feature/scenario layers.

Applied to files:

  • tests/e2e/features/steps/common_http.py
  • tests/e2e/features/steps/rlsapi_v1.py
  • tests/e2e/features/steps/models.py
  • tests/e2e/features/steps/llm_query_response.py
  • tests/e2e/features/steps/token_counters.py
📚 Learning: 2026-04-13T13:39:54.963Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1490
File: tests/e2e/features/environment.py:206-211
Timestamp: 2026-04-13T13:39:54.963Z
Learning: In lightspeed-stack E2E tests under tests/e2e/features, it is intentional to set context.feature_config inside Background/step functions (scenario-scoped Behave layer). The environment.py after_scenario restore logic should only restore configuration when context.scenario_lightspeed_override_active is True; this flag is set by configure_service only when a real config switch occurs (so restore does not run for scenarios without a switch). Additionally, steps/common.py’s module-level _active_lightspeed_stack_config_basename is used to prevent re-applying the same config across subsequent scenarios, ensuring scenario_lightspeed_override_active stays False after the first apply. Therefore, reviewers should not “fix” this flow as if feature_config were incorrectly scoped or if after_scenario restoration is missing—config switching and restoration are meant to happen exactly once per actual switch, not redundantly per scenario.

Applied to files:

  • tests/e2e/features/steps/common_http.py
  • tests/e2e/features/steps/rlsapi_v1.py
  • tests/e2e/features/steps/models.py
  • tests/e2e/features/steps/llm_query_response.py
  • tests/e2e/features/steps/token_counters.py
📚 Learning: 2026-04-19T15:40:25.624Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to tests/e2e/**/*.{py,feature} : Use behave (BDD) framework for end-to-end testing

Applied to files:

  • tests/e2e/features/steps/models.py
📚 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/e2e/features/query.feature
  • tests/e2e/features/streaming_query.feature
🔇 Additional comments (9)
tests/e2e/features/mcp.feature (1)

34-445: LGTM — wording-only updates consistent with the rest of the suite.

All MCP scenarios were updated to the new present-tense phrasing for fragment and token-metrics assertions; no scenario flow, payload, or expected status changed. Coverage of step-decorator alignment is verified in the comments on the other feature files.

tests/e2e/features/streaming_query.feature (1)

31-123: LGTM — wording aligned with the rest of the e2e suite.

Streaming query scenarios use the same present-tense phrasing applied across the PR (contains following fragments, token metrics have increased, token metrics are not changed, streamed response contains token counter fields); no test logic is altered. Step-decorator existence is covered by verification on the other feature files.

tests/e2e/features/rlsapi_v1_errors.feature (1)

35-35: Step decorator is properly updated and matches the feature file.

The step decorator @then("The rlsapi response has valid structure") in tests/e2e/features/steps/rlsapi_v1.py correctly matches the wording in the feature file, so the scenario will execute without step implementation errors.

tests/e2e/features/inline_rag.feature (1)

34-42: RAG step decorators correctly registered.

Both The response contains non-empty rag_chunks and The response contains non-empty referenced_documents are registered in tests/e2e/features/steps/llm_query_response.py (lines 200 and 211), and the old "should contain non-empty" variants have been removed.

tests/e2e/features/rlsapi_v1.feature (1)

19-57: All rlsapi step decorators are properly implemented.

Both The rlsapi response has valid structure and The rlsapi request_id is different from the stored one decorators are present in tests/e2e/features/steps/rlsapi_v1.py with matching step definitions, and old "should …" decorators have been removed. Scenarios will execute correctly without conflicts.

tests/e2e/features/responses.feature (1)

423-528: All step decorators already exist and match the feature file usage. No action required.

tests/e2e/features/query.feature (1)

21-95: All renamed step decorators are properly registered.

All four step phrasings used in the feature file have corresponding @then registrations in the steps modules:

  • "The response contains following fragments" (llm_query_response.py:248)
  • "The token metrics have increased" (token_counters.py:46)
  • "The token metrics are not changed" (token_counters.py:73)
  • "The response contains token counter fields" (token_counters.py:13)

The scenarios will bind correctly without failures.

tests/e2e/features/mcp_servers_api_no_config.feature (2)

12-12: The bearer token will be accepted by the configuration. The lightspeed-stack-no-cache.yaml file uses authentication.module: "noop-with-token" with auth_enabled: false at the service level, which means the token mcp-e2e-no-config-token will be accepted without validation against an external service.


13-13: No issues found. The lightspeed-stack-no-cache.yaml configuration (both server-mode and library-mode variants) explicitly has zero mcp_servers sections, which satisfies the scenario's semantic requirement for "no MCP servers configured." The lightspeed-stack-no-mcp.yaml file never existed, making the configuration change appropriate. The test will correctly assert {"servers": []} with this config.

Comment thread tests/e2e/features/responses.feature Outdated
Then The status code of the response is 200
And The responses output should not include any tool invocation item types
And The token metrics should have increased
And The responses output do not include any tool invocation item types
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the registered decorator phrasing in steps modules.
rg -nP --type=py -C2 'responses output (do|does) not include' tests/e2e/features/steps/

Repository: lightspeed-core/lightspeed-stack

Length of output: 990


Subject–verb agreement: output dooutput does.

The subject "responses output" is singular. Lines 406 and 549 read do not include, which is inconsistent with line 527 in the same file (does not include). The corresponding step decorator in tests/e2e/features/steps/llm_query_response.py:39 also uses the incorrect do not include, while the decorator at line 64 correctly uses does not include. Update both the feature file lines and the step decorator to use does not include for grammatical correctness.

📝 Proposed fix

Feature file (lines 406 and 549):

-      And The responses output do not include any tool invocation item types
+      And The responses output does not include any tool invocation item types

Step decorator (tests/e2e/features/steps/llm_query_response.py:39):

-@then("The responses output do not include any tool invocation item types")
+@then("The responses output does not include any tool invocation item types")

Also applies to: 549-549

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/features/responses.feature` at line 406, Update the incorrect
subject–verb agreement "The responses output do not include any tool invocation
item types" to "The responses output does not include any tool invocation item
types" in both occurrences in tests/e2e/features/responses.feature, and update
the matching step decorator string in
tests/e2e/features/steps/llm_query_response.py (the decorator currently using
the same incorrect phrase) so the step text matches the corrected grammar.



@then("The responses output should not include any tool invocation item types")
@then("The responses output do not include any tool invocation item types")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n --type=gherkin -e 'responses output (do|should) (not )?include' tests/e2e/features || \
rg -n -g '*.feature' -e 'responses output (do|should) (not )?include' tests/e2e/features

Repository: lightspeed-core/lightspeed-stack

Length of output: 362


Subject-verb agreement: "output do" → "output does".

Line 39 uses "The responses output do not include", which has a subject-verb mismatch. The singular form should be "output does not include", consistent with the phrasing on line 64.

This step definition is referenced in feature files at tests/e2e/features/responses.feature lines 406 and 549. All three locations must be updated to maintain the exact string match required by the BDD framework.

✏️ Fixes required

In tests/e2e/features/steps/llm_query_response.py line 39:

-@then("The responses output do not include any tool invocation item types")
+@then("The responses output does not include any tool invocation item types")

In tests/e2e/features/responses.feature lines 406 and 549:

-      And The responses output do not include any tool invocation item types
+      And The responses output does not include any tool invocation item types
📝 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
@then("The responses output do not include any tool invocation item types")
`@then`("The responses output does not include any tool invocation item types")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/features/steps/llm_query_response.py` at line 39, The step
decorator string has a subject-verb agreement error: change the `@then` step text
from "The responses output do not include any tool invocation item types" to
"The responses output does not include any tool invocation item types" in the
step definition (the `@then`(...) decorator shown in the diff) and update the
exact same step text in the two feature-file occurrences so the BDD matcher
still finds the step.



@then("The token metrics should not have changed")
@then("The token metrics are not changed")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -g '*.feature' -e 'token metrics (should not|are not|have not) (have )?(changed|change)' tests/e2e/features

Repository: lightspeed-core/lightspeed-stack

Length of output: 507


🏁 Script executed:

cd tests/e2e/features/steps && head -100 token_counters.py | tail -50

Repository: lightspeed-core/lightspeed-stack

Length of output: 1965


🏁 Script executed:

cat -n tests/e2e/features/steps/token_counters.py | sed -n '40,80p'

Repository: lightspeed-core/lightspeed-stack

Length of output: 1865


Update step definition and feature files for consistency: "are not changed" → "have not changed".

Line 73's step definition uses "are not changed" but its own docstring (line 75) already says "have not changed". Additionally, the sibling step on line 46 uses the perfect tense ("have increased") for symmetry and better Gherkin readability.

Update line 73 and corresponding occurrences in feature files:

  • tests/e2e/features/streaming_query.feature (lines 80, 97, 123)
  • tests/e2e/features/query.feature (lines 66, 95)
✏️ Suggested fix
-@then("The token metrics are not changed")
+@then("The token metrics have not changed")
 def check_token_metrics_unchanged(context: Context) -> None:

Update matching step lines in .feature files accordingly.

📝 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
@then("The token metrics are not changed")
`@then`("The token metrics have not changed")
def check_token_metrics_unchanged(context: Context) -> None:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/features/steps/token_counters.py` at line 73, The step decorator
string "@then('The token metrics are not changed')" is inconsistent with its
docstring and sibling steps; update the step definition in
tests/e2e/features/steps/token_counters.py to "@then('The token metrics have not
changed')" and update the matching step text in the feature files
tests/e2e/features/streaming_query.feature (lines around the current step
occurrences) and tests/e2e/features/query.feature so their step sentences read
"The token metrics have not changed" to keep Gherkin tense consistent with the
existing "have increased" step.

Copy link
Copy Markdown
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks ok, TY

@radofuchs radofuchs merged commit 2d0c79a into lightspeed-core:main Apr 27, 2026
26 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants