fix: changed integration tests to save logs instead of dumping them#1991
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates CI trigger filters, Docker cleanup, build parallelism, and failure log handling across the workflow, Makefile, and integration script. ChangesCI workflow and log handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
bug
![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
🧹 Nitpick comments (1)
Makefile (1)
810-933: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test-citarget missing the same failure log-saving logic added totest-ci-local.The new
service-logs/capture block (mkdir + per-servicelogsredirection) was only added totest-ci-local(Lines 1053-1060). The near-identicaltest-citarget ends at Line 933 withexit $$TEST_RESULTand still has no failure-log capture at all, so manual/local runs ofmake test-ciget no log artifacts on failure whiletest-ci-localdoes.♻️ Suggested fix: mirror the block before `test-ci`'s exit
+ if [ $$TEST_RESULT -ne 0 ]; then \ + echo "$(RED)=== Tests failed, saving container logs to service-logs/ ===$(NC)"; \ + mkdir -p service-logs; \ + $(CONTAINER_RUNTIME) logs langflow > service-logs/langflow.log 2>&1 || echo "$(RED)Could not get Langflow logs$(NC)"; \ + $(CONTAINER_RUNTIME) logs openrag-backend > service-logs/backend.log 2>&1 || echo "$(RED)Could not get backend logs$(NC)"; \ + $(CONTAINER_RUNTIME) logs openrag-frontend > service-logs/frontend.log 2>&1 || echo "$(RED)Could not get frontend logs$(NC)"; \ + $(CONTAINER_RUNTIME) logs os > service-logs/opensearch.log 2>&1 || echo "$(RED)Could not get OpenSearch logs$(NC)"; \ + fi; \ ($(call test_jwt_opensearch)) || TEST_RESULT=1; \Also applies to: 1053-1060
🤖 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 `@Makefile` around lines 810 - 933, The test-ci target is missing the same failure log-saving behavior already added in test-ci-local, so failing runs don’t preserve service logs. Mirror the existing service-logs capture block from test-ci-local into test-ci near the end of the recipe, before exit $$TEST_RESULT, and make it save per-service logs only when the overall test result is non-zero. Use the existing test-ci and test-ci-local targets plus the service-logs redirection pattern to keep both targets consistent.
🤖 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 `@scripts/ci/run_integration_suite.sh`:
- Around line 70-77: The dump_logs function in run_integration_suite.sh is
writing full container logs for langflow, openrag-backend, openrag-frontend, and
os straight into service-logs files, which are then uploaded as CI artifacts.
Update this flow to redact known sensitive patterns before the logs are
persisted, or otherwise limit what gets written by the dump_logs helper and the
related artifact upload path, so the saved logs do not expose secrets or tokens.
---
Nitpick comments:
In `@Makefile`:
- Around line 810-933: The test-ci target is missing the same failure log-saving
behavior already added in test-ci-local, so failing runs don’t preserve service
logs. Mirror the existing service-logs capture block from test-ci-local into
test-ci near the end of the recipe, before exit $$TEST_RESULT, and make it save
per-service logs only when the overall test result is non-zero. Use the existing
test-ci and test-ci-local targets plus the service-logs redirection pattern to
keep both targets consistent.
🪄 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: CHILL
Plan: Pro
Run ID: 3746d206-60bd-4e6c-a9a6-e7da12083f9e
📒 Files selected for processing (3)
.github/workflows/test-ci.ymlMakefilescripts/ci/run_integration_suite.sh
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test-ci.yml (1)
41-63: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the workflow token permissions. Add job-level
permissionshere with at leastcontents: readandpull-requests: readsoactions/checkoutanddorny/paths-filterdon’t inherit broader repo defaults.Suggested change
changes: runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read outputs:🤖 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 @.github/workflows/test-ci.yml around lines 41 - 63, The changes job in the test-ci workflow currently inherits broader default token scopes; add an explicit job-level permissions block for the changes job with at least contents: read and pull-requests: read so actions/checkout and dorny/paths-filter run with least privilege. Locate the job by its changes key and keep the permission scope limited to that job only.Source: Linters/SAST tools
🤖 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 @.github/workflows/test-ci.yml:
- Around line 59-63: The integration path filter is excluding frontend changes,
which prevents frontend updates from triggering suites that depend on the
frontend app. Update the workflow’s path rules in the integration job so
`frontend/**` is included for integration runs, while keeping the other
exclusions if needed. Use the existing `integration` paths block in the CI
workflow to adjust the matching behavior so changes that affect
`scripts/ci/run_integration_suite.sh`’s frontend-backed tests still run.
---
Outside diff comments:
In @.github/workflows/test-ci.yml:
- Around line 41-63: The changes job in the test-ci workflow currently inherits
broader default token scopes; add an explicit job-level permissions block for
the changes job with at least contents: read and pull-requests: read so
actions/checkout and dorny/paths-filter run with least privilege. Locate the job
by its changes key and keep the permission scope limited to that job only.
🪄 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: CHILL
Plan: Pro
Run ID: 3a1b0c4d-d45f-4afb-976e-a9d3e3dd98b5
📒 Files selected for processing (1)
.github/workflows/test-ci.yml
lucaseduoli
force-pushed
the
fix/tests_integration_e2e
branch
from
June 30, 2026 19:14
3990e95 to
b7cae96
Compare
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Collaborator
|
Good One! I would also see if we can generate a error report also Like which test was failing and etc not the full log may be that can help for more effective debug. Like a Test Failure Report. @coderabbitai create a plan for the above comment and your suggestions too. |
github-actions
Bot
added
bug
github-actions
Bot
added
bug
…on suite (#1992) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
github-actions
Bot
added
bug
edwinjosechittilappilly
approved these changes
Jun 30, 2026
edwinjosechittilappilly
left a comment
edwinjosechittilappilly
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, awaiting test in current PR.
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Makefile (2)
1073-1075: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame non-best-effort report generation in
test-ci-local.Line 1074 is unguarded; under
set -ea generator failure skips teardown (1077-1079) andexit $$TEST_RESULT. Apply|| trueas in thetest-cifix.🤖 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 `@Makefile` around lines 1073 - 1075, The test failure report generation in test-ci-local is still unguarded, so a failure in generate_test_report.py can abort the target before teardown and exit handling run. Update the test-ci-local recipe to match the test-ci fix by making the service-logs report generation non-fatal, using the same guard pattern around the generate_test_report.py call so the cleanup and exit $$TEST_RESULT steps still execute. Reference the test-ci-local Makefile target and the generate_test_report.py invocation when applying the change.
1030-1039: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame
set -ecore-pytest scoping bug intest-ci-local.Identical to the
test-cicore step: Line 1038 runsuv run pytestunguarded and Line 1039 captures$?. Underset -ea core failure aborts before the SDK suites, container-log capture (1065-1072), report generation, and teardown. Apply the sameTEST_RESULT=0; ... || TEST_RESULT=$$?fix.🤖 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 `@Makefile` around lines 1030 - 1039, The `test-ci-local` core pytest step has the same `set -e` scoping bug as `test-ci`: `uv run pytest` in the core block can abort the shell before `TEST_RESULT=$$?` is assigned, which skips later SDK runs, log collection, report generation, and teardown. Update the core test block in the Makefile to initialize `TEST_RESULT=0` and capture failures with `|| TEST_RESULT=$$?` around the `uv run pytest tests/integration/core` invocation, matching the existing `test-ci` fix so `TEST_RESULT` is always set before the script continues.
🤖 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 `@Makefile`:
- Around line 900-909: The core pytest step in the Makefile recipe is not
guarded, so under set -e a failure exits before TEST_RESULT can be captured and
the remaining SDK suites, reports, and teardown run. Update the core integration
test invocation around the pytest command to use the same failure-capture
pattern as the later test steps, keeping TEST_RESULT assigned even when tests
fail; use the existing surrounding recipe block and the pytest/core test command
as the anchor points.
- Around line 935-937: The test report generation step in the Makefile is not
best-effort, so a failure in the report generator can abort the recipe under set
-e and prevent teardown and the final test exit from running. Update the
report-generation block around the Test Failure Report echo/uv run sequence to
ignore generator failures, following the same best-effort pattern used in
run_integration_suite.sh, so the remaining cleanup and exit logic still
executes.
---
Duplicate comments:
In `@Makefile`:
- Around line 1073-1075: The test failure report generation in test-ci-local is
still unguarded, so a failure in generate_test_report.py can abort the target
before teardown and exit handling run. Update the test-ci-local recipe to match
the test-ci fix by making the service-logs report generation non-fatal, using
the same guard pattern around the generate_test_report.py call so the cleanup
and exit $$TEST_RESULT steps still execute. Reference the test-ci-local Makefile
target and the generate_test_report.py invocation when applying the change.
- Around line 1030-1039: The `test-ci-local` core pytest step has the same `set
-e` scoping bug as `test-ci`: `uv run pytest` in the core block can abort the
shell before `TEST_RESULT=$$?` is assigned, which skips later SDK runs, log
collection, report generation, and teardown. Update the core test block in the
Makefile to initialize `TEST_RESULT=0` and capture failures with `||
TEST_RESULT=$$?` around the `uv run pytest tests/integration/core` invocation,
matching the existing `test-ci` fix so `TEST_RESULT` is always set before the
script continues.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 599ea375-7ec6-42b9-9b06-63d67fb472dd
📒 Files selected for processing (3)
Makefilescripts/ci/generate_test_report.pyscripts/ci/run_integration_suite.sh
✅ Files skipped from review due to trivial changes (1)
- scripts/ci/generate_test_report.py
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
github-actions
Bot
added
bug
lucaseduoli
force-pushed
the
fix/tests_integration_e2e
branch
from
June 30, 2026 21:44
db70b47 to
fd876c3
Compare
github-actions
Bot
added
bug
This was referenced Jul 14, 2026
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.
This pull request improves how test failure logs are handled and makes them more accessible for debugging, especially in CI environments. Instead of just dumping service logs to the console on test failures, logs are now saved to a
service-logs/directory and uploaded as artifacts in GitHub Actions, making it easier to review logs after a failed run.CI/CD improvements:
.github/workflows/test-ci.yml: Added a step to upload theservice-logs/directory as a GitHub Actions artifact when tests fail, with a retention period of 7 days.Test log handling improvements:
Makefile: Changed the test failure handling to save logs from all main services (langflow,openrag-backend,openrag-frontend,os) to individual files in theservice-logs/directory instead of printing them to the console.scripts/ci/run_integration_suite.sh: Updated thedump_logsfunction to save logs for each service to separate files inservice-logs/rather than echoing the last N lines to the console.Summary by CodeRabbit
service-logs/, redacting sensitive values, and persisting per-suite pytest logs.service-logs/for easier diagnosis.