LCORE-1329: Adding new MCP E2E Tests#1327
Open
jrobertboos wants to merge 1 commit intolightspeed-core:mainfrom
Open
LCORE-1329: Adding new MCP E2E Tests#1327jrobertboos wants to merge 1 commit intolightspeed-core:mainfrom
jrobertboos wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
Contributor
WalkthroughAdds comprehensive MCP e2e auth permutations (file, invalid-file, kubernetes, client, oauth), new test configs and secret, expands MCP feature tests, updates environment hooks to pick/restore MCP configs and perform mode-specific cleanup, and adds utilities to unregister MCP toolgroups and clear Llama Stack storage. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant EnvHook as Environment Hook
participant Docker as Docker/Compose
participant LCS as Lightspeed Stack
participant MCP as Mock MCP
TestRunner->>EnvHook: start scenario with MCP tag
EnvHook->>EnvHook: resolve config & selected cleanup
EnvHook->>EnvHook: if server -> unregister_mcp_toolgroups / if library -> clear storage
EnvHook->>Docker: apply config files & mount secrets
EnvHook->>LCS: restart service
TestRunner->>LCS: send API request (tools/query/stream)
LCS->>MCP: forward request with configured Authorization
MCP-->>LCS: respond (200 / 401)
LCS-->>TestRunner: return response
TestRunner->>EnvHook: teardown -> restore config & cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/steps/common_http.py`:
- Around line 181-197: There are two identical step definitions for the
`@then`("The body of the response does not contain {substring}") decorator
(function check_response_body_does_not_contain), causing AmbiguousStep and F811
errors; remove the duplicate definition so only the first
check_response_body_does_not_contain implementation remains, leaving its
assertion and docstring intact and deleting the second repeated block.
In `@tests/e2e/utils/llama_stack_utils.py`:
- Around line 40-53: The docstring for _unregister_toolgroup_async is
misleading—it says it returns (provider_id, provider_shield_id) but the function
signature and implementation return None; update the docstring in
_unregister_toolgroup_async to accurately state that it unregisters the
toolgroup and returns None (or remove the restoration tuple reference), making
sure to reference the function name _unregister_toolgroup_async and not change
the current return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c485079-4707-4284-b15d-82140aa7f41f
📒 Files selected for processing (22)
docker-compose.yamldocs/e2e_testing.mdtests/e2e/configuration/library-mode/lightspeed-stack-invalid-mcp-file-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-client-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-file-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-kubernetes-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-oauth-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp.yamltests/e2e/configuration/server-mode/lightspeed-stack-invalid-mcp-file-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-client-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-file-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-kubernetes-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-oauth-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp.yamltests/e2e/features/environment.pytests/e2e/features/mcp.featuretests/e2e/features/mcp_file_auth.featuretests/e2e/features/steps/common_http.pytests/e2e/secrets/invalid-mcp-tokentests/e2e/test_list.txttests/e2e/utils/llama_stack_utils.pytests/e2e/utils/utils.py
💤 Files with no reviewable changes (2)
- tests/e2e/test_list.txt
- tests/e2e/features/mcp_file_auth.feature
Comment on lines
+40
to
+53
| async def _unregister_toolgroup_async(identifier: str) -> None: | ||
| """Unregister a toolgroup by identifier; return (provider_id, provider_shield_id) for restore.""" | ||
| client = _get_llama_stack_client() | ||
| try: | ||
| await client.toolgroups.unregister(identifier) | ||
| except APIConnectionError: | ||
| raise | ||
| except APIStatusError as e: | ||
| # 400 "not found": toolgroup already absent, scenario can proceed | ||
| if e.status_code == 400 and "not found" in str(e).lower(): | ||
| return None | ||
| raise | ||
| finally: | ||
| await client.close() |
Contributor
There was a problem hiding this comment.
Misleading docstring: function returns None, not a tuple.
The docstring says "return (provider_id, provider_shield_id) for restore" but the function signature is -> None and the implementation only returns None. This appears to be copied from _unregister_shield_async which does return restoration data.
📝 Proposed fix for the docstring
async def _unregister_toolgroup_async(identifier: str) -> None:
- """Unregister a toolgroup by identifier; return (provider_id, provider_shield_id) for restore."""
+ """Unregister a toolgroup by identifier."""
client = _get_llama_stack_client()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/utils/llama_stack_utils.py` around lines 40 - 53, The docstring for
_unregister_toolgroup_async is misleading—it says it returns (provider_id,
provider_shield_id) but the function signature and implementation return None;
update the docstring in _unregister_toolgroup_async to accurately state that it
unregisters the toolgroup and returns None (or remove the restoration tuple
reference), making sure to reference the function name
_unregister_toolgroup_async and not change the current return behavior.
tisnik
requested changes
Mar 16, 2026
Contributor
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM in overall. Two or three nits. Also please rebase + squash commits before merge
tests/e2e/features/environment.py
Outdated
| switch_config(context.scenario_config) | ||
| restart_container("lightspeed-stack") | ||
|
|
||
| config_name: str | None = None |
Contributor
There was a problem hiding this comment.
later it will need to be refactored, not to use tags
| config_name: str | None = None | ||
| if "MCPFileAuthConfig" in scenario.effective_tags: | ||
| config_name = "mcp-file-auth" | ||
| elif "InvalidMCPFileAuthConfig" in scenario.effective_tags: |
Contributor
There was a problem hiding this comment.
and one reason why not to use tags is this (basically) mapping set->one item
| raise | ||
| except APIStatusError as e: | ||
| # 400 "not found": toolgroup already absent, scenario can proceed | ||
| if e.status_code == 400 and "not found" in str(e).lower(): |
Contributor
There was a problem hiding this comment.
prob. ok, but later 400 might be changed to names constants (from requests etc.)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/features/mcp.feature (1)
12-12: Trailing whitespace in scenario names.Several scenario names have trailing whitespace (e.g., lines 12, 51, 86, 106, 148, 185). This can cause unnecessary diff noise in future changes.
🧹 Remove trailing whitespace
- Scenario: Check if tools endpoint succeeds when MCP file-based auth token is passed + Scenario: Check if tools endpoint succeeds when MCP file-based auth token is passedApply similar fix to lines 51, 86, 106, 148, and 185.
Also applies to: 51-51, 86-86, 106-106, 148-148, 185-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/mcp.feature` at line 12, Remove the trailing spaces at the end of the Scenario titles so they don't introduce diff noise; specifically trim the trailing whitespace from "Scenario: Check if tools endpoint succeeds when MCP file-based auth token is passed " and the other Scenario lines with trailing spaces (the Scenario names at the same positions such as the ones currently ending with a space on lines with texts like the ones referenced in the review), ensuring each Scenario title string has no trailing whitespace.tests/e2e/utils/utils.py (1)
266-276: Consider adding debug logging for observability.The function silently ignores all non-timeout failures due to
check=Falsewithout any logging. While this is reasonable (e.g.,~/.llamamay not exist yet), the complete lack of feedback can make E2E test debugging harder.💡 Optional: Add minimal logging for debugging
try: - subprocess.run( + result = subprocess.run( ["docker", "exec", container_name, "sh", "-c", "rm -rf ~/.llama"], capture_output=True, text=True, timeout=10, check=False, ) + if result.returncode != 0: + print(f"Note: clear_llama_stack_storage returned {result.returncode} (may be expected if directory doesn't exist)") except subprocess.TimeoutExpired as e: print(f"Failed to clear Llama Stack storage: {e}") raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/utils.py` around lines 266 - 276, The subprocess.run call that removes ~/.llama (inside the try block using container_name) uses check=False so non-timeout failures are silent; capture the CompletedProcess result (assign subprocess.run(...) to a variable) and if result.returncode != 0 log a debug/info message including container_name, the command run, and result.stderr/result.stdout (or at least returncode) so failed removals are observable while still not raising; keep the existing TimeoutExpired except branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/features/mcp.feature`:
- Line 12: Remove the trailing spaces at the end of the Scenario titles so they
don't introduce diff noise; specifically trim the trailing whitespace from
"Scenario: Check if tools endpoint succeeds when MCP file-based auth token is
passed " and the other Scenario lines with trailing spaces (the Scenario names
at the same positions such as the ones currently ending with a space on lines
with texts like the ones referenced in the review), ensuring each Scenario title
string has no trailing whitespace.
In `@tests/e2e/utils/utils.py`:
- Around line 266-276: The subprocess.run call that removes ~/.llama (inside the
try block using container_name) uses check=False so non-timeout failures are
silent; capture the CompletedProcess result (assign subprocess.run(...) to a
variable) and if result.returncode != 0 log a debug/info message including
container_name, the command run, and result.stderr/result.stdout (or at least
returncode) so failed removals are observable while still not raising; keep the
existing TimeoutExpired except branch unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11b9840f-0d97-4f06-9634-2227f5fed4d1
📒 Files selected for processing (3)
tests/e2e/features/environment.pytests/e2e/features/mcp.featuretests/e2e/utils/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/features/environment.py
- Introduced new YAML configuration files for MCP authentication methods: file, Kubernetes, and OAuth. - Updated existing MCP configuration to include new authentication methods. - Enhanced end-to-end tests to cover scenarios for file-based, Kubernetes, and OAuth authentication. - Added utility functions for unregistering MCP toolgroups in the testing environment. - Implemented new step definitions for checking response body content in tests. Add invalid MCP token configuration and update tests - Added a new invalid MCP token file for testing purposes. - Updated the Docker Compose configuration to mount the invalid MCP token. - Introduced a new YAML configuration for testing invalid MCP file authentication. - Enhanced the test scenarios to include checks for invalid MCP file authentication. - Updated feature files to reflect the new authentication configurations. Add additional MCP authentication configurations and update scenario handling - Added new configurations for invalid, Kubernetes, client, and OAuth MCP authentication methods. - Updated scenario handling to reference the new configuration paths for Kubernetes, client, and OAuth authentication. - Enhanced the testing environment to support the new authentication configurations. fixed errored tests skipped failing scenarios and added library mode configs fixed library mode tests error fixed black skipped acidentally missed failing test Refactor Llama Stack utilities and update E2E tests - Renamed `llama_stack_shields.py` to `llama_stack_utils.py` and expanded its functionality to manage both toolgroups and shields. - Removed the deprecated `llama_stack_tools.py` file. - Updated E2E test scenarios to utilize the new utility functions for unregistering toolgroups and clearing Llama Stack storage. - Enhanced feature files to include comments indicating pending fixes for skipped scenarios. Enhance MCP feature tests and streamline Llama Stack storage clearing - Added checks to ensure the response body does not contain 'mcp-client' in MCP feature tests. - Simplified the `clear_llama_stack_storage` function to remove the entire `~/.llama` directory instead of specific files, improving clarity and efficiency. - Updated comments to reflect the changes in storage clearing logic. Remove deprecated MCP file-based authorization feature and update test list addressed code rabbit Add new MCP authentication configurations and update tests - Introduced new YAML configuration files for MCP authentication methods: file, Kubernetes, and OAuth. - Updated existing MCP configuration to include new authentication methods. - Enhanced end-to-end tests to cover scenarios for file-based, Kubernetes, and OAuth authentication. - Added utility functions for unregistering MCP toolgroups in the testing environment. - Implemented new step definitions for checking response body content in tests. fixed errored tests fixed library mode tests error fixed black Refactor Llama Stack utilities and update E2E tests - Renamed `llama_stack_shields.py` to `llama_stack_utils.py` and expanded its functionality to manage both toolgroups and shields. - Removed the deprecated `llama_stack_tools.py` file. - Updated E2E test scenarios to utilize the new utility functions for unregistering toolgroups and clearing Llama Stack storage. - Enhanced feature files to include comments indicating pending fixes for skipped scenarios. Enhance MCP feature tests and streamline Llama Stack storage clearing - Added checks to ensure the response body does not contain 'mcp-client' in MCP feature tests. - Simplified the `clear_llama_stack_storage` function to remove the entire `~/.llama` directory instead of specific files, improving clarity and efficiency. - Updated comments to reflect the changes in storage clearing logic. Remove deprecated MCP file-based authorization feature and update test list addressed code rabbit fixed ruff addressed comments
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/features/environment.py (1)
229-248: Consider simplifying tag-to-config mapping with a dictionary.The if/elif chain could be reduced to a dictionary lookup for better maintainability, though the current implementation is functionally correct.
♻️ Optional refactor using dictionary lookup
- config_name: Optional[str] = None - if "MCPFileAuthConfig" in scenario.effective_tags: - config_name = "mcp-file-auth" - elif "InvalidMCPFileAuthConfig" in scenario.effective_tags: - config_name = "invalid-mcp-file-auth" - elif "MCPKubernetesAuthConfig" in scenario.effective_tags: - config_name = "mcp-kubernetes-auth" - elif "MCPClientAuthConfig" in scenario.effective_tags: - config_name = "mcp-client-auth" - elif "MCPOAuthAuthConfig" in scenario.effective_tags: - config_name = "mcp-oauth-auth" + _MCP_TAG_TO_CONFIG = { + "MCPFileAuthConfig": "mcp-file-auth", + "InvalidMCPFileAuthConfig": "invalid-mcp-file-auth", + "MCPKubernetesAuthConfig": "mcp-kubernetes-auth", + "MCPClientAuthConfig": "mcp-client-auth", + "MCPOAuthAuthConfig": "mcp-oauth-auth", + } + config_name: Optional[str] = None + for tag, name in _MCP_TAG_TO_CONFIG.items(): + if tag in scenario.effective_tags: + config_name = name + break🤖 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 229 - 248, The if/elif chain mapping scenario.effective_tags to config_name can be simplified by replacing it with a dictionary lookup: build a tag->config map and set config_name = tag_map.get(next_matching_tag) (or iterate keys to find the first tag present) instead of the long if/elif; keep the surrounding logic that checks config_name, calls unregister_mcp_toolgroups()/clear_llama_stack_storage(), sets context.scenario_config via _get_config_path(config_name, mode_dir), then calls switch_config(context.scenario_config) and restart_container("lightspeed-stack"). Ensure you reference the existing symbols config_name, scenario.effective_tags, _get_config_path, switch_config, restart_container, unregister_mcp_toolgroups, clear_llama_stack_storage, and context.scenario_config when making the change.
🤖 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/steps/common_http.py`:
- Around line 181-187: The negated step check_response_body_does_not_contain
currently checks the raw substring against context.response.text without
performing placeholder replacement; update the function to run the incoming
substring through the same placeholder resolver used by the positive variant
(e.g., call context.replace_placeholders(substring) or the project’s placeholder
helper) before asserting it is not in context.response.text, and use the
resolved value in the assertion message as well.
---
Nitpick comments:
In `@tests/e2e/features/environment.py`:
- Around line 229-248: The if/elif chain mapping scenario.effective_tags to
config_name can be simplified by replacing it with a dictionary lookup: build a
tag->config map and set config_name = tag_map.get(next_matching_tag) (or iterate
keys to find the first tag present) instead of the long if/elif; keep the
surrounding logic that checks config_name, calls
unregister_mcp_toolgroups()/clear_llama_stack_storage(), sets
context.scenario_config via _get_config_path(config_name, mode_dir), then calls
switch_config(context.scenario_config) and
restart_container("lightspeed-stack"). Ensure you reference the existing symbols
config_name, scenario.effective_tags, _get_config_path, switch_config,
restart_container, unregister_mcp_toolgroups, clear_llama_stack_storage, and
context.scenario_config when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a0586f4-7f52-4779-a794-08ebdc383d7d
📒 Files selected for processing (22)
docker-compose.yamldocs/e2e_testing.mdtests/e2e/configuration/library-mode/lightspeed-stack-invalid-mcp-file-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-client-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-file-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-kubernetes-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-oauth-auth.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp.yamltests/e2e/configuration/server-mode/lightspeed-stack-invalid-mcp-file-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-client-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-file-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-kubernetes-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-oauth-auth.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp.yamltests/e2e/features/environment.pytests/e2e/features/mcp.featuretests/e2e/features/mcp_file_auth.featuretests/e2e/features/steps/common_http.pytests/e2e/secrets/invalid-mcp-tokentests/e2e/test_list.txttests/e2e/utils/llama_stack_utils.pytests/e2e/utils/utils.py
💤 Files with no reviewable changes (2)
- tests/e2e/features/mcp_file_auth.feature
- tests/e2e/test_list.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/e2e/configuration/library-mode/lightspeed-stack-invalid-mcp-file-auth.yaml
- tests/e2e/configuration/server-mode/lightspeed-stack-mcp-client-auth.yaml
- tests/e2e/configuration/server-mode/lightspeed-stack-mcp-kubernetes-auth.yaml
- tests/e2e/configuration/library-mode/lightspeed-stack-mcp-kubernetes-auth.yaml
- tests/e2e/secrets/invalid-mcp-token
- tests/e2e/utils/utils.py
- tests/e2e/configuration/server-mode/lightspeed-stack-mcp.yaml
Comment on lines
+181
to
+187
| @then("The body of the response does not contain {substring}") | ||
| def check_response_body_does_not_contain(context: Context, substring: str) -> None: | ||
| """Check that response body does not contain a substring.""" | ||
| assert context.response is not None, "Request needs to be performed first" | ||
| assert ( | ||
| substring not in context.response.text | ||
| ), f"The response text '{context.response.text}' contains '{substring}'" |
Contributor
There was a problem hiding this comment.
Apply placeholder replacement in the negated body assertion.
This step checks the raw substring while the positive variant resolves placeholders first. Placeholder-based scenarios can fail incorrectly.
🐛 Proposed fix
`@then`("The body of the response does not contain {substring}")
def check_response_body_does_not_contain(context: Context, substring: str) -> None:
"""Check that response body does not contain a substring."""
assert context.response is not None, "Request needs to be performed first"
+ expected = replace_placeholders(context, substring)
assert (
- substring not in context.response.text
- ), f"The response text '{context.response.text}' contains '{substring}'"
+ expected not in context.response.text
+ ), f"The response text '{context.response.text}' contains '{expected}'"🤖 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 181 - 187, The negated
step check_response_body_does_not_contain currently checks the raw substring
against context.response.text without performing placeholder replacement; update
the function to run the incoming substring through the same placeholder resolver
used by the positive variant (e.g., call context.replace_placeholders(substring)
or the project’s placeholder helper) before asserting it is not in
context.response.text, and use the resolved value in the assertion message as
well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
E2E tests were added to cover all of the types of MCP authorization (static token, kubernetes, client, oauth). For each type of MCP Auth all endpoints that could be affected by auth are tested on each path (valid, invalid, omitted).
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
Tests
Documentation