chore(rs): run tests under nextest, and stop wasting CPU on RSA keygen - #2558
Conversation
Plain `cargo test` has no timeout. A test that parks with nothing to wake it never returns, so the harness never exits: it holds the target lock and burns a core until someone kills it by hand. That is not hypothetical here, since a lost `kio` wakeup is a hang rather than a failure, and it cost two such runs while reviewing #2556. Add `.config/nextest.toml` with a `slow-timeout` that terminates, turning a hang into a TIMEOUT failure in bounded time (SLOW at 30s, killed at 60s; double that on CI). Point `just rs test` at nextest to match what `just rs ci` already ran, and give doctests their own recipe rather than dropping them silently, since nextest does not run them. For that timeout to mean anything the honest tests have to be fast, and the moq-token RSA tests were not: six 2048-bit keygens across two tests, ~16s, because `rsa` and its bignum backend are orders of magnitude slower unoptimized. Give those three dependencies an `opt-level` override in the dev profile, which test builds inherit. The tests still generate production-size keys and now take 0.8s; the full workspace suite drops from 87s to 35s, and the slowest single test from 35s to under 9s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughUpdated Rust test infrastructure to use cargo nextest for standard test runs while retaining cargo test for doctests. Added shared default and CI nextest profiles with bounded timeouts, disabled retries, and CI failure output settings. Expanded testing guidance to document the new commands and timeout expectations. Added development profile optimization overrides for RSA and bignum-related dependencies. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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: 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 @.config/nextest.toml:
- Around line 24-28: Update the Rust CI test command in the justfile’s nextest
invocation to explicitly select the existing ci profile, using --profile ci or
the equivalent NEXTEST_PROFILE=ci configuration. Preserve the current workspace,
target, and feature arguments so the profile’s timeout and failure-output
settings apply in CI.
In `@Cargo.toml`:
- Around line 126-132: Remove the stale before/after timing claims from the
optimization comment in Cargo.toml lines 126-132, while retaining the bignum
dependency optimization rationale and production key-size guarantee. Also update
rs/CLAUDE.md lines 153-160 to retain the SLOW-test policy and opt-level
guidance, removing the workspace and RSA timing claims; both sites require
direct documentation-only edits.
In `@rs/CLAUDE.md`:
- Around line 142-143: Update the command guidance near the just and cargo
examples in the project documentation to state that these commands should run
inside the Nix development shell to match CI tooling. Retain direct cargo or bun
invocation as the fallback when Nix is unavailable.
🪄 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: d9f31879-a576-4a05-935a-60fe478cd477
📒 Files selected for processing (4)
.config/nextest.tomlCargo.tomlrs/CLAUDE.mdrs/justfile
| # Optimize the bignum crates even in dev/test builds. RSA keygen is bignum | ||
| # arithmetic in a loop, and unoptimized it is orders of magnitude slower: the | ||
| # moq-token key tests spend ~16s generating 2048-bit keys, versus well under a | ||
| # second here. These are dependencies we never step through in a debugger, so | ||
| # optimizing them costs nothing an unoptimized build was buying us, and the | ||
| # tests keep exercising production key sizes rather than being weakened to | ||
| # stay fast. `profile.test` inherits these overrides. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove stale before/after timings from both comments.
Both comments should explain the current optimization policy without embedding migration measurements that will become inaccurate.
Cargo.toml#L126-L132: retain the dependency optimization rationale and production key-size guarantee; remove the fixed timing comparison.rs/CLAUDE.md#L153-L160: retain the SLOW-test policy andopt-levelguidance; remove the workspace and RSA timing claims.
As per coding guidelines, comments and documentation must describe the current behavior, not historical migration context or obsolete behavior.
📍 Affects 2 files
Cargo.toml#L126-L132(this comment)rs/CLAUDE.md#L153-L160
🤖 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 `@Cargo.toml` around lines 126 - 132, Remove the stale before/after timing
claims from the optimization comment in Cargo.toml lines 126-132, while
retaining the bignum dependency optimization rationale and production key-size
guarantee. Also update rs/CLAUDE.md lines 153-160 to retain the SLOW-test policy
and opt-level guidance, removing the workspace and RSA timing claims; both sites
require direct documentation-only edits.
Source: Coding guidelines
| - `just check` runs all tests + lint; `just fix` auto-fixes formatting/lint. `just rs test -p <crate>` (or `cargo nextest run -p <crate>`) for one crate. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the Nix-shell requirement for these commands.
The repository guideline requires project commands to run in the Nix development shell so local tooling matches CI. Add a short scope note around the new just and cargo commands, retaining direct Cargo as the fallback when Nix is unavailable.
As per coding guidelines, use the Nix development shell for project commands so local runs match CI tooling; if unavailable, use cargo or bun directly.
🤖 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 `@rs/CLAUDE.md` around lines 142 - 143, Update the command guidance near the
just and cargo examples in the project documentation to state that these
commands should run inside the Nix development shell to match CI tooling. Retain
direct cargo or bun invocation as the fallback when Nix is unavailable.
Source: Coding guidelines
The [profile.ci] block added alongside the nextest switch was dead config: nothing set NEXTEST_PROFILE and `just rs ci` ran a bare `cargo nextest run`, so CI silently used profile.default and the longer timeout the comment described never applied. Pass --profile ci so it does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Two related annoyances, both about tests eating CPU for no benefit.
Plain
cargo testhas no timeout. A test that parks with nothing to wake it never returns, so the harness never exits: it holds the target lock and burns a core until someone kills it by hand. That is not hypothetical in this repo, since a lostkiowakeup is a hang rather than a failure, and it cost two such runs while reviewing #2556..config/nextest.tomladds aslow-timeoutwithterminate-after, so the same test is reportedTIMEOUTand killed in bounded time: SLOW at 30s, killed at 60s, doubled on CI.just rs testnow runs nextest, matching whatjust rs cialready did; doctests get their own recipe rather than being silently dropped, since nextest does not run them.A timeout is only meaningful if the honest tests are fast, and the moq-token RSA tests were not: six 2048-bit keygens across two tests, ~16s, because
rsaand its bignum backend are orders of magnitude slower unoptimized. Rather than shrinking the key size (which weakens the test and diverges from production), give those three dependencies anopt-leveloverride in the dev profile, which test builds inherit.With nothing legitimately near the limit any more, the threshold is real signal: a test flagged SLOW is now a bug to fix, not a number to raise, and
rs/CLAUDE.mdsays so along with theopt-levelescape hatch for a slow dependency.Public API changes
None. Build profile, test runner config,
justrecipes, and a guide.Test plan
cargo nextest run --workspace --all-targets --all-features: 2441 passed, 0 slow.std::future::pending()is killed at exactly 120s under the old setting and reportedTIMEOUTinstead of hanging the run.cargo fmt --check,taplo format --check(which covers the new.config/nextest.toml),cargo sort --check, and clippy-D warningsall clean.Split out of #2556, which is where the hangs turned up.
(Written by Opus 5)