Skip to content

chore: Rust format and testing in CI#2205

Open
aclauer wants to merge 8 commits into
mainfrom
andrew/chore/rust-tests-in-ci
Open

chore: Rust format and testing in CI#2205
aclauer wants to merge 8 commits into
mainfrom
andrew/chore/rust-tests-in-ci

Conversation

@aclauer
Copy link
Copy Markdown
Collaborator

@aclauer aclauer commented May 21, 2026

Problem

Need to run Rust formatting checks and tests in CI

Closes DIM-938

Solution

Added auto discovery job to find all Rust workspaces in the repository based on Cargo.lock. Then run cargo fmt, cargo clippy, cargo test in all work spaces in parallel.

How to Test

Run in CI, result of all three commands matches local results. All workspaces are automatically detected without any hardcoded paths.

Contributor License Agreement

  • I have read and approved the CLA.

@aclauer aclauer linked an issue May 21, 2026 that may be closed by this pull request
@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@aclauer aclauer marked this pull request as ready for review May 21, 2026 16:56
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 21, 2026

Greptile Summary

This PR adds a rust CI job that auto-discovers all Cargo workspaces in the repo by searching for Cargo.lock files, then sequentially runs cargo fmt --check, cargo clippy, and cargo test in each workspace. The job is wired into ci-complete so it gates branch protection.

  • Auto-discovery: find locates every Cargo.lock outside target/, .venv/, node_modules/, and .git/, extracting parent directories as workspace roots — no hardcoded paths needed.
  • Consistent flags: all three Cargo commands use --workspace (or --all for fmt) and --locked, ensuring full workspace coverage and reproducible builds.
  • Empty-workspace safety: the for-loop approach, rather than a matrix, means an empty workspace list simply skips all loop bodies and exits 0 — avoiding the "matrix must have at least one element" error that a matrix strategy would trigger.

Confidence Score: 5/5

Safe to merge — the new rust CI job correctly discovers workspaces, runs all three checks with workspace-scoped flags, and integrates cleanly with ci-complete.

The implementation is straightforward and avoids the matrix-empty-list pitfall by using a for loop. All three Cargo commands use consistent, workspace-wide flags. The only note is that unquoted $CRATES expansion would misbehave for paths with spaces, which is very unlikely in practice but easy to harden.

No files require special attention.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Adds a new rust job that auto-discovers Cargo workspaces via find/Cargo.lock, installs the stable toolchain with rustfmt+clippy, and runs cargo fmt, cargo clippy, and cargo test sequentially for each workspace; also wires rust into ci-complete.

Sequence Diagram

sequenceDiagram
    participant GHA as GitHub Actions
    participant find as find (Cargo.lock)
    participant toolchain as dtolnay/rust-toolchain
    participant cache as Swatinem/rust-cache
    participant cargo as cargo (per workspace)

    GHA->>find: Search repo for Cargo.lock files
    find-->>GHA: Newline-separated workspace paths → GITHUB_OUTPUT
    GHA->>toolchain: Install stable + rustfmt + clippy
    GHA->>cache: Cache cargo build (workspaces list)
    loop For each workspace path
        GHA->>cargo: cargo fmt --all -- --check
        cargo-->>GHA: exit 0 / non-zero
        GHA->>cargo: cargo clippy --workspace --all-targets --all-features --locked -D warnings
        cargo-->>GHA: exit 0 / non-zero
        GHA->>cargo: cargo test --workspace --all-features --locked
        cargo-->>GHA: exit 0 / non-zero
    end
    GHA->>GHA: ci-complete evaluates rust job result
Loading

Reviews (5): Last reviewed commit: "Merge branch 'main' into andrew/chore/ru..." | Re-trigger Greptile

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
@leshy
Copy link
Copy Markdown
Member

leshy commented May 22, 2026

@Dreamsorcerer can you review?

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
leshy and others added 4 commits May 22, 2026 16:45
Co-authored-by: Sam Bull <git@sambull.org>
Co-authored-by: Sam Bull <git@sambull.org>
Co-authored-by: Sam Bull <git@sambull.org>
leshy
leshy previously approved these changes May 22, 2026
@leshy leshy enabled auto-merge (squash) May 22, 2026 13:46
Comment thread .github/workflows/ci.yml Outdated
Comment on lines +71 to +72
matrix:
crate: ${{ fromJSON(needs.rust-discover.outputs.crates) }}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One blocking detail here I think. I don't think we should be using a matrix here, atleast from what I see of the current run.

Currently, this results in 3 separate jobs, requiring 3 workers. Each job only takes ~20s with about 50% of that time doing the setup/teardown of the job.

So currently this is basically 3 x 20s, instead of 1 x 40s. The difference today is neglible, but if we start adding in more crates over time, then this will lead to worker starvation and delay the other (slower) test jobs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops I approved, but disabled auto-merge, letting you guys decide, just ping me when you need approval

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, I'll change that

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed now, also removed the separate discover job

@leshy leshy disabled auto-merge May 22, 2026 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Rust tests to CI

3 participants