LCORE-1016: add files required for library mode tests run #864
Conversation
WalkthroughAdds library-mode support to E2E CI and tests: introduces a mode×environment matrix, mode-aware config selection and startup paths (server vs library), new library-mode configs, mode-aware test hooks and tags, and related container/compose and run config updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Workflow (.github)
participant FS as Repo (configs)
participant Runner as Job Runner
participant Orchestrator as startup script
participant Compose as docker-compose / docker-compose-library
participant Service as lightspeed-stack container(s)
participant Tests as E2E test runner
CI->>Runner: start matrix job (mode, environment)
Runner->>Orchestrator: set E2E_DEPLOYMENT_MODE (mode) and ENV
Orchestrator->>FS: load lightspeed-stack-<mode>-mode.yaml and run-<env>.yaml
Orchestrator->>Compose: choose compose file based on mode
Compose->>Service: start containers (server: multiple, library: single)
Service-->>Orchestrator: healthcheck / logs
Orchestrator->>Tests: run e2e suite with mode-aware hooks (skip tags, configs)
Tests-->>Service: interact (HTTP, library client)
alt failure
Orchestrator->>Runner: collect mode-specific logs
end
Tests->>Runner: report results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (10)
tests/e2e/configs/run-rhelai.yaml (1)
19-19: external_providers_dir now hard‑codes container pathSetting
external_providers_dirto/opt/app-root/src/.llama/providers.dmatches the new container layout and should unblock RHEL AI runs. Please confirm this config is only used in that containerized context; if anyone reusesrun-rhelai.yamloutside the image, this absolute path may not exist and could cause provider loading issues.run.yaml (1)
19-19: Double‑check impact of absolute providers path on local runsFor containerized runs (including library‑mode in the new test container),
/opt/app-root/src/.llama/providers.dis correct and consistent. Ifrun.yamlis ever used directly on a developer machine (e.g., local llama‑stack runs outside Docker), that absolute path likely won’t exist; depending on llama‑stack behavior this could be a no‑op or an error. Worth confirming local usage expectations and, if needed, documenting thatrun.yamlis container‑only or making the path configurable.Makefile (1)
27-30: Good local e2e shortcut; clarify usage and keep flags in sync
test-e2e-localmirrorstest-e2ewithout thescriptwrapper, which is nice for local debugging. Two small suggestions:
- Consider clarifying in the description that
test-e2eis the CI/recorded variant whiletest-e2e-localis for local interactive runs.- Whenever you change behave flags or
tests/e2e/test_list.txt, keep both targets in sync so local and CI behave runs exercise the same scenarios. Based on learnings,tests/e2e/test_list.txtis the canonical e2e test manifest.test.containerfile (1)
4-11: Container layout matches configs; consider minor hardeningThe added
mkdir/chownsteps correctly provision:
/opt/app-root/src/.llama/distributions/ollama/opt/app-root/src/.llama/providers.dwhich matches the updated
run*.yamlconfigurations. A couple of optional tweaks:
- Use
python -m pip install faiss-cpu==1.11.0instead of barepipto avoid surprises ifpippoints at a different interpreter.- If you expect group‑writable access under
.llama, you might mirror/app-rootand add an explicitchmod -R 775 /opt/app-root/src/.llama; currently 755 + correct ownership is probably fine for UID 1001, though.Nothing blocking here.
tests/e2e/configuration/lightspeed-stack-server-mode.yaml (1)
1-21: Server‑mode config looks coherent; verify data paths and wiringThis server‑mode LCS config is internally consistent:
use_as_library_client: falseandurl: http://llama-stack:8321match the “external llama‑stack service” comment.noopauth and test‑onlyapi_keyare appropriate for e2e.Two things to verify:
- That the docker‑compose/test harness for server‑mode mounts or creates
/tmp/data/feedbackand/tmp/data/transcriptswith suitable permissions, so tests don’t fail on I/O.- That environment/tag wiring (e.g., in
tests/e2e/features/environment.py) actually routes the relevant server‑mode scenarios to this config, similar to how noop vs noop‑with‑token configs are swapped. Based on learnings, config selection is tag‑driven.tests/e2e/configuration/library-mode/lightspeed-stack-invalid-feedback-storage.yaml (1)
1-20: Library‑mode invalid feedback storage config matches intentThis config correctly sets up a library‑mode LCS using
run.yamlwhile deliberately pointingfeedback_storageat an invalid path, which should drive negative test coverage for feedback persistence. A couple of follow‑ups:
- Confirm that the new library‑mode tests (or tags) actually select this config via the existing environment/tag‑driven mechanism (similar to the
noop-with-tokenflow mentioned in prior learnings).- Ensure the application surfaces a clear, testable failure (HTTP status / error message) when
/invalidis used, so the e2e scenario can assert on behavior rather than just a generic crash.Based on learnings, config swapping is tag‑based, so making sure this file is part of that mapping is key.
tests/e2e/configuration/lightspeed-stack-library-mode.yaml (1)
1-20: Base library-mode configuration looks consistent with intended usageUsing
use_as_library_client: truewithlibrary_client_config_path: run.yamland a noop auth module (withauth_enabled: false) is appropriate for the default, non-Authorized library-mode runs. The structure also mirrors the auth-noop-token variant, which keeps configs predictable.tests/e2e/features/environment.py (1)
54-59: Deployment mode detection is straightforward and robust enoughUsing
E2E_DEPLOYMENT_MODEwith a lower-cased value and defaulting to"server"cleanly exposescontext.is_library_modefor downstream hooks and logging, without impacting existing behavior when the env var is unset..github/workflows/e2e_tests.yaml (2)
128-129: Path transformation viasedis fragile; recommend using a dedicated config update function or YAML parser.The
sedcommands on lines 128-129 assume exact string matches and may fail silently if the run.yaml structure differs (e.g., different indentation, comments, or key ordering):sed -i 's|db_path: \.llama/distributions|db_path: /app-root/.llama/distributions|g' run.yaml sed -i 's|db_path: tmp/|db_path: /app-root/.llama/distributions/|g' run.yamlIf these patterns don't match the actual content, the paths remain unchanged and tests may fail with cryptic container path errors.
Consider one of the following alternatives:
- Pre-generate config files with correct paths for container environments instead of transforming them at runtime.
- Use a YAML-aware tool (e.g.,
yq) to update paths safely:yq eval '.db_path = "/app-root/.llama/distributions"' -i run.yaml- Add validation to confirm the sed replacements succeeded:
if grep -q 'db_path: \./\|db_path: tmp/' run.yaml; then echo "Error: Path transformation failed" exit 1 fi
191-204: Hardcoded 20-second wait may be insufficient or excessive; consider using polling with a timeout.Line 191 uses a fixed
sleep 20sbefore connectivity tests. This is:
- Too short if library initialization or external API calls are slow (especially on CI infrastructure).
- Too long if services start quickly, delaying feedback on failures.
Replace the fixed sleep with a polling loop that checks service health and fails fast on errors:
- name: Wait for services run: | echo "Waiting for services to be ready..." max_attempts=30 attempt=0 while [ $attempt -lt $max_attempts ]; do if curl -sf http://localhost:8080/liveness > /dev/null 2>&1; then echo "✓ Service is ready" exit 0 fi attempt=$((attempt + 1)) echo "Attempt $attempt/$max_attempts - service not ready yet, retrying in 2s..." sleep 2 done echo "✗ Service failed to become ready after ${max_attempts} attempts ($(($max_attempts * 2))s)" exit 1This provides faster feedback on success and clearer diagnostics on timeout.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/e2e_tests.yaml(7 hunks)Containerfile(1 hunks)Makefile(1 hunks)docker-compose-library.yaml(1 hunks)run.yaml(1 hunks)test.containerfile(1 hunks)tests/e2e/configs/run-azure.yaml(1 hunks)tests/e2e/configs/run-ci.yaml(1 hunks)tests/e2e/configs/run-rhelai.yaml(1 hunks)tests/e2e/configuration/library-mode/lightspeed-stack-auth-noop-token.yaml(1 hunks)tests/e2e/configuration/library-mode/lightspeed-stack-invalid-feedback-storage.yaml(1 hunks)tests/e2e/configuration/lightspeed-stack-library-mode.yaml(1 hunks)tests/e2e/configuration/lightspeed-stack-server-mode.yaml(1 hunks)tests/e2e/features/conversations.feature(2 hunks)tests/e2e/features/environment.py(4 hunks)tests/e2e/features/health.feature(2 hunks)tests/e2e/features/info.feature(4 hunks)tests/e2e/features/query.feature(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Files:
tests/e2e/features/query.featuretests/e2e/features/health.featuretests/e2e/features/info.featuretests/e2e/features/conversations.feature
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/e2e/features/environment.py
🧠 Learnings (7)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/e2e/test_list.txt : Maintain test list in `tests/e2e/test_list.txt` for end-to-end tests
Applied to files:
tests/e2e/features/health.featureMakefile.github/workflows/e2e_tests.yaml
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Run `uv run make verify` to run all linters (black, pylint, pyright, ruff, docstyle, check-types) before completion
Applied to files:
Makefile
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/e2e/features/**/*.feature : Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Applied to files:
Makefiletests/e2e/features/environment.py.github/workflows/e2e_tests.yaml
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/configuration/library-mode/lightspeed-stack-auth-noop-token.yamltests/e2e/configuration/lightspeed-stack-library-mode.yamltests/e2e/configuration/lightspeed-stack-server-mode.yamltests/e2e/configuration/library-mode/lightspeed-stack-invalid-feedback-storage.yaml
📚 Learning: 2025-09-02T11:15:02.411Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Applied to files:
tests/e2e/configuration/library-mode/lightspeed-stack-auth-noop-token.yaml
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/{client,app/endpoints/**}.py : Handle `APIConnectionError` from Llama Stack in integration code
Applied to files:
tests/e2e/features/info.featuretests/e2e/features/conversations.feature
📚 Learning: 2025-08-18T10:56:55.349Z
Learnt from: matysek
Repo: lightspeed-core/lightspeed-stack PR: 292
File: pyproject.toml:0-0
Timestamp: 2025-08-18T10:56:55.349Z
Learning: The lightspeed-stack project intentionally uses a "generic image" approach, bundling many dependencies directly in the base runtime image to work for everyone, rather than using lean base images with optional dependency groups.
Applied to files:
docker-compose-library.yaml
⏰ 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). (3)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (17)
tests/e2e/configs/run-azure.yaml (1)
19-19: Consistent provider directory for Azure configUsing
/opt/app-root/src/.llama/providers.dkeeps this Azure run config aligned with the container filesystem. Just make sure this file is not used for any non‑container Azure/dev flow where that path wouldn’t exist, otherwise provider discovery could fail silently.tests/e2e/configs/run-ci.yaml (1)
19-19: CI config correctly aligned with container layoutPointing
external_providers_dirat/opt/app-root/src/.llama/providers.dmatches the directories created in the test container and keeps CI config in sync with Azure/RHEL AI configs. Just ensure any future container layout changes are reflected in this path as well.tests/e2e/features/conversations.feature (2)
132-146: Library-mode skip tag for llama-stack GET error scenario looks correctTagging this llama-stack-unavailable GET scenario with
@skip-in-library-modealigns with the newbefore_scenariogating and avoids invalid behavior when no separate llama-stack container exists.
181-194: Consistent skip behavior for DELETE + llama-stack-unavailable scenarioSame here: marking the DELETE-on-unavailable-llama-stack scenario as
@skip-in-library-modeis consistent with the library/server mode split and prevents misuse of the external container in library mode.Containerfile (1)
76-79: Directory setup for llama-stack library mode is consistentProvisioning
/opt/app-root/src/.llama/distributions/ollamaand/opt/app-root/src/.llama/providers.dwith ownership1001:1001matches the non-root runtime user and the external_providers_dir used in the e2e configs.tests/e2e/features/health.feature (1)
43-52: Health endpoint llama-stack-down scenarios correctly skipped in library modeAdding
@skip-in-library-modeto the readiness and liveness scenarios that disrupt llama-stack is aligned with the new library/server mode split and avoids invalid container operations in library mode.Also applies to: 55-63
tests/e2e/features/info.feature (1)
21-30: Mode-aware skipping for llama-stack failure scenarios is appropriateAll added
@skip-in-library-modetags on the info/models/shields/tools “llama-stack unreachable” scenarios correctly restrict these to server mode and align with the newbefore_scenarioskip rule.Also applies to: 39-48, 57-66, 114-123
tests/e2e/features/query.feature (1)
110-120: Query 503-on-llama-stack-down scenario correctly gated to server modeAdding
@skip-in-library-modehere is consistent with the environment hook and other features: scenarios that disrupt llama-stack now only run when there is a separate llama-stack container (server mode).tests/e2e/features/environment.py (3)
85-88: Library-mode skipping and mode-specific invalid-feedback config paths make sense
- The
skip-in-library-modetag gate inbefore_scenariocorrectly skips scenarios that rely on a separate llama-stack container whencontext.is_library_modeis true.- The
InvalidFeedbackStorageConfigscenario configuration path being derived fromlibrary-modevsserver-modeensures each mode can use its own invalid-storage config without overloading a single file.It’d be worth double-checking that:
- Every scenario that disrupts the llama-stack container is tagged with
@skip-in-library-mode.- Both
tests/e2e/configuration/library-mode/lightspeed-stack-invalid-feedback-storage.yamlandtests/e2e/configuration/server-mode/lightspeed-stack-invalid-feedback-storage.yamlexist and mirror the previous single-file behavior.Also applies to: 91-93
101-106: Conditioning llama-stack restoration on non-library mode is appropriateGuarding the llama-stack restart logic with
not context.is_library_modeavoids attempting to manage a container that doesn’t exist in library mode, while preserving the previous restoration behavior for server mode.
156-159: Mode-specific auth-noop-token config wiring is good; confirm server/library configs and auth flagsUsing
mode_dir = "library-mode" if context.is_library_mode else "server-mode"for the@Authorizedfeature config keeps server and library modes clearly separated and lines up with the new config files.Given this, please verify that:
tests/e2e/configuration/server-mode/lightspeed-stack-auth-noop-token.yamlexists and retains the previous noop-with-token semantics for server mode.- The new library-mode variant (
tests/e2e/configuration/library-mode/lightspeed-stack-auth-noop-token.yaml) hasservice.auth_enabledset as intended for @Authorized tests (likelytrueso that noop-with-token actually enforces token presence), to avoid divergence between server and library mode auth behavior.This will ensure Authorized e2e coverage behaves consistently across both deployment modes.
tests/e2e/configuration/library-mode/lightspeed-stack-auth-noop-token.yaml (1)
1-20: Review comment cannot be fully verified without test execution resultsThe assumption that
auth_enabled: falsebreaks @Authorized semantics appears questionable: web documentation examples shownoop-with-tokenintentionally paired withauth_enabled: false. The learnings indicate deliberate config separation between noop (default) and noop-with-token (@Authorized) tests. Without confirming whether @Authorized tests currently pass or fail with the library-mode config, and without visibility into test-specific override logic in environment.py, the recommendation to changeauth_enabled: truecannot be substantiated.docker-compose-library.yaml (1)
1-37: Verify Containerfile exists and mount paths align with workflow expectations.The docker-compose-library.yaml configuration looks structurally sound, but a few details need verification:
Mount contexts: Lines 13-14 mount
./lightspeed-stack.yamland./run.yamlfrom the workflow working directory (should be generated by lines 65 and 119 of the workflow). Confirm these paths are predictable when the workflow runs in the repository root.SELinux mount option (
:Z): Line 13-14 use:Zfor relabeling. This is SELinux-specific and idempotent across containers but may not apply to all systems. If cross-platform compatibility is important, consider documenting this requirement or using conditional logic.Containerfile build context: Line 5-6 build from context
.usingContainerfile. Verify the Containerfile exists at the repository root and builds correctly in the CI environment.Healthcheck timing: The
start_period: 15son line 36 is appropriate for library initialization overhead, but if initialization can take longer (especially under load), this may need adjustment.Can you confirm:
- Containerfile exists and builds successfully?
- Are the mount paths (./lightspeed-stack.yaml, ./run.yaml) correctly staged by the workflow before docker-compose is invoked?
- Is the
:Zmount option intentional for SELinux, or should it be:zfor broader compatibility?.github/workflows/e2e_tests.yaml (4)
171-186: Verifydocker-compose-library.yamlexists in the repository root.The workflow references
docker-compose-library.yamlin the library mode startup conditional. Ensure this file is present in the repository root; otherwise, the workflow will fail when attempting to reference it.
12-15: Verify all 4 matrix job combinations have required config files.The 2D matrix (mode × environment) creates 4 job combinations: server/ci, server/azure, library/ci, library/azure. Before merging, ensure these configuration files exist:
tests/e2e/configuration/lightspeed-stack-server-mode.yamltests/e2e/configuration/lightspeed-stack-library-mode.yamltests/e2e/configs/run-ci.yamltests/e2e/configs/run-azure.yamlAlso verify
Containerfileis present and themake test-e2etarget respects theE2E_DEPLOYMENT_MODEenvironment variable.
52-66: Mode-specific config loading includes defensive checks; verify that referenced config files exist and have the expected YAML structure.The config loading logic includes good error handling and file existence validation. However, manual verification is needed to confirm that the config files (lightspeed-stack-server-mode.yaml and lightspeed-stack-library-mode.yaml) contain the expected YAML structure and keys expected by subsequent processing steps.
206-217: Verify thatmake test-e2etarget respects theE2E_DEPLOYMENT_MODEenvironment variable.The workflow propagates
E2E_DEPLOYMENT_MODEto the test step (line 210), but you should confirm themake test-e2etarget (or its underlying test runner) actually reads and uses this variable for conditional test logic. Check the Makefile for thetest-e2etarget definition and verify the test infrastructure (environment.py, conftest.py, or similar) readsE2E_DEPLOYMENT_MODEat runtime to configure test behavior based on deployment mode.
radofuchs
force-pushed
the
E2E_tests_library_mode
branch
from
0b175b7 to
af4b4e5
Compare
radofuchs
force-pushed
the
E2E_tests_library_mode
branch
from
2133c5d to
2a4bdf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Containerfile (1)
76-78: Library-mode directories and ownership look correct; consider centralizing the base pathPre-creating
/opt/app-root/src/.llama/...and chowning to1001:1001before switching toUSER 1001is the right pattern for allowing the non-root user to write provider/distribution data. This should work well with the library-mode tests and configs that reference this path.As a minor maintainability improvement, you could centralize the
.llamabase path in an env var so it’s not hard-coded in multiple places and is easier to change later. For example:-# Create llama-stack directories for library mode -RUN mkdir -p /opt/app-root/src/.llama/distributions/ollama /opt/app-root/src/.llama/providers.d && \ - chown -R 1001:1001 /opt/app-root/src/.llama +# Create llama-stack directories for library mode +ENV LLAMA_HOME=/opt/app-root/src/.llama +RUN mkdir -p "$LLAMA_HOME/distributions/ollama" "$LLAMA_HOME/providers.d" && \ + chown -R 1001:1001 "$LLAMA_HOME"This is optional, the current behavior is functionally fine.
tests/e2e/configs/run-ci.yaml (1)
19-19: Explicit external_providers_dir path looks reasonable; please verify container pathSetting
external_providers_dirto/opt/app-root/src/.llama/providers.davoids relying on defaults and matches the new provider-discovery model, but other paths (DBs) are rewritten to/app-root/.llama/distributionsin the workflow. Please double‑check that this directory actually exists in the CI image and that the/opt/app-root/srcvs/app-rootdistinction is intentional; if so, consider a brief comment or doc note so future changes don’t “normalize” it accidentally.tests/e2e/features/environment.py (1)
54-59: Mode detection + mode-specific config selection look consistent; consider centralizing mode_dirReading
E2E_DEPLOYMENT_MODEintocontext.deployment_mode/context.is_library_modeand then usingmode_dir = "library-mode" if context.is_library_mode else "server-mode"for both scenario and feature configs ties cleanly into the GitHub Actions matrix and the new directory layout. The@skip-in-library-modegating is also scoped correctly to only skip scenarios that depend on a separate llama-stack container.Two small follow‑ups:
- It may be worth storing
mode_dironce oncontextinbefore_all(e.g.,context.mode_dir) and reusing it inbefore_scenario/before_featureto avoid recomputing and to keep paths consistent if the logic ever changes.- Please verify that all of the referenced mode-specific files actually exist:
tests/e2e/configuration/server-mode/library-mode/lightspeed-stack-invalid-feedback-storage.yamltests/e2e/configuration/server-mode/library-mode/lightspeed-stack-no-cache.yamltests/e2e/configuration/server-mode/library-mode/lightspeed-stack-auth-noop-token.yaml
so CI doesn’t fail at runtime due to a missing config.Also applies to: 85-91, 92-97, 166-172
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/e2e_tests.yaml(7 hunks)Containerfile(1 hunks)Makefile(1 hunks)docker-compose-library.yaml(1 hunks)run.yaml(1 hunks)test.containerfile(1 hunks)tests/e2e/configs/run-azure.yaml(1 hunks)tests/e2e/configs/run-ci.yaml(1 hunks)tests/e2e/configs/run-rhelai.yaml(1 hunks)tests/e2e/configuration/library-mode/lightspeed-stack-auth-noop-token.yaml(1 hunks)tests/e2e/configuration/library-mode/lightspeed-stack-invalid-feedback-storage.yaml(1 hunks)tests/e2e/configuration/lightspeed-stack-library-mode.yaml(1 hunks)tests/e2e/configuration/lightspeed-stack-server-mode.yaml(1 hunks)tests/e2e/features/conversation_cache_v2.feature(2 hunks)tests/e2e/features/conversations.feature(2 hunks)tests/e2e/features/environment.py(4 hunks)tests/e2e/features/health.feature(2 hunks)tests/e2e/features/info.feature(4 hunks)tests/e2e/features/query.feature(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- tests/e2e/configs/run-rhelai.yaml
- tests/e2e/features/health.feature
- tests/e2e/features/conversations.feature
- tests/e2e/configuration/library-mode/lightspeed-stack-invalid-feedback-storage.yaml
- run.yaml
- Makefile
- tests/e2e/features/info.feature
- test.containerfile
- tests/e2e/configuration/lightspeed-stack-library-mode.yaml
- docker-compose-library.yaml
- tests/e2e/configuration/library-mode/lightspeed-stack-auth-noop-token.yaml
- tests/e2e/configs/run-azure.yaml
- tests/e2e/features/query.feature
- tests/e2e/configuration/lightspeed-stack-server-mode.yaml
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Files:
tests/e2e/features/conversation_cache_v2.feature
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/e2e/features/environment.py
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/e2e/test_list.txt : Maintain test list in `tests/e2e/test_list.txt` for end-to-end tests
Applied to files:
.github/workflows/e2e_tests.yaml
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to tests/e2e/features/**/*.feature : Use behave (BDD) framework with Gherkin feature files for end-to-end tests
Applied to files:
.github/workflows/e2e_tests.yamltests/e2e/features/environment.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (4)
tests/e2e/features/conversation_cache_v2.feature (1)
191-210: Library-mode skip tags correctly gate llama-stack–dependent scenariosUsing
@skip-in-library-modeon the “llama-stack is down” GET/DELETE scenarios aligns with the newcontext.is_library_mode+skip-in-library-modehandling inenvironment.py, so these tests still run in server mode but are cleanly skipped when there’s no separate llama-stack container. Taging/indentation are valid for Behave/Gherkin.Also applies to: 286-307
tests/e2e/features/environment.py (1)
114-120: Guarding llama-stack restoration by deployment mode avoids invalid docker operationsAdding
not context.is_library_modeto thellama_stack_was_runningcheck ensures the restore logic only runs when there is a separate llama-stack container, which is exactly what you want in server mode and avoids unnecessary/invaliddocker start/execcalls under library mode. The error handling aroundCalledProcessErrorand the health-check loop remain intact..github/workflows/e2e_tests.yaml (2)
11-16: Matrix + deployment-mode wiring + config selection are coherent; verify required files are presentThe mode/environment matrix,
E2E_DEPLOYMENT_MODEpropagation, and the newlightspeed-stack-${MODE}-mode.yamlloading step line up nicely with the new test hooks:
matrix.modedrives both the job name andE2E_DEPLOYMENT_MODE, whichenvironment.pyconsumes viacontext.deployment_mode.- Mode-specific
lightspeed-stack-${MODE}-mode.yamlis copied intolightspeed-stack.yamlwith an explicit existence check and a clear failure message.run.yamlremains environment-specific (run-ci.yaml/run-azure.yaml), and the workflow prints which config was used plus a small preview, which should make debugging much easier.Please just verify that all of the following exist and are kept in sync with the test expectations:
tests/e2e/configuration/lightspeed-stack-server-mode.yamltests/e2e/configuration/lightspeed-stack-library-mode.yamltests/e2e/configs/run-ci.yaml,tests/e2e/configs/run-azure.yamlso that new matrix combinations don’t fail purely due to missing config.
Also applies to: 23-23, 52-67, 94-97, 137-147
149-187: Server vs library docker-compose flows and failure logging look solidSplitting startup into:
- Server mode:
docker compose up -dwith basic health checks and full logs on failure.- Library mode:
docker compose -f docker-compose-library.yaml up -dwith mode-specific ps/logs handling,and then using the same quick connectivity probe plus mode-aware log dumping on both initial connectivity failure and test failure is a clean way to keep both modes covered without duplicating too much logic. The env handling (OpenAI / Azure keys via
GITHUB_ENV) is scoped to the job and you only print presence/length, not the secret values.Main thing to keep an eye on is that
docker-compose-library.yamldefines alightspeed-stackservice as expected; otherwise the library-mode log commands will fail. Once that’s confirmed, this looks ready to exercise both modes in CI.Also applies to: 193-205, 219-233
1 participant
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
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.