LCORE-1472: remove unnecessary steps#1538
Conversation
WalkthroughThe PR refactors shield disabling from tag-based annotations to explicit scenario steps, consolidates multiple HTTP step definitions into a single generalized REST API endpoint helper, and updates environment setup to manage cleanup via context flags rather than feature tags. Feedback conversation cleanup registration is added to track conversations for teardown. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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
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/steps/common_http.py (1)
208-233: 🧹 Nitpick | 🔵 TrivialNormalize
methodbefore branching on it.
methodis taken verbatim from the Gherkin step, so"Get"or"get"would skip theGET/DELETEbranch and then fail thecontext.text is not Noneassertion (or try tojson.loadsa missing payload) even thoughrequests.requestitself is case-insensitive. Uppercasing once keeps the step tolerant and intent clear.♻️ Proposed fix
`@when`("I access REST API endpoint {endpoint} using HTTP {method} method") def access_rest_api_endpoint(context: Context, endpoint: str, method: str) -> None: """Send HTTP request with JSON payload to tested service. The JSON payload is retrieved from `context.text` attribute, which must not be None. The response is stored in `context.response` attribute. """ endpoint = normalize_endpoint(endpoint) base = f"http://{context.hostname}:{context.port}" path = f"{context.api_prefix}/{endpoint}".replace("//", "/") url = base + path - if method in ["GET", "DELETE"]: + method = method.upper() + if method in ("GET", "DELETE"): data = None else: assert context.text is not None, "Payload needs to be specified" data = json.loads(context.text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/steps/common_http.py` around lines 208 - 233, The branching on HTTP method in access_rest_api_endpoint uses the raw Gherkin-provided method string and should be normalized first; update access_rest_api_endpoint to convert the incoming method to a canonical form (e.g., method = method.upper()) immediately after receiving the parameter and before the if method in ["GET", "DELETE"] check so that case-insensitive inputs like "Get" or "get" follow the correct branch and payload handling.
🤖 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/environment.py`:
- Around line 163-170: The manual cleanup loop that deletes context attributes
"shields_disabled_for_scenario", "llama_guard_provider_id", and
"llama_guard_provider_shield_id" is redundant because these are set by the
`@given`("shields are disabled for this scenario") step and live on Behave’s
scenario context; remove the for-loop that calls hasattr/delattr in
environment.py (the block referencing those three attribute names) so the
scenario-layer attributes are allowed to be discarded automatically, and only
re-introduce explicit deletion if you can demonstrate those attributes are being
set on a higher layer outside the shields_are_disabled_for_scenario step.
---
Outside diff comments:
In `@tests/e2e/features/steps/common_http.py`:
- Around line 208-233: The branching on HTTP method in access_rest_api_endpoint
uses the raw Gherkin-provided method string and should be normalized first;
update access_rest_api_endpoint to convert the incoming method to a canonical
form (e.g., method = method.upper()) immediately after receiving the parameter
and before the if method in ["GET", "DELETE"] check so that case-insensitive
inputs like "Get" or "get" follow the correct branch and payload handling.
🪄 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: 0d2a930e-3014-46cd-b6dd-4d64c409a54f
📒 Files selected for processing (9)
tests/e2e/features/environment.pytests/e2e/features/query.featuretests/e2e/features/steps/common_http.pytests/e2e/features/steps/feedback.pytests/e2e/features/steps/health.pytests/e2e/features/steps/llm_query_response.pytests/e2e/features/steps/shields.pytests/e2e/features/streaming_query.featuretests/e2e/utils/llama_stack_utils.py
💤 Files with no reviewable changes (2)
- tests/e2e/features/steps/llm_query_response.py
- tests/e2e/features/steps/health.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). (12)
- GitHub Check: build-pr
- GitHub Check: unit_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: Pylinter
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 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/query.featuretests/e2e/features/streaming_query.featuretests/e2e/features/steps/shields.pytests/e2e/features/steps/feedback.pytests/e2e/utils/llama_stack_utils.pytests/e2e/features/environment.pytests/e2e/features/steps/common_http.py
**/*.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/e2e/features/steps/shields.pytests/e2e/features/steps/feedback.pytests/e2e/utils/llama_stack_utils.pytests/e2e/features/environment.pytests/e2e/features/steps/common_http.py
🧠 Learnings (4)
📚 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/shields.pytests/e2e/features/steps/feedback.pytests/e2e/features/environment.pytests/e2e/features/steps/common_http.py
📚 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/shields.pytests/e2e/features/steps/feedback.pytests/e2e/features/environment.pytests/e2e/features/steps/common_http.py
📚 Learning: 2025-09-02T11:14:17.117Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/steps/common_http.py:244-255
Timestamp: 2025-09-02T11:14:17.117Z
Learning: The POST step in tests/e2e/features/steps/common_http.py (`access_rest_api_endpoint_post`) is intentionally designed as a general-purpose HTTP POST method, not specifically for REST API endpoints, so it should not include context.api_prefix in the URL construction.
Applied to files:
tests/e2e/features/steps/feedback.pytests/e2e/features/steps/common_http.py
📚 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
🪛 Betterleaks (1.1.2)
tests/e2e/features/environment.py
[high] 392-392: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (2)
tests/e2e/features/steps/shields.py (1)
1-37: LGTM!Clean scenario-scoped implementation: library mode is skipped explicitly, server-mode failure is converted to a scenario skip, and the three context attributes (
shields_disabled_for_scenario,llama_guard_provider_id,llama_guard_provider_shield_id) match whatenvironment.after_scenarioexpects for re-registration. Cleanup of these attributes atbefore_scenariokeeps the flow deterministic across scenarios.tests/e2e/features/environment.py (1)
372-397: Step-set feedback attributes won't survive intoafter_feature— cleanup is likely ineffective.
context.feedback_e2e_conversation_cleanupandcontext.feedback_conversationsare set only inside step functions (feedback.py lines 29–31, 155). Per Behave's documented layer mechanics, attributes set ingiven/when/thensteps are stored on the scenario layer and are automatically removed when the scenario ends. Sincefeedback.featurehas multiple scenarios that each create conversations, only the last scenario's attributes will be visible whenafter_featureruns — intermediate scenarios' conversations will never be deleted.The
before_featurecleanup (lines 373–375) is also ineffective: it only deletes attributes that somehow survived between features, not cleanup within the same feature's scenarios.Fix: Either initialize these as feature-layer attributes in
before_feature(set an empty list/flag that step functions mutate), or move them to module-level infeedback.py(similar to how_active_lightspeed_stack_config_basenameis used incommon.py).
| # Clear shield unregister state from previous scenarios (see ``shields_are_disabled_for_scenario``). | ||
| for _attr in ( | ||
| "shields_disabled_for_scenario", | ||
| "llama_guard_provider_id", | ||
| "llama_guard_provider_shield_id", | ||
| ): | ||
| if hasattr(context, _attr): | ||
| delattr(context, _attr) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Manual cleanup likely redundant here.
shields_disabled_for_scenario, llama_guard_provider_id, and llama_guard_provider_shield_id are assigned inside the @given("shields are disabled for this scenario") step, so they sit on Behave’s scenario layer and are already discarded when the scenario ends. Unless you’re guarding against something else setting them on a higher layer, this delattr loop can be dropped.
Based on learnings: “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.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/features/environment.py` around lines 163 - 170, The manual cleanup
loop that deletes context attributes "shields_disabled_for_scenario",
"llama_guard_provider_id", and "llama_guard_provider_shield_id" is redundant
because these are set by the `@given`("shields are disabled for this scenario")
step and live on Behave’s scenario context; remove the for-loop that calls
hasattr/delattr in environment.py (the block referencing those three attribute
names) so the scenario-layer attributes are allowed to be discarded
automatically, and only re-introduce explicit deletion if you can demonstrate
those attributes are being set on a higher layer outside the
shields_are_disabled_for_scenario step.
2 participants
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
Release Notes
Tests
Chores