LCORE-492: E2E tests for feedback endpoint #613
Conversation
WalkthroughRefactors e2e config management into modular helpers, adds an invalid-feedback-storage LCS config, expands feedback feature tests and step implementations (enable/disable/status/submit flows), enhances HTTP step helpers with header support and REST POST/PUT, adds partial JSON validation, and introduces auth header removal and feedback cleanup hooks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Behave Runner
participant Env as environment.py
participant Utils as tests.e2e.utils
participant Docker as Docker
Runner->>Env: before_feature(feature)
Env->>Utils: create_config_backup(feature_config)
Env->>Utils: switch_config(feature_config)
Env->>Utils: restart_container("lightspeed-stack")
Utils->>Docker: restart container & wait_for_container_health
Docker-->>Utils: healthy
Runner->>Env: before_scenario(scenario)
Note over Env: scenario may set invalid_feedback config
Runner->>Env: after_scenario(scenario)
Env->>Utils: switch_config(default_config_backup)
Env->>Utils: restart_container("lightspeed-stack")
Runner->>Env: after_feature(feature)
Env->>Env: DELETE created feedback conversations via requests
Env->>Utils: remove_config_backup(backup)
sequenceDiagram
autonumber
participant Steps as feedback.steps
participant API as Lightspeed Stack API
participant Store as Feedback Storage
Steps->>API: PUT /feedback/status (enable/disable) [Auth header]
API-->>Steps: 200 | 4xx/5xx
Steps->>API: GET /feedback/status [Auth header]
API-->>Steps: 200 with status
Steps->>API: POST /conversations ... (init)
API-->>Steps: 200 {conversation_id}
Steps->>API: POST /feedback {conversation_id, sentiment, note} [Auth]
API->>Store: persist feedback
Store-->>API: success | error
API-->>Steps: 200 | 400/403/422/500
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 3
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)
122-129: Avoid hardcoded localhost:8080; use context host/port/prefix and add timeoutHardcoding the base URL can break non-local or differently-configured runs and may hang without a timeout. Build from context and set a timeout.
Apply this diff:
- for conversation_id in context.feedback_conversations: - url = f"http://localhost:8080/v1/conversations/{conversation_id}" - headers = context.auth_headers if hasattr(context, "auth_headers") else {} - response = requests.delete(url, headers=headers) - assert response.status_code == 200, url + for conversation_id in context.feedback_conversations: + host = getattr(context, "hostname", "localhost") + port = getattr(context, "port", 8080) + prefix = getattr(context, "rest_api_prefix", "/v1") + if not prefix.startswith("/"): + prefix = "/" + prefix + url = f"http://{host}:{port}{prefix}/conversations/{conversation_id}" + headers = getattr(context, "auth_headers", {}) + response = requests.delete(url, headers=headers, timeout=10) + assert response.status_code == 200, url
🧹 Nitpick comments (8)
tests/e2e/utils/utils.py (1)
69-76: Fix pydocstyle warnings and strengthen partial JSON validatorAddress D205/D400/D401 and add clearer error messages and typing for validate_json_partially. This should resolve the pipeline warnings and make failures more actionable.
Apply this diff:
-def validate_json_partially(actual: Any, expected: Any): - """ - Recursively validate that `actual` JSON contains all keys and values - specified in `expected`. Extra elements/keys are ignored. - - Raises AssertionError if validation fails. - """ +def validate_json_partially(actual: Any, expected: Any) -> None: + """Validate that actual JSON contains all keys/values from expected. + + Recursively checks dicts and lists; extra keys/elements in actual are ignored. + + Raises: + AssertionError: If validation fails. + """ if isinstance(expected, dict): - for key, expected_value in expected.items(): + assert isinstance(actual, dict), ( + f"Type mismatch: expected dict at this level, got {type(actual).__name__}" + ) + for key, expected_value in expected.items(): assert key in actual, f"Missing key in JSON: {key}" validate_json_partially(actual[key], expected_value) elif isinstance(expected, list): - for schema_item in expected: + assert isinstance(actual, list), ( + f"Type mismatch: expected list at this level, got {type(actual).__name__}" + ) + for schema_item in expected: matched = False for item in actual: try: validate_json_partially(item, schema_item) matched = True break except AssertionError: continue assert ( matched - ), f"No matching element found in list for schema item {schema_item}" + ), f"No matching element found in list for schema item {schema_item!r}" else: assert actual == expected, f"Value mismatch: expected {expected}, got {actual}" -def switch_config( +def switch_config( source_path: str, destination_path: str = "lightspeed-stack.yaml" ) -> None: - """Overwrites the config in `destination_path` by `source_path`.""" + """Overwrite the config at destination_path with source_path.""" try: shutil.copy(source_path, destination_path) except (FileNotFoundError, PermissionError, OSError) as e: print(f"Failed to copy replacement file: {e}") raise -def create_config_backup(config_path: str) -> str: - """Creates a backup of `config_path` if it does not already exist.""" +def create_config_backup(config_path: str) -> str: + """Create a backup of config_path if it does not already exist.""" backup_file = f"{config_path}.backup" if not os.path.exists(backup_file): try: shutil.copy(config_path, backup_file) except (FileNotFoundError, PermissionError, OSError) as e: print(f"Failed to create backup: {e}") raise return backup_file -def remove_config_backup(backup_path: str) -> None: - """Deletes the backup file at `backup_path` if it exists.""" +def remove_config_backup(backup_path: str) -> None: + """Delete the backup file at backup_path if it exists.""" if os.path.exists(backup_path): try: os.remove(backup_path) except OSError as e: print(f"Warning: Could not remove backup file {backup_path}: {e}")This addresses D205, D400, and D401 warnings in the pipeline.
As per coding guidelinesAlso applies to: 99-107, 110-120, 122-129
tests/e2e/features/steps/feedback.py (6)
34-36: Fix D401: Use imperative mood in docstringRephrase the first sentence.
-def set_feedback(context: Context) -> None: - """Internal helper to enable or disable feedback via PUT request.""" +def set_feedback(context: Context) -> None: + """Enable or disable feedback via PUT request."""As per coding guidelines
54-54: Typo in docstring ("previousl")Fix spelling.
- """Submit feedback for previousl created conversation.""" + """Submit feedback for previously created conversation."""
90-93: Preserve existing step and add a correctly spelled aliasThe step string has a typo (“retreive”). Add an alias with the correct spelling to avoid breaking existing features.
-@when("I retreive the current feedback status") # type: ignore +@when("I retreive the current feedback status") # type: ignore +@when("I retrieve the current feedback status") # type: ignore def access_feedback_get_endpoint(context: Context) -> None: - """Retrieve the current feedback status via GET request.""" + """Retrieve the current feedback status via GET request."""
6-7: Tighten type annotations for payloadsUse precise dict types. Import Any.
import requests +from typing import Any import json @@ -def access_feedback_put_endpoint(context: Context, payload: dict) -> None: +def access_feedback_put_endpoint(context: Context, payload: dict[str, Any]) -> None: @@ -def access_feedback_post_endpoint( - context: Context, conversation_id: str | None -) -> None: +def access_feedback_post_endpoint( + context: Context, conversation_id: str | None +) -> None: """Send POST HTTP request with JSON payload to tested service."""As per coding guidelines
Also applies to: 40-41, 75-77
118-123: Avoid hardcoded container nameUse a context-provided container name with a safe default.
def configure_invalid_feedback_storage_path(context: Context) -> None: """Set an invalid feedback storage path and restart the container.""" switch_config(context.scenario_config) - restart_container("lightspeed-stack") + container_name = getattr(context, "container_name", "lightspeed-stack") + restart_container(container_name)
40-49: Reduce duplication by reusing REST helpersYou can reuse access_rest_api_endpoint_put/post from common_http instead of duplicating URL/header logic here (set context.text to payload JSON, then call the helper). Keeps step code thinner.
Also applies to: 75-88
tests/e2e/features/steps/common_http.py (1)
366-372: Add docstring and validate expected structure is providedAddress D103 and guard against missing context.text.
-@then("And the body of the response has the following structure") -def check_response_partially(context: Context) -> None: - assert context.response is not None, "Request needs to be performed first" - body = context.response.json() - expected = json.loads(context.text or "{}") - validate_json_partially(body, expected) +@then("And the body of the response has the following structure") +def check_response_partially(context: Context) -> None: + """Validate the response body partially against the expected structure in context.text.""" + assert context.response is not None, "Request needs to be performed first" + assert context.text is not None, "Expected partial structure must be provided" + body = context.response.json() + expected = json.loads(context.text) + validate_json_partially(body, expected)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/e2e/configuration/lightspeed-stack-invalid-feedback-storage.yaml(1 hunks)tests/e2e/features/environment.py(3 hunks)tests/e2e/features/feedback.feature(1 hunks)tests/e2e/features/steps/auth.py(1 hunks)tests/e2e/features/steps/common_http.py(4 hunks)tests/e2e/features/steps/feedback.py(1 hunks)tests/e2e/utils/utils.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/e2e/features/steps/auth.pytests/e2e/utils/utils.pytests/e2e/features/steps/common_http.pytests/e2e/features/environment.pytests/e2e/features/steps/feedback.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place behave step definitions under tests/e2e/features/steps/
Files:
tests/e2e/features/steps/auth.pytests/e2e/features/steps/common_http.pytests/e2e/features/steps/feedback.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/e2e/features/steps/auth.pytests/e2e/utils/utils.pytests/e2e/features/steps/common_http.pytests/e2e/features/environment.pytests/e2e/features/steps/feedback.py
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/feedback.feature
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:14:17.117Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#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/common_http.py
🧬 Code graph analysis (3)
tests/e2e/features/steps/common_http.py (2)
tests/e2e/utils/utils.py (3)
normalize_endpoint(11-16)validate_json(19-31)validate_json_partially(69-96)tests/e2e/features/steps/auth.py (1)
access_rest_api_endpoint_post(26-45)
tests/e2e/features/environment.py (1)
tests/e2e/utils/utils.py (4)
switch_config(99-107)restart_container(131-145)remove_config_backup(122-128)create_config_backup(110-119)
tests/e2e/features/steps/feedback.py (2)
tests/e2e/utils/utils.py (2)
switch_config(99-107)restart_container(131-145)tests/e2e/features/steps/common_http.py (1)
access_rest_api_endpoint_get(236-247)
🪛 GitHub Actions: Pydocstyle
tests/e2e/utils/utils.py
[warning] 70-70: D205: 1 blank line required between summary line and description (found 0)
[warning] 70-70: D400: First line should end with a period (not 's')
[warning] 102-102: D401: First line should be in imperative mood (perhaps 'Overwrite', not 'Overwrites')
[warning] 111-111: D401: First line should be in imperative mood (perhaps 'Create', not 'Creates')
[warning] 123-123: D401: First line should be in imperative mood (perhaps 'Delete', not 'Deletes')
tests/e2e/features/steps/common_http.py
[warning] 367-367: D103: Missing docstring in public function
tests/e2e/features/steps/feedback.py
[warning] 34-34: D401: First line should be in imperative mood; try rephrasing (found 'Internal')
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (8)
tests/e2e/configuration/lightspeed-stack-invalid-feedback-storage.yaml (1)
19-25: Verify auth settings vs @Authorized tag inheritanceThis invalid-storage config sets service.auth_enabled: false while the feature carries @Authorized. If the scenario step switches to this config, auth checks will be disabled for the duration. Confirm this won’t mask auth-related expectations in that scenario or subsequent cleanup. If needed, set auth_enabled: true for consistency with @Authorized.
tests/e2e/features/steps/auth.py (1)
18-23: LGTM: header removal stepCleanly removes Authorization from context.auth_headers if present. Works with existing header propagation.
tests/e2e/features/environment.py (2)
41-45: Ensure the step applies context.scenario_configYou set context.scenario_config for @InvalidFeedbackStorageConfig, but do not switch configs here. Please verify the step “An invalid feedback storage path is configured” calls switch_config(context.scenario_config) and restart_container, otherwise the scenario won’t actually use the invalid config.
104-110: LGTM: modular config switch with backup/restoreBackup, switch, restart in before_feature, and restore+cleanup in after_feature look correct and cohesive.
Also applies to: 118-121
tests/e2e/features/feedback.feature (1)
69-69: Fix typos in feedback.feature
- Line 69 & 84:
“When I retreive the current feedback status” → “When I retrieve the current feedback status”- Line 200:
“Scenario: Check if feedback submittion fails when invald sentiment is passed” → “Scenario: Check if feedback submission fails when invalid sentiment is passed”- Line 227:
“Scenario: Check if feedback submittion fails when nonexisting conversation ID is passed” → “Scenario: Check if feedback submission fails when nonexistent conversation ID is passed”- Line 270:
“Scenario: Check if feedback submittion fails when invalid feedback storage path is configured” → “Scenario: Check if feedback submission fails when invalid feedback storage path is configured”Confirm that each corrected phrase exactly matches its step definition to avoid binding mismatches.
tests/e2e/features/steps/common_http.py (2)
242-248: Auth headers added to REST GET look goodHeader propagation via context.auth_headers is correct and consistent.
251-273: Non‑REST POST handler rename and header usage are correctNo api_prefix in URL and headers included. Matches prior design. Based on learnings.
tests/e2e/features/steps/feedback.py (1)
114-115: Resolved: context.feedback_conversations is initialized
Initialization exists in tests/e2e/features/environment.py:112, so no changes needed.
| Given The system is in default state | ||
| When The feedback is enabled | ||
| Then The status code of the response is 200 | ||
| And And the body of the response has the following structure |
There was a problem hiding this comment.
Fix duplicated “And” in steps — breaks step matching
The literal “And And …” won’t match step definitions. Remove the extra “And”.
Apply this diff:
- And And the body of the response has the following structure
+ And the body of the response has the following structure
- And And the body of the response has the following structure
+ And the body of the response has the following structure
- And And the body of the response has the following structure
+ And the body of the response has the following structure
- And And the body of the response has the following structure
+ And the body of the response has the following structure
- And And the body of the response has the following structure
+ And the body of the response has the following structureAlso applies to: 31-31, 49-49, 126-126, 214-214
🤖 Prompt for AI Agents
In tests/e2e/features/feedback.feature around lines 16, 31, 49, 126, and 214,
several steps contain a duplicated "And And" prefix which prevents proper step
matching; edit each occurrence to remove the extra "And" so the lines start with
a single "And" (e.g., change "And And the body of the response..." to "And the
body of the response...") and save the feature file.
| response = requests.put(url, headers=headers, json=payload) | ||
| context.response = response |
There was a problem hiding this comment.
Add timeouts to all HTTP calls to avoid hangs
External calls lack timeouts. Use DEFAULT_TIMEOUT consistently to prevent blocking tests. Based on learnings.
- response = requests.put(url, headers=headers, json=payload)
+ response = requests.put(url, headers=headers, json=payload, timeout=DEFAULT_TIMEOUT)
@@
- context.response = requests.post(url, headers=headers, json=payload)
+ context.response = requests.post(url, headers=headers, json=payload, timeout=DEFAULT_TIMEOUT)
@@
- response = requests.post(url, headers=headers, json=payload)
+ response = requests.post(url, headers=headers, json=payload, timeout=DEFAULT_TIMEOUT)Also applies to: 87-87, 106-106
🤖 Prompt for AI Agents
In tests/e2e/features/steps/feedback.py around lines 48-49 (and also at lines 87
and 106), HTTP requests are made without timeouts which can hang tests; update
each requests call to pass timeout=DEFAULT_TIMEOUT, and ensure DEFAULT_TIMEOUT
is imported or defined (e.g., from your test helpers/config where
DEFAULT_TIMEOUT is declared). Add the timeout argument to the
requests.put/requests.get/requests.post calls at the specified lines and import
DEFAULT_TIMEOUT at the top of the file if not already present.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM, but the RabbitAI comment about POST/PUT incoherecny would need to be solved.
asimurka
force-pushed
the
feedback_e2e_tests
branch
from
0ae898c to
37c932a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/e2e/features/feedback.feature (1)
16-16: Remove duplicated step keywordsPlease drop the extra leading “And” in these steps so the Gherkin text reads naturally and we can keep the step definition phrased without the redundant keyword.
- And And the body of the response has the following structure + And the body of the response has the following structureApply the same edit to every occurrence listed above so the feature stays consistent.
Also applies to: 30-30, 49-49, 126-126, 214-214
tests/e2e/features/steps/feedback.py (1)
48-49: Use timeouts on feedback HTTP callsWe still issue PUT/POST requests here without a timeout, so a hung endpoint will freeze the scenario. The earlier review called this out—please wire
timeout=DEFAULT_TIMEOUTinto every feedbackrequestscall.- response = requests.put(url, headers=headers, json=payload) + response = requests.put( + url, headers=headers, json=payload, timeout=DEFAULT_TIMEOUT + ) @@ - context.response = requests.post(url, headers=headers, json=payload) + context.response = requests.post( + url, headers=headers, json=payload, timeout=DEFAULT_TIMEOUT + ) @@ - response = requests.post(url, headers=headers, json=payload) + response = requests.post( + url, headers=headers, json=payload, timeout=DEFAULT_TIMEOUT + )Also applies to: 87-87, 106-106
🧹 Nitpick comments (1)
tests/e2e/features/steps/common_http.py (1)
367-377: Rename the step to drop the redundant “And”Once the feature lines lose the duplicated keyword, this step should be registered as “The body of the response…” rather than encoding the conjunction in the pattern. That keeps the wording consistent across scenarios and helpers.
-@then("And the body of the response has the following structure") +@then("The body of the response has the following structure")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/e2e/configuration/lightspeed-stack-invalid-feedback-storage.yaml(1 hunks)tests/e2e/features/environment.py(3 hunks)tests/e2e/features/feedback.feature(1 hunks)tests/e2e/features/steps/auth.py(1 hunks)tests/e2e/features/steps/common_http.py(4 hunks)tests/e2e/features/steps/feedback.py(1 hunks)tests/e2e/utils/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/features/steps/auth.py
- tests/e2e/configuration/lightspeed-stack-invalid-feedback-storage.yaml
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/e2e/utils/utils.pytests/e2e/features/steps/common_http.pytests/e2e/features/steps/feedback.pytests/e2e/features/environment.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/e2e/utils/utils.pytests/e2e/features/steps/common_http.pytests/e2e/features/steps/feedback.pytests/e2e/features/environment.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place behave step definitions under tests/e2e/features/steps/
Files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/feedback.py
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/feedback.feature
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:14:17.117Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#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/common_http.py
🧬 Code graph analysis (3)
tests/e2e/features/steps/common_http.py (2)
tests/e2e/utils/utils.py (3)
normalize_endpoint(11-16)validate_json(19-31)validate_json_partially(69-94)tests/e2e/features/steps/auth.py (1)
access_rest_api_endpoint_post(26-45)
tests/e2e/features/steps/feedback.py (2)
tests/e2e/utils/utils.py (2)
switch_config(97-105)restart_container(129-143)tests/e2e/features/steps/common_http.py (1)
access_rest_api_endpoint_get(236-247)
tests/e2e/features/environment.py (1)
tests/e2e/utils/utils.py (4)
switch_config(97-105)restart_container(129-143)remove_config_backup(120-126)create_config_backup(108-117)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
| print(context.feedback_conversations) | ||
| for conversation_id in context.feedback_conversations: | ||
| url = f"http://localhost:8080/v1/conversations/{conversation_id}" | ||
| headers = context.auth_headers if hasattr(context, "auth_headers") else {} | ||
| response = requests.delete(url, headers=headers) | ||
| assert response.status_code == 200, url |
There was a problem hiding this comment.
Add timeouts to cleanup DELETE calls
This cleanup DELETE runs without a timeout, so if the service stalls we hang the entire suite. Please add a shared timeout (mirroring the other HTTP helpers) before asserting the response.
import requests
+
+DEFAULT_TIMEOUT = 10
@@
- response = requests.delete(url, headers=headers)
+ response = requests.delete(
+ url, headers=headers, timeout=DEFAULT_TIMEOUT
+ )📝 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.
| print(context.feedback_conversations) | |
| for conversation_id in context.feedback_conversations: | |
| url = f"http://localhost:8080/v1/conversations/{conversation_id}" | |
| headers = context.auth_headers if hasattr(context, "auth_headers") else {} | |
| response = requests.delete(url, headers=headers) | |
| assert response.status_code == 200, url | |
| ++ b/tests/e2e/features/environment.py | |
| @@ | |
| import requests | |
| DEFAULT_TIMEOUT = 10 | |
| @@ | |
| response = requests.delete( | |
| url, | |
| headers=headers, | |
| timeout=DEFAULT_TIMEOUT | |
| ) | |
| assert response.status_code == 200, url |
🤖 Prompt for AI Agents
In tests/e2e/features/environment.py around lines 123 to 128, the cleanup DELETE
loop calls requests.delete without any timeout which can hang the entire suite
if the service stalls; update the call to include the shared timeout used by
other HTTP helpers (e.g. use context.http_timeout or a module-level
HTTP_TIMEOUT) by passing it as the timeout parameter to requests.delete, then
assert the response.status_code as before.
| def validate_json_partially(actual: Any, expected: Any): | ||
| """Recursively validate that `actual` JSON contains all keys and values specified in `expected`. | ||
| Args: | ||
| original_file: Path to the original configuration file | ||
| replacement_file: Path to the replacement configuration file | ||
| container_name: Name of the container to restart | ||
| cleanup: If True, remove the backup file after restoration (default: False) | ||
| Returns: | ||
| str: Path to the backup file for restoration | ||
| Extra elements/keys are ignored. Raises AssertionError if validation fails. | ||
| """ | ||
| backup_file = f"{original_file}.backup" | ||
| if isinstance(expected, dict): | ||
| for key, expected_value in expected.items(): | ||
| assert key in actual, f"Missing key in JSON: {key}" | ||
| validate_json_partially(actual[key], expected_value) | ||
|
|
||
| elif isinstance(expected, list): | ||
| for schema_item in expected: | ||
| matched = False | ||
| for item in actual: | ||
| try: | ||
| validate_json_partially(item, schema_item) | ||
| matched = True | ||
| break | ||
| except AssertionError: | ||
| continue | ||
| assert ( | ||
| matched | ||
| ), f"No matching element found in list for schema item {schema_item}" | ||
|
|
||
| else: | ||
| assert actual == expected, f"Value mismatch: expected {expected}, got {actual}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add return annotation to validate_json_partially
The coding guidelines require complete type annotations. Please declare the return type explicitly.
As per coding guidelines
-def validate_json_partially(actual: Any, expected: Any):
+def validate_json_partially(actual: Any, expected: Any) -> None:📝 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.
| def validate_json_partially(actual: Any, expected: Any): | |
| """Recursively validate that `actual` JSON contains all keys and values specified in `expected`. | |
| Args: | |
| original_file: Path to the original configuration file | |
| replacement_file: Path to the replacement configuration file | |
| container_name: Name of the container to restart | |
| cleanup: If True, remove the backup file after restoration (default: False) | |
| Returns: | |
| str: Path to the backup file for restoration | |
| Extra elements/keys are ignored. Raises AssertionError if validation fails. | |
| """ | |
| backup_file = f"{original_file}.backup" | |
| if isinstance(expected, dict): | |
| for key, expected_value in expected.items(): | |
| assert key in actual, f"Missing key in JSON: {key}" | |
| validate_json_partially(actual[key], expected_value) | |
| elif isinstance(expected, list): | |
| for schema_item in expected: | |
| matched = False | |
| for item in actual: | |
| try: | |
| validate_json_partially(item, schema_item) | |
| matched = True | |
| break | |
| except AssertionError: | |
| continue | |
| assert ( | |
| matched | |
| ), f"No matching element found in list for schema item {schema_item}" | |
| else: | |
| assert actual == expected, f"Value mismatch: expected {expected}, got {actual}" | |
| def validate_json_partially(actual: Any, expected: Any) -> None: | |
| """Recursively validate that `actual` JSON contains all keys and values specified in `expected`. | |
| Extra elements/keys are ignored. Raises AssertionError if validation fails. | |
| """ | |
| if isinstance(expected, dict): | |
| for key, expected_value in expected.items(): | |
| assert key in actual, f"Missing key in JSON: {key}" | |
| validate_json_partially(actual[key], expected_value) | |
| elif isinstance(expected, list): | |
| for schema_item in expected: | |
| matched = False | |
| for item in actual: | |
| try: | |
| validate_json_partially(item, schema_item) | |
| matched = True | |
| break | |
| except AssertionError: | |
| continue | |
| assert ( | |
| matched | |
| ), f"No matching element found in list for schema item {schema_item}" | |
| else: | |
| assert actual == expected, f"Value mismatch: expected {expected}, got {actual}" |
🤖 Prompt for AI Agents
In tests/e2e/utils/utils.py around lines 69 to 95, the function
validate_json_partially is missing an explicit return type annotation; update
the function signature to include an explicit return type of None (e.g., def
validate_json_partially(actual: Any, expected: Any) -> None:) so the file
complies with the project's typing guidelines and no other runtime changes are
needed.
LCORE-492: E2E tests for feedback endpoint
LCORE-492: E2E tests for feedback endpoint
2 participants
Description
Added e2e tests for feedback endpoint. Modified test environment for switching between multiple configs during runtime.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Tests
Refactor
Chores