LEADS-247: add lightspeed evaluation e2e tests#1313
LEADS-247: add lightspeed evaluation e2e tests#1313radofuchs merged 1 commit intolightspeed-core:mainfrom
Conversation
WalkthroughA new GitHub Actions workflow is added to run Lightspeed Evaluation end-to-end tests: it checks out repos, loads lightspeed-evaluation config, starts services via Docker Compose, waits for the LSC service, runs test commands, exposes logs on failure, and performs cleanup. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220, 230, 241, 0.5)
participant GH as GitHub Actions
end
rect rgba(200, 240, 200, 0.5)
participant Repo as Repository Checkout
participant Eval as lightspeed-evaluation
end
rect rgba(250, 230, 200, 0.5)
participant Docker as Docker Compose
participant LSC as LSC Service (port 8080)
participant Tests as Test Runner
end
GH->>Repo: checkout repository (handle PR_TARGET)
GH->>Eval: fetch & load lightspeed-evaluation config
GH->>Docker: start services (docker-compose-library.yaml)
Docker->>LSC: bring up LSC service
GH->>LSC: wait for health / port 8080 readiness
GH->>Tests: run e2e steps (install, sync, make e2e_tests_lcore)
Tests-->>GH: report results
alt failure
GH->>Docker: collect container logs
end
GH->>Docker: cleanup containers and images (local dev step)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 for PR comments
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 |
0fda973 to
525d625
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/e2e_tests_lightspeed_evaluation.yaml (2)
103-117: Consider reducing log verbosity during polling.Showing 30 lines of logs on every 2-second iteration (line 111) produces very verbose output. Consider only showing logs on final timeout failure or reducing frequency. Also, adding
--silentto curl would suppress the progress meter.♻️ Suggested improvement
- name: Wait for the LSC run: | echo "Waiting for service on port 8080..." for i in {1..30}; do - if curl --output /dev/null --fail http://localhost:8080/v1/models ; then + if curl --silent --output /dev/null --fail http://localhost:8080/v1/models ; then echo "Service is up!" exit 0 fi - docker compose -f docker-compose-library.yaml logs --tail=30 + # Show logs less frequently to reduce noise + if [ $((i % 5)) -eq 0 ]; then + docker compose -f docker-compose-library.yaml logs --tail=10 + fi echo "Still waiting..." sleep 2 done echo "Service did not start in time" + docker compose -f docker-compose-library.yaml logs --tail=50 exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e_tests_lightspeed_evaluation.yaml around lines 103 - 117, The polling loop in the "Wait for the LSC" step is overly noisy because it runs curl without --silent and prints `docker compose -f docker-compose-library.yaml logs --tail=30` on every iteration; change the curl call to include `--silent` and move or gate the docker compose logs so they only print on the final timeout (or at a reduced frequency, e.g., every 5th iteration) instead of every 2 seconds, updating the existing loop that checks `curl http://localhost:8080/v1/models` and the `docker compose ... logs --tail=30` invocation accordingly.
6-15: Consider adding concurrency control to prevent parallel runs.Multiple workflow runs on the same PR (e.g., rapid pushes) could interfere with each other. Adding a concurrency group would cancel in-progress runs when a new one starts.
♻️ Suggested improvement
jobs: e2e_tests: runs-on: ubuntu-latest + concurrency: + group: e2e-lightspeed-${{ github.head_ref || github.ref }} + cancel-in-progress: true strategy: fail-fast: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e_tests_lightspeed_evaluation.yaml around lines 6 - 15, The workflow lacks concurrency control which can cause overlapping runs for the same PR; add a concurrency section at the top-level of the workflow (near jobs or the existing top keys) to define a group that uniquely identifies the PR/run (e.g., use github.ref or github.head_ref) and set cancel-in-progress: true so the e2e_tests job (name: "E2E Tests for Lightspeed Evaluation job", job id: e2e_tests) will cancel any in-flight run when a new run for the same PR starts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e_tests_lightspeed_evaluation.yaml:
- Line 1: Update the top-line comment to reference the actual file name: change
the commented filename string "# .github/workflows/e2e_tests.yml" to "#
.github/workflows/e2e_tests_lightspeed_evaluation.yaml" so the comment matches
the file name (ensure the exact string e2e_tests_lightspeed_evaluation.yaml is
used).
- Around line 30-32: The workflow uses github.event.pull_request.head.ref
(branch name) when constructing ref for checkout, which is not a reproducible
commit; replace github.event.pull_request.head.ref with
github.event.pull_request.head.sha so ref becomes
github.event.pull_request.head.sha || github.sha to guarantee the workflow
checks out the exact PR commit SHA (use this change where ref: ${{ ... }} is
set).
- Around line 141-148: The workflow uses an invalid command string "docker
compose down lightspeed-stack --rmi \"all\""; fix it by removing the service
name (down takes no services) and either call "docker compose down --rmi all" to
tear down the entire composition or, if you intended to stop/remove only the
lightspeed-stack service, run "docker compose stop lightspeed-stack" followed by
"docker compose rm -f lightspeed-stack"; ensure any non-default compose file is
passed with -f if needed.
---
Nitpick comments:
In @.github/workflows/e2e_tests_lightspeed_evaluation.yaml:
- Around line 103-117: The polling loop in the "Wait for the LSC" step is overly
noisy because it runs curl without --silent and prints `docker compose -f
docker-compose-library.yaml logs --tail=30` on every iteration; change the curl
call to include `--silent` and move or gate the docker compose logs so they only
print on the final timeout (or at a reduced frequency, e.g., every 5th
iteration) instead of every 2 seconds, updating the existing loop that checks
`curl http://localhost:8080/v1/models` and the `docker compose ... logs
--tail=30` invocation accordingly.
- Around line 6-15: The workflow lacks concurrency control which can cause
overlapping runs for the same PR; add a concurrency section at the top-level of
the workflow (near jobs or the existing top keys) to define a group that
uniquely identifies the PR/run (e.g., use github.ref or github.head_ref) and set
cancel-in-progress: true so the e2e_tests job (name: "E2E Tests for Lightspeed
Evaluation job", job id: e2e_tests) will cancel any in-flight run when a new run
for the same PR starts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 424b6ec4-d9a7-4ae9-b414-78e6246a4c79
📒 Files selected for processing (1)
.github/workflows/e2e_tests_lightspeed_evaluation.yaml
525d625 to
f6a7c52
Compare
Description
add lightspeed evaluation e2e tests
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
Summary by CodeRabbit
Chores
Tests