LCORE-2079: added agent skills e2e feature file#1742
Open
jrobertboos wants to merge 3 commits into
Open
Conversation
Contributor
WalkthroughAdds a Gherkin end-to-end feature file defining agent skills tests: setup/background, REST /tools checks, list/activate/load tool flows (query and streaming), error cases, deduplication, and multi-skill discovery. ChangesAgent Skills E2E Test Suite
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/features/skills.feature`:
- Line 7: The feature file contains a hardcoded Authorization header in the step
"And I set the Authorization header to Bearer …"; replace that literal token
with a placeholder backed by an env/fixture (e.g., use a Cucumber parameter or
placeholder like "<AUTH_TOKEN>" or a step that reads process.env.TEST_TOKEN) and
update the test bootstrap/fixture loader to inject a real token at runtime (or
add a dedicated step that loads/sets a token from a fixture or env). Ensure the
step implementation that handles "I set the Authorization header" reads the
placeholder and substitutes the runtime token so no static secrets remain in the
feature text.
🪄 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: 987e5347-9fc7-41ee-afd7-775a3c1cba2d
📒 Files selected for processing (1)
tests/e2e/features/skills.feature
📜 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). (7)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server 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
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{py,feature}
📄 CodeRabbit inference engine (AGENTS.md)
Use behave (BDD) framework for end-to-end testing with Gherkin feature files
Files:
tests/e2e/features/skills.feature
🔇 Additional comments (1)
tests/e2e/features/skills.feature (1)
1-6: LGTM!Also applies to: 8-377
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/features/skills.feature`:
- Around line 19-42: The scenario step "And The body of the response is the
following" in features/skills.feature currently contains placeholder, non-JSON
payloads (Python-like True/None and trailing commas); replace each placeholder
block with real, parseable contract assertions: valid JSON (true/null, no
trailing commas) or explicit Gherkin-friendly key checks (e.g., tools array
length, expected tool.identifier/tool.description/tool.parameters entries), and
ensure assertions cover all actual tool entries (not only skill tools); update
all similar blocks referenced in the comment so the test validates the real
response shape rather than parsing invalid placeholders.
- Around line 13-42: The feature tests for the /tools endpoint use the same
expected response body for both the "Skill tools are registered when skills are
configured" and the complementary scenario, so they don't assert opposite
conditions; update the Scenario "Skill tools are registered when skills are
configured" and the complementary scenario (lines referenced around the scenario
titles and the step 'When I access REST API endpoint "tools" using HTTP GET
method') to assert opposite expectations: one should assert that the response
body includes the specific skill-tool identifier(s) like "filesystem_read" and
their provider_id/toolgroup_id entries, and the other should assert that those
skill-tool identifier(s) are absent (negated assertions) while still validating
the overall response shape and status 200; locate and change the step
implementations that compare the response body so they perform inclusion vs
exclusion checks for the "filesystem_read" identifier (and analogous identifiers
in the other scenario) rather than using the identical placeholder body for both
tests.
- Line 1: Remove the file-level `@skip` annotation at the top of the feature file
(the line containing "`@e2e_group_2` `@skip`") so the entire E2E suite in
skills.feature runs; if certain scenarios still need to be skipped, apply `@skip`
only to those individual Scenario/Scenario Outline blocks instead of the file
header to avoid disabling all tests.
🪄 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: 5f23fca9-e75c-4ea5-ac50-8da87ff190b5
📒 Files selected for processing (1)
tests/e2e/features/skills.feature
📜 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). (9)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{py,feature}
📄 CodeRabbit inference engine (AGENTS.md)
Use behave (BDD) framework for end-to-end testing with Gherkin feature files
Files:
tests/e2e/features/skills.feature
| @@ -0,0 +1,432 @@ | |||
| @e2e_group_2 @skip | |||
Contributor
There was a problem hiding this comment.
Remove file-level @skip; it disables the entire new E2E suite.
With @skip on Line 1, none of these scenarios execute, so this PR adds no effective coverage.
Suggested minimal fix
-@e2e_group_2 `@skip`
+@e2e_group_2📝 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
| @e2e_group_2 @skip | |
| `@e2e_group_2` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/features/skills.feature` at line 1, Remove the file-level `@skip`
annotation at the top of the feature file (the line containing "`@e2e_group_2`
`@skip`") so the entire E2E suite in skills.feature runs; if certain scenarios
still need to be skipped, apply `@skip` only to those individual Scenario/Scenario
Outline blocks instead of the file header to avoid disabling all tests.
Comment on lines
+13
to
+42
| Scenario: Skill tools are registered when skills are configured | ||
| Given The e2e-test-skill skill directory path is "e2e-test-skill" | ||
| And The service uses the lightspeed-stack-skills.yaml configuration | ||
| And The service is restarted | ||
| When I access REST API endpoint "tools" using HTTP GET method | ||
| Then The status code of the response is 200 | ||
| And The body of the response is the following #TODO: Currently placeholder, should reflect actual tools (all tools not just skill tools) | ||
| """ | ||
| { | ||
| "tools": [ | ||
| { | ||
| "identifier": "filesystem_read", | ||
| "description": "Read contents of a file from the filesystem", | ||
| "parameters": [ | ||
| { | ||
| "name": "path", | ||
| "description": "Path to the file to read", | ||
| "parameter_type": "string", | ||
| "required": True, | ||
| "default": None, | ||
| } | ||
| ], | ||
| "provider_id": "model-context-protocol", | ||
| "toolgroup_id": "filesystem-tools", | ||
| "server_source": "http://localhost:3000", | ||
| "type": "tool", | ||
| } | ||
| ], | ||
| } | ||
| """ |
Contributor
There was a problem hiding this comment.
/tools positive/negative scenarios currently assert the same body shape.
“Skill tools are registered” and “not registered” should assert opposite conditions for skill-tool identifiers. Right now they do not distinguish the contract and can both pass/fail for the wrong reason.
Also applies to: 44-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/features/skills.feature` around lines 13 - 42, The feature tests
for the /tools endpoint use the same expected response body for both the "Skill
tools are registered when skills are configured" and the complementary scenario,
so they don't assert opposite conditions; update the Scenario "Skill tools are
registered when skills are configured" and the complementary scenario (lines
referenced around the scenario titles and the step 'When I access REST API
endpoint "tools" using HTTP GET method') to assert opposite expectations: one
should assert that the response body includes the specific skill-tool
identifier(s) like "filesystem_read" and their provider_id/toolgroup_id entries,
and the other should assert that those skill-tool identifier(s) are absent
(negated assertions) while still validating the overall response shape and
status 200; locate and change the step implementations that compare the response
body so they perform inclusion vs exclusion checks for the "filesystem_read"
identifier (and analogous identifiers in the other scenario) rather than using
the identical placeholder body for both tests.
Comment on lines
+19
to
+42
| And The body of the response is the following #TODO: Currently placeholder, should reflect actual tools (all tools not just skill tools) | ||
| """ | ||
| { | ||
| "tools": [ | ||
| { | ||
| "identifier": "filesystem_read", | ||
| "description": "Read contents of a file from the filesystem", | ||
| "parameters": [ | ||
| { | ||
| "name": "path", | ||
| "description": "Path to the file to read", | ||
| "parameter_type": "string", | ||
| "required": True, | ||
| "default": None, | ||
| } | ||
| ], | ||
| "provider_id": "model-context-protocol", | ||
| "toolgroup_id": "filesystem-tools", | ||
| "server_source": "http://localhost:3000", | ||
| "type": "tool", | ||
| } | ||
| ], | ||
| } | ||
| """ |
Contributor
There was a problem hiding this comment.
Replace placeholder expected payloads with real, parseable contract assertions.
Most scenarios still assert TODO placeholders ("content": "bla"), and the blocks are not valid JSON (True/None/trailing commas). Once unskipped, this will either fail parsing or validate the wrong behavior.
Also applies to: 49-72, 87-98, 114-125, 141-152, 168-179, 196-207, 223-234, 249-260, 274-285, 298-309, 323-334, 350-361, 368-379, 395-406, 421-432
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/features/skills.feature` around lines 19 - 42, The scenario step
"And The body of the response is the following" in features/skills.feature
currently contains placeholder, non-JSON payloads (Python-like True/None and
trailing commas); replace each placeholder block with real, parseable contract
assertions: valid JSON (true/null, no trailing commas) or explicit
Gherkin-friendly key checks (e.g., tools array length, expected
tool.identifier/tool.description/tool.parameters entries), and ensure assertions
cover all actual tool entries (not only skill tools); update all similar blocks
referenced in the comment so the test validates the real response shape rather
than parsing invalid placeholders.
Contributor
Author
|
Ready For Review: |
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
1 participant
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
Added E2E feature file for agent skills.
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