CI: Extract sandbox tests to their own job#7888
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CI workflow adds ChangesCI Sandbox Test Extraction
Sequence DiagramsequenceDiagram
participant GitHubActions
participant determine_changes
participant sandbox_test_job
participant rust_tests_job
GitHubActions->>determine_changes: run diff checks (Lib/** excl Lib/test/**, extra_tests/**)
determine_changes->>GitHubActions: set outputs (rust_code, upstream_lib, extra_tests)
GitHubActions->>sandbox_test_job: conditional start (outputs true or branch main)
sandbox_test_job->>sandbox_test_job: checkout, restore cache, install toolchain
sandbox_test_job->>sandbox_test_job: cargo check (sandbox, no host_env)
sandbox_test_job->>sandbox_test_job: cargo build (freeze-stdlib)
sandbox_test_job->>sandbox_test_job: run sandbox smoke test scripts
sandbox_test_job->>sandbox_test_job: run ssl-openssl build
GitHubActions->>rust_tests_job: continue without sandbox steps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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)
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: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
179-225: ⚡ Quick winConsider adding a timeout to prevent indefinite hangs.
Other test-related jobs in this workflow specify timeouts (e.g.,
rust_testshas 45 minutes,mirihas 30 minutes). Adding a timeout to this job would prevent it from hanging indefinitely if compilation or smoke tests encounter issues.⏱️ Suggested timeout addition
sandbox_test: runs-on: ubuntu-latest + timeout-minutes: 30 needs: - determine_changes🤖 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/ci.yaml around lines 179 - 225, The sandbox_test job currently lacks a timeout and can hang indefinitely; add a timeout-minutes value (e.g., timeout-minutes: 45) to the sandbox_test job definition so the GitHub Actions runner will cancel the job after the specified time; locate the sandbox_test job block (job id: sandbox_test) and insert the timeout-minutes key at the job level (alongside runs-on/name/needs) to match other jobs like rust_tests or miri.
🤖 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/ci.yaml:
- Around line 183-190: The CI condition is referencing
needs.determine_changes.outputs.extra_tests (plural) but the actual output is
named extra_test (singular), causing the check to miss changes; update the
condition to use needs.determine_changes.outputs.extra_test (or alternatively
rename the output in the determine_changes job to extra_tests) so the boolean
produced by determine_changes (extra_test) is evaluated correctly alongside
needs.determine_changes.outputs.rust_code and
needs.determine_changes.outputs.upstream_lib and the job triggers when files
under extra_tests/** are modified.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 179-225: The sandbox_test job currently lacks a timeout and can
hang indefinitely; add a timeout-minutes value (e.g., timeout-minutes: 45) to
the sandbox_test job definition so the GitHub Actions runner will cancel the job
after the specified time; locate the sandbox_test job block (job id:
sandbox_test) and insert the timeout-minutes key at the job level (alongside
runs-on/name/needs) to match other jobs like rust_tests or miri.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 5713da13-cbe0-46d8-a019-89c8a1a19ffc
📒 Files selected for processing (1)
.github/workflows/ci.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yaml (1)
183-190:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the output reference mismatch.
Line 188 references
extra_testsbut the output is defined asextra_test(singular) on line 49. This will cause the condition to always evaluate the wrong variable, and the job won't trigger when onlyextra_tests/**files are modified.🐛 Proposed fix
!contains(github.event.pull_request.labels.*.name, 'skip:ci') && ( needs.determine_changes.outputs.rust_code == 'true' || needs.determine_changes.outputs.upstream_lib == 'true' || - needs.determine_changes.outputs.extra_tests == 'true' + needs.determine_changes.outputs.extra_test == 'true' ) ) || github.ref == 'refs/heads/main'🤖 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/ci.yaml around lines 183 - 190, The condition in the CI job references needs.determine_changes.outputs.extra_tests but the actual output from the determine_changes job is named extra_test; update the reference so both names match (either change needs.determine_changes.outputs.extra_tests to needs.determine_changes.outputs.extra_test in the conditional or rename the determine_changes job output to extra_tests) so the conditional block that checks extra_test(s) (the needs.determine_changes.outputs.* entry used in the if clause) evaluates correctly.
🤖 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.
Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 183-190: The condition in the CI job references
needs.determine_changes.outputs.extra_tests but the actual output from the
determine_changes job is named extra_test; update the reference so both names
match (either change needs.determine_changes.outputs.extra_tests to
needs.determine_changes.outputs.extra_test in the conditional or rename the
determine_changes job output to extra_tests) so the conditional block that
checks extra_test(s) (the needs.determine_changes.outputs.* entry used in the if
clause) evaluates correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 12de6e6b-7dce-479c-8bea-cb1eef00e1a9
📒 Files selected for processing (1)
.github/workflows/ci.yaml
|
I am not sure how this help CI optimization. could you explain more? |
Sure, the idea is to make I'm decoupling the |
|
isn't host_env expected to share the same triggering condition? |
host_env is tested as long as a PR doesn't have the |
2 participants
related to #7883.
I'll continue optimizing the CI so only necessary jobs trigger on their related changes
Summary by CodeRabbit