LCORE-1141: better comments in e2e tests #970
Conversation
WalkthroughThe changes enhance the E2E test environment setup with deployment-mode detection, scenario tag-based filtering, container health management, and configuration switching for specific test scenarios, alongside comprehensive docstring improvements across test utilities and step definitions. Changes
Sequence Diagram(s)sequenceDiagram
participant Test Framework
participant Test Context
participant Service
participant Docker API
participant Config Manager
Note over Test Framework: before_all()
Test Framework->>Service: _fetch_models_from_service()
alt Model detection succeeds
Service-->>Test Context: default_model, default_provider
else Model detection fails
Test Context->>Test Context: Fallback to gpt-4o-mini, openai
end
Note over Test Framework: before_scenario()
Test Framework->>Test Context: Check scenario tags
alt `@skip` or `@local` (not in local mode) or `@skip-in-library-mode`
Test Framework->>Test Framework: Skip scenario
else Config tag present (InvalidFeedbackStorageConfig/NoCacheConfig)
Test Framework->>Config Manager: Switch to scenario-specific config
Config Manager->>Config Manager: Backup original config
end
Note over Test Framework: Scenario Execution
Test Framework->>Test Framework: Execute scenario steps
Note over Test Framework: after_scenario()
alt Config was switched
Test Framework->>Docker API: Restart lightspeed-stack container
Docker API-->>Test Framework: Container restarted
Test Framework->>Config Manager: Restore original config
end
alt llama-stack was disrupted
Test Framework->>Docker API: Restart llama-stack container
loop Health check (multiple attempts)
Docker API-->>Test Framework: Health status
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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/environment.py (1)
53-93: Fix docstring inconsistency with fallback model.Line 63 in the docstring states the fallback is
"gpt-4-turbo", but line 89 in the code setscontext.default_model = "gpt-4o-mini". The docstring must match the actual implementation.Proposed fix for the docstring
Attempts to detect a default LLM model and provider via _fetch_models_from_service() and stores results in context.default_model and context.default_provider; if detection fails, falls back to - "gpt-4-turbo" and "openai". + "gpt-4o-mini" and "openai". Parameters:
🤖 Fix all issues with AI agents
In @tests/e2e/features/steps/health.py:
- Around line 57-65: The docstring claims the step stops a service but the step
implementation is a stub (contains only a TODO and assert), so either implement
the stop logic or adjust the docstring to reflect that it is intentionally
unimplemented; specifically, for the function that contains the TODO and "assert
context is not None", either (A) implement the behavior described in the
docstring by invoking the test harness/service teardown on context (e.g., call
the existing context.stop_service/context.process_manager.stop or terminate the
subprocess associated with the scenario) and remove the TODO, or (B) if you
intend to leave it unimplemented, replace the docstring with a short note that
the step is a stub and raise NotImplementedError to make intent explicit. Ensure
the change updates the docstring and removes the misleading description and TODO
accordingly.
🧹 Nitpick comments (2)
tests/e2e/features/steps/auth.py (1)
11-15: Consider documenting all parameters for consistency.The enhanced docstring describes
header_valuebut omits thecontextparameter. Other enhanced docstrings in this PR document all parameters includingcontext(e.g.,health.py,common.py). Adding it would improve consistency.Suggested enhancement for consistency
"""Set a custom Authorization header value. Parameters: + context (Context): Behave context object storing auth_headers. header_value (str): The value to set for the `Authorization` header. """tests/e2e/utils/utils.py (1)
63-79: Consider reordering docstring sections for consistency.The docstring content is excellent and comprehensive. However, the Returns section (lines 73-74) appears before the Parameters section (lines 76-78). Python docstring conventions typically place Parameters before Returns for consistency.
Suggested section reordering
"""Wait for container to be healthy. Polls a Docker container until its health status becomes `healthy` or the attempt limit is reached. Checks the container's `Health.Status` using `docker inspect` up to `max_attempts`, printing progress and final status messages. Transient inspect errors or timeouts are ignored and retried; the function returns after the container is observed healthy or after all attempts complete. -Returns: - None - Parameters: container_name (str): Docker container name or ID to check. max_attempts (int): Maximum number of health check attempts (default 3). + +Returns: + None """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/e2e/features/environment.pytests/e2e/features/steps/auth.pytests/e2e/features/steps/common.pytests/e2e/features/steps/health.pytests/e2e/utils/utils.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/e2e/features/steps/health.pytests/e2e/utils/utils.pytests/e2e/features/environment.pytests/e2e/features/steps/common.pytests/e2e/features/steps/auth.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/e2e/features/**/*.feature : Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/e2e/test_list.txt : Maintain test list in `tests/e2e/test_list.txt` for end-to-end tests
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
tests/e2e/features/steps/health.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/auth.py
⏰ 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). (4)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (16)
tests/e2e/features/steps/health.py (1)
11-26: LGTM!The enhanced docstring is comprehensive, well-structured, and accurately describes the container disruption behavior including state tracking and error handling.
tests/e2e/features/steps/common.py (2)
10-23: Enhanced docstring with new context attributes added.The docstring improvement is clear and helpful. Note that lines 21-22 add new
hostname_llamaandport_llamacontext attributes beyond just documentation enhancement, which extends the test infrastructure for llama-stack endpoint configuration.
27-37: LGTM!The enhanced docstring is clear, well-structured, and includes appropriate Parameters and Raises sections.
tests/e2e/features/steps/auth.py (1)
36-39: LGTM!The parameter descriptions are clear and accurately describe the normalization behavior and quote removal.
tests/e2e/utils/utils.py (8)
13-29: LGTM!The enhanced docstring is clear and comprehensive, accurately describing the normalization behavior.
33-59: Enhanced docstring with improved error messages.The docstring enhancement is excellent. Note that lines 56-59 also improve the assertion messages to distinguish between validation errors and schema errors, which is a functional improvement beyond pure documentation that enhances debugging.
118-123: LGTM!The added Returns and Raises sections are clear and accurately describe the recursive validation behavior and failure conditions.
151-168: LGTM!The enhanced docstring is thorough and well-structured, with detailed exception documentation that will help developers handle errors appropriately.
177-189: LGTM!The enhanced docstring clearly documents the backup creation behavior including the conditional logic and potential exceptions.
201-217: Enhanced docstring with added error logging.The docstring enhancement is clear and helpful. Note that line 217 adds a warning print statement on removal failure, which is a functional improvement beyond pure documentation that enhances observability.
221-242: Enhanced docstring with significant functional addition.The docstring enhancement accurately reflects the function's behavior. However, lines 241-242 add a
wait_for_container_healthcall that represents a significant functional change beyond documentation improvement. This new behavior ensures the container is healthy before returning, which may affect test timing but improves reliability.This functional addition goes beyond the PR's stated goal of "better comments in e2e tests."
Consider whether this functional change should be:
- Documented in the PR description as a behavior change, or
- Split into a separate commit/PR focused on functional improvements
248-250: LGTM!The enhanced parameter descriptions are clear and accurately describe the context attributes and placeholder replacement behavior.
tests/e2e/features/environment.py (4)
97-108: LGTM!The enhanced docstring clearly describes the scenario filtering logic and configuration selection based on tags and deployment mode.
132-156: LGTM!The enhanced docstring is comprehensive and clearly documents the teardown process, including configuration restoration and llama-stack health verification logic.
217-220: LGTM!The enhanced docstring appropriately describes the feature-level setup and configuration application.
237-241: LGTM!The enhanced docstring clearly describes the feature-level teardown, configuration restoration, and feedback cleanup.
| """Stop service. | ||
| Stop a service used by the current test scenario. | ||
| Parameters: | ||
| context (Context): Behave step context carrying scenario state and configuration. | ||
| """ | ||
| # TODO: add step implementation | ||
| assert context is not None |
There was a problem hiding this comment.
Docstring describes unimplemented behavior.
The enhanced docstring describes stopping a service, but the implementation is just a stub with a TODO comment. The docstring should either reflect the stub nature of the function or the implementation should be completed.
Suggested docstring for the current stub implementation
- """Stop service.
-
- Stop a service used by the current test scenario.
-
- Parameters:
- context (Context): Behave step context carrying scenario state and configuration.
- """
+ """Placeholder for stopping a service.
+
+ TODO: Implement service stop functionality.
+
+ Parameters:
+ context (Context): Behave step context carrying scenario state and configuration.
+ """🤖 Prompt for AI Agents
In @tests/e2e/features/steps/health.py around lines 57 - 65, The docstring
claims the step stops a service but the step implementation is a stub (contains
only a TODO and assert), so either implement the stop logic or adjust the
docstring to reflect that it is intentionally unimplemented; specifically, for
the function that contains the TODO and "assert context is not None", either (A)
implement the behavior described in the docstring by invoking the test
harness/service teardown on context (e.g., call the existing
context.stop_service/context.process_manager.stop or terminate the subprocess
associated with the scenario) and remove the TODO, or (B) if you intend to leave
it unimplemented, replace the docstring with a short note that the step is a
stub and raise NotImplementedError to make intent explicit. Ensure the change
updates the docstring and removes the misleading description and TODO
accordingly.
1 participant
Description
LCORE-1141: better comments in e2e tests
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.