ci(checks): run the test suite on Linux and Windows - #234
Conversation
`checks.yml` runs clippy, fmt, license checks and cargo audit, but never `cargo test`, while `release.yml` compiles, signs and ships a Windows binary. A test that only fails on Windows can therefore reach a release unnoticed, which is what Piebald-AI#226 turned out to be. The new `test` job uses the matrix rather than the Linux-only version, per the preference stated in Piebald-AI#231. `fail-fast: false` matters here: without it a failure on one OS cancels the other, and telling platform-specific failures apart is the whole point of the job. `test` is added to the `needs` list of `all-checks`. Without that the job would run and report, but a failure would not block the merge.
|
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
📝 WalkthroughWalkthroughThe checks workflow adds a matrix test job for Ubuntu and Windows. The job installs nightly Rust with Cranelift and runs ChangesCI testing
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/checks.yml:
- Around line 63-65: Restrict the test job permissions by adding a job-level
permissions block under the test job, granting only contents: read for checkout
and leaving all other permissions unavailable.
- Around line 74-75: Update the tests job’s actions/checkout@v4 step to set
persist-credentials to false, ensuring checkout credentials are not retained for
subsequent repository-controlled commands.
- Around line 76-82: Update the workflow’s Rust test configuration around the
“Install Rust toolchain” and “Run tests” steps to opt Cargo into the installed
rustc-codegen-cranelift-preview backend and configure the test profile to use
Cranelift. Ensure cargo test --locked explicitly runs with Cranelift rather than
falling back to LLVM.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8324778c-08c0-4a37-9567-4020957cd06a
📒 Files selected for processing (1)
.github/workflows/checks.yml
|
CodeRabbit flagged three things, and all three apply to the jobs already on The missing job-level The Cranelift point is correct on the facts. I have left all three alone. Fixing them only in the job I am adding would make it the one job that differs from its neighbours, which seems worse than leaving them consistent. I can open a separate PR covering all of them together if you want that. |
|
@NickAme03 I'll just fix them in a seperate PR, you don't need to. Merging. |
I'll fix them in a seperate PR.
|
Merged current The 08-02 run was red on one test, not on the job. Green on my fork on the same tree (
I ran it there because the run here is sitting on The diff is unchanged: the same 22 lines adding the #231 still describes |
Closes #231.
Adds a
testjob tochecks.yml, in the matrix version you preferred rather than the Linux-only one.fail-fast: falseis deliberate: without it a failure on one OS cancels the other, and telling platform-specific failures apart is the whole point of running both.testis also added to theneedslist ofall-checks. Without that the job would run and report, but a failure would not block a merge, which is the state the issue describes.Verified on my fork before opening this, since the
pushtrigger here only coversmain. Both platforms green, andall-checkspicks up the new dependency: run 30719515514.The Windows row is the one worth checking: the suite passes there today, so this job goes in green rather than red.
Summary by CodeRabbit