LCORE-2341-unified mode feature files#2020
Conversation
WalkthroughFive new Gherkin E2E feature files are added covering unified-mode configuration: boot, legacy two-file deprecation, legacy-to-unified migration, configuration synthesis, and validation. All five are registered in ChangesUnified mode E2E test suite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🤖 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/unified-mode-boot.feature`:
- Around line 47-60: The profile-path boot scenarios only verify readiness and
never exercise query serving, so the relative and absolute variants can still
break inference without failing the test. Update the unified-mode boot scenarios
in unified-mode-boot.feature to include an actual inference/query request after
the service restarts and readiness passes, using the existing scenario patterns
for the relative and absolute profile-path cases so both boot and serve behavior
are covered.
In `@tests/e2e/features/unified-mode-migration.feature`:
- Around line 25-36: The migrated unified-mode scenarios have the action step
out of standard Given-When-Then order; in the affected feature cases, move the
service configuration precondition into the Given phase before the migration
action, or rewrite the migration step as part of the When/And action phase.
Update the affected scenarios in unified-mode-migration.feature so the step
sequence around lightspeed-stack --migrate-config, The service uses the
lightspeed-stack-unified-migrated.yaml configuration, and The service is
restarted reads clearly and follows BDD structure.
🪄 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: 43a48d62-94ab-4079-97fc-0bd6d5288518
📒 Files selected for processing (6)
tests/e2e/features/unified-mode-boot.featuretests/e2e/features/unified-mode-legacy.featuretests/e2e/features/unified-mode-migration.featuretests/e2e/features/unified-mode-synthesis.featuretests/e2e/features/unified-mode-validation.featuretests/e2e/test_list.txt
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: spectral
- GitHub Check: unit_tests (3.12)
- GitHub Check: Pyright
- GitHub Check: unit_tests (3.13)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- 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
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 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/unified-mode-legacy.featuretests/e2e/features/unified-mode-validation.featuretests/e2e/features/unified-mode-migration.featuretests/e2e/features/unified-mode-synthesis.featuretests/e2e/features/unified-mode-boot.feature
🔇 Additional comments (6)
tests/e2e/features/unified-mode-synthesis.feature (1)
1-59: LGTM!tests/e2e/features/unified-mode-legacy.feature (1)
1-40: LGTM!tests/e2e/features/unified-mode-validation.feature (2)
16-19: LGTM!
10-13: 🎯 Functional CorrectnessNo change needed. The validator message for
inference.providers+library_client_config_pathalready includes bothmutually exclusiveand--migrate-config, so the E2E assertion matches the implementation.> Likely an incorrect or invalid review comment.tests/e2e/test_list.txt (2)
35-39: LGTM!
35-39: All referenced feature files exist. The fivetests/e2e/features/unified-mode-*.featurepaths are present at the expected locations.
| @skip-in-server-mode | ||
| Scenario: Unified config with relative profile path boots in library mode | ||
| Given The service uses the lightspeed-stack-unified-relative-profile.yaml configuration | ||
| And The service is restarted | ||
| When I access endpoint "readiness" using HTTP GET method | ||
| Then The status code of the response is 200 | ||
|
|
||
|
|
||
| @skip-in-server-mode | ||
| Scenario: Unified config with absolute profile path boots in library mode | ||
| Given The service uses the lightspeed-stack-unified-absolute-profile.yaml configuration | ||
| And The service is restarted | ||
| When I access endpoint "readiness" using HTTP GET method | ||
| Then The status code of the response is 200 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exercise query serving for the profile-path variants.
These four scenarios stop at /readiness, so they never verify that the relative/absolute profile configs can actually serve inference requests. That leaves the boot contract untested for those variants and would miss regressions where startup succeeds but profile resolution breaks query handling.
Also applies to: 101-116
🤖 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/unified-mode-boot.feature` around lines 47 - 60, The
profile-path boot scenarios only verify readiness and never exercise query
serving, so the relative and absolute variants can still break inference without
failing the test. Update the unified-mode boot scenarios in
unified-mode-boot.feature to include an actual inference/query request after the
service restarts and readiness passes, using the existing scenario patterns for
the relative and absolute profile-path cases so both boot and serve behavior are
covered.
| @skip-in-server-mode | ||
| Scenario: Migrated unified configuration drives byte-identical Llama Stack behavior in library mode | ||
| When lightspeed-stack --migrate-config is run for the legacy migration fixture pair | ||
| Given The service uses the lightspeed-stack-unified-migrated.yaml configuration | ||
| And The service is restarted | ||
| When I access endpoint "readiness" using HTTP GET method | ||
| Then The status code of the response is 200 | ||
| When I use "query" to ask question | ||
| """ | ||
| {"query": "Say hello", "model": "{MODEL}", "provider": "{PROVIDER}"} | ||
| """ | ||
| Then The status code of the response is 200 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
Reorder steps to follow standard Given-When-Then structure.
In both library and server mode scenarios, the When step running --migrate-config precedes the Given step that configures the service to use the migrated file. While behave executes steps sequentially regardless of keyword, standard BDD style places all Given preconditions before When actions. Consider restructuring so that Given The service uses... appears before When lightspeed-stack --migrate-config is run..., or use And to clarify that the migration is part of the action phase.
📝 Suggested reordering for library mode scenario
`@skip-in-server-mode`
Scenario: Migrated unified configuration drives byte-identical Llama Stack behavior in library mode
- When lightspeed-stack --migrate-config is run for the legacy migration fixture pair
Given The service uses the lightspeed-stack-unified-migrated.yaml configuration
+ When lightspeed-stack --migrate-config is run for the legacy migration fixture pair
And The service is restartedOr if the migrate-config must happen first:
`@skip-in-server-mode`
Scenario: Migrated unified configuration drives byte-identical Llama Stack behavior in library mode
- When lightspeed-stack --migrate-config is run for the legacy migration fixture pair
- Given The service uses the lightspeed-stack-unified-migrated.yaml configuration
+ Given The legacy migration fixture pair has been migrated to lightspeed-stack-unified-migrated.yaml
+ And The service uses the lightspeed-stack-unified-migrated.yaml configuration
And The service is restartedAlso applies to: 41-53
🤖 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/unified-mode-migration.feature` around lines 25 - 36, The
migrated unified-mode scenarios have the action step out of standard
Given-When-Then order; in the affected feature cases, move the service
configuration precondition into the Given phase before the migration action, or
rewrite the migration step as part of the When/And action phase. Update the
affected scenarios in unified-mode-migration.feature so the step sequence around
lightspeed-stack --migrate-config, The service uses the
lightspeed-stack-unified-migrated.yaml configuration, and The service is
restarted reads clearly and follows BDD structure.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/unified-mode-boot.feature`:
- Line 1: The feature is currently tagged with `@skip`, which prevents every
scenario in unified-mode-boot from running because environment.py skips any
scenario carrying that tag. Remove the feature-level `@skip` from
unified-mode-boot.feature unless this suite is intentionally disabled, and keep
the existing e2e_group_2 tag so the scenarios can execute normally.
In `@tests/e2e/features/unified-mode-legacy.feature`:
- Line 1: Remove the feature-level `@skip` tag from the unified-mode-legacy
feature so the scenarios are no longer skipped entirely; keep the existing
scenario-level mode tags to control which legacy cases run. Update the feature
header in tests/e2e/features/unified-mode-legacy.feature so only the intended
scenario gating remains.
In `@tests/e2e/features/unified-mode-migration.feature`:
- Line 1: The feature is currently marked with a file-level `@skip`, which
prevents all unified mode migration scenarios from running in E2E. Remove the
`@skip` from the feature header in unified-mode-migration.feature so behave
executes the registered migration coverage, while keeping any narrower
scenario-level tags only if needed.
In `@tests/e2e/features/unified-mode-synthesis.feature`:
- Line 1: Remove the feature-level `@skip` tag from unified-mode-synthesis.feature
so the six synthesis scenarios are no longer disabled. Update the feature header
in tests/e2e/features/unified-mode-synthesis.feature to keep the existing
`@e2e_group_2` tag but drop `@skip`, ensuring the scenarios run normally.
In `@tests/e2e/features/unified-mode-validation.feature`:
- Line 1: The feature-level `@skip` tag on unified-mode-validation.feature is
preventing both scenarios from running, including the new --migrate-config error
path. Remove the top-level `@skip` from the feature declaration so the validation
scenarios execute normally, while keeping the existing scenario-level tags or
any selective skips only where explicitly needed.
🪄 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: 3b3a2ebc-72c5-48aa-8f43-3ff13b1d42ec
📒 Files selected for processing (5)
tests/e2e/features/unified-mode-boot.featuretests/e2e/features/unified-mode-legacy.featuretests/e2e/features/unified-mode-migration.featuretests/e2e/features/unified-mode-synthesis.featuretests/e2e/features/unified-mode-validation.feature
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
🧰 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/unified-mode-validation.featuretests/e2e/features/unified-mode-legacy.featuretests/e2e/features/unified-mode-migration.featuretests/e2e/features/unified-mode-boot.featuretests/e2e/features/unified-mode-synthesis.feature
🔇 Additional comments (3)
tests/e2e/features/unified-mode-boot.feature (2)
47-60: Still missing query coverage for the profile-path variants.This is the same gap already flagged earlier: both relative and absolute profile-path scenarios stop at
readiness, so they still won't catch regressions where startup succeeds but request serving fails.Also applies to: 101-116
13-44: LGTM!Also applies to: 65-98
tests/e2e/features/unified-mode-migration.feature (1)
27-29: The Given/When ordering is still inverted in these scenarios.This is the same BDD step-ordering issue already called out in the earlier review and it still applies here.
Also applies to: 43-45
| @@ -0,0 +1,116 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Feature files carrying a top-level skip tag:"
rg -n '^@.*\bskip\b' tests/e2e/features
echo
echo "Behave hook handling for skip-related tags:"
fd -HI 'environment.py' tests/e2e . -x sh -c '
echo "--- $1 ---"
rg -n -C3 "skip|skip-in-server-mode|skip-in-library-mode" "$1" || true
' sh {}Repository: lightspeed-core/lightspeed-stack
Length of output: 8141
Remove the feature-level @skip unless this suite is meant to stay disabled. tests/e2e/features/environment.py skips any scenario tagged @skip, so this feature will never execute as written.
🤖 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/unified-mode-boot.feature` at line 1, The feature is
currently tagged with `@skip`, which prevents every scenario in unified-mode-boot
from running because environment.py skips any scenario carrying that tag. Remove
the feature-level `@skip` from unified-mode-boot.feature unless this suite is
intentionally disabled, and keep the existing e2e_group_2 tag so the scenarios
can execute normally.
| @@ -0,0 +1,39 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how `@skip` is handled in the E2E harness and where it is used.
fd -i 'environment.py' tests/e2e .
rg -n -C2 --glob 'tests/e2e/**' '`@skip`\b|`@skip-in-server-mode`|`@skip-in-library-mode`'
rg -n -C2 --glob '*' 'behave|--tags|`@skip`|skip-in-server-mode|skip-in-library-mode'Repository: lightspeed-core/lightspeed-stack
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== feature file ==\n'
cat -n tests/e2e/features/unified-mode-legacy.feature
printf '\n== environment.py ==\n'
cat -n tests/e2e/features/environment.py
printf '\n== tag references in e2e ==\n'
rg -n -C2 '`@skip`\b|`@skip-in-server-mode`|`@skip-in-library-mode`|before_tag|after_tag|active_tags|tag_expression|behave' tests/e2eRepository: lightspeed-core/lightspeed-stack
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('tests/e2e/features/unified-mode-legacy.feature')
text = p.read_text()
print('top tags:', text.splitlines()[:5])
print('has scenario tags:', '`@skip-in-server-mode`' in text, '`@skip-in-library-mode`' in text)
PYRepository: lightspeed-core/lightspeed-stack
Length of output: 365
Remove the feature-level @skip tag. tests/e2e/features/unified-mode-legacy.feature:1 skips both scenarios, so this legacy coverage never runs; the scenario-level mode tags already gate the two cases.
🤖 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/unified-mode-legacy.feature` at line 1, Remove the
feature-level `@skip` tag from the unified-mode-legacy feature so the scenarios
are no longer skipped entirely; keep the existing scenario-level mode tags to
control which legacy cases run. Update the feature header in
tests/e2e/features/unified-mode-legacy.feature so only the intended scenario
gating remains.
| @@ -0,0 +1,53 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the file-level @skip.
Line 1 skips the entire feature, so none of these newly added migration scenarios will execute even though this PR registers them as E2E coverage. That leaves the migration path untested in CI. As per coding guidelines, tests/e2e/**/*.{py,feature} should be used for behave-based end-to-end testing, and a file-level skip prevents that coverage from running.
🤖 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/unified-mode-migration.feature` at line 1, The feature is
currently marked with a file-level `@skip`, which prevents all unified mode
migration scenarios from running in E2E. Remove the `@skip` from the feature
header in unified-mode-migration.feature so behave executes the registered
migration coverage, while keeping any narrower scenario-level tags only if
needed.
Source: Coding guidelines
| @@ -0,0 +1,59 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how `@skip` is handled in this repo's behave setup and where else it is used.
fd -i 'environment.py' tests . -x sh -c 'echo "=== $1 ==="; sed -n "1,220p" "$1"' sh {}
echo
rg -n -C2 '`@skip`\b|skip-in-server-mode|skip-in-library-mode' tests/e2eRepository: lightspeed-core/lightspeed-stack
Length of output: 32630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tests/e2e/features/unified-mode-synthesis.feature ==="
sed -n '1,120p' tests/e2e/features/unified-mode-synthesis.feature
echo
echo "=== Behave availability ==="
python3 - <<'PY'
try:
import behave
print("behave:", behave.__version__)
except Exception as e:
print("behave_import_error:", type(e).__name__, e)
PYRepository: lightspeed-core/lightspeed-stack
Length of output: 3113
Remove the feature-level @skip tag. This disables all six scenarios in tests/e2e/features/unified-mode-synthesis.feature, so the new synthesis coverage never runs.
🤖 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/unified-mode-synthesis.feature` at line 1, Remove the
feature-level `@skip` tag from unified-mode-synthesis.feature so the six synthesis
scenarios are no longer disabled. Update the feature header in
tests/e2e/features/unified-mode-synthesis.feature to keep the existing
`@e2e_group_2` tag but drop `@skip`, ensuring the scenarios run normally.
| @@ -0,0 +1,19 @@ | |||
| @e2e_group_2 @skip | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how `@skip` is handled in the E2E harness and where it is used.
fd -i 'environment.py' tests/e2e .
rg -n -C2 --glob 'tests/e2e/**' '`@skip`\b|`@skip-in-server-mode`|`@skip-in-library-mode`'
rg -n -C2 --glob '*' 'behave|--tags|`@skip`|skip-in-server-mode|skip-in-library-mode'Repository: lightspeed-core/lightspeed-stack
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== feature file ==\n'
cat -n tests/e2e/features/unified-mode-validation.feature
printf '\n== environment.py outline ==\n'
ast-grep outline tests/e2e/features/environment.py --view expanded || true
printf '\n== environment.py ==\n'
cat -n tests/e2e/features/environment.py
printf '\n== skip tag usage ==\n'
rg -n -C2 '`@skip`\b|skip-in-server-mode|skip-in-library-mode|--tags|behave' tests/e2e . --glob '!**/.git/**'Repository: lightspeed-core/lightspeed-stack
Length of output: 50390
Remove the feature-level @skip tag.
That tag skips both validation scenarios, so the new --migrate-config error path never runs.
🤖 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/unified-mode-validation.feature` at line 1, The
feature-level `@skip` tag on unified-mode-validation.feature is preventing both
scenarios from running, including the new --migrate-config error path. Remove
the top-level `@skip` from the feature declaration so the validation scenarios
execute normally, while keeping the existing scenario-level tags or any
selective skips only where explicitly needed.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
LGTM. The When/Given steps switch looked weird at 1st glance, but seems to be ok.
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