Adopt cargo-nextest as the default test runner (#463) - #473
Conversation
|
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:
Summary
WalkthroughAdopt cargo-nextest for non-doctest tests, retain a separate doctest pass, pin CI installation, add contract coverage, document the workflow, and narrow ChangesCargo-nextest test adoption
Whitaker filesystem-lint exemptions
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Makefile
participant Nextest
participant CargoTest
Developer->>Makefile: make test
Makefile->>Nextest: run non-doctest tests
Nextest->>Nextest: serialize serial-env binaries
Makefile->>CargoTest: run doctests with --doc
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdopts cargo-nextest as the default non-doctest test runner, refactors Flow diagram for make test using cargo-nextest and doctest runnersflowchart TD
A[make_test] --> B[test-nextest]
A --> C[doctest]
B --> D[cargo_nextest_run_all_targets_all_features]
C --> E[cargo_test_doc_all_features]
D --> F[.config_nextest_toml_serial-env_group]
F --> D
subgraph Env
G[BUILD_JOBS]
H[NEXTEST_BUILD_JOBS]
end
G --> C
H --> D
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7ab32bdbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/developers-guide.md`:
- Around line 87-96: Update the Nextest installation guidance in
docs/developers-guide.md lines 87-96 and AGENTS.md lines 247-250 to use the
version pinned by NEXTEST_VERSION in .github/workflows/ci.yml for both cargo
install and cargo binstall examples, preserving the claim that local runs match
CI.
- Around line 125-128: Update the lint guidance in the developers guide to
remove the in-source #[allow(no_std_fs_operations)] exemption path. Direct
permanent exceptions to narrowly scoped configuration and mention item-level
#[expect(...)] only for permitted temporary cases, while preserving the
preference for migrating to cap_std.
In `@docs/whitaker-users-guide.md`:
- Around line 646-648: Remove the tip’s guidance to use
#[allow(no_std_fs_operations)] and replace it with the repository-approved
exemption mechanism, preserving the single-site exemption context without
recommending forbidden lint attributes.
In `@tests/makefile_test_target.rs`:
- Around line 111-124: The test-nextest assertions in the makefile test should
validate the complete runner configuration rather than isolated substrings.
Extend the checks around target_recipe and the related assertions near the
additional range to require --all-targets and verify that the override
containing all three binary filters assigns test-group = 'serial-env',
preserving the existing runner, feature, and warning requirements.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ac5de7a5-594a-4cd5-9e8a-27fdf23f0aed
📒 Files selected for processing (10)
.config/nextest.toml.github/workflows/ci.ymlAGENTS.mdMakefiledocs/developers-guide.mddocs/snapshot-testing-in-netsuke-using-insta.mddocs/whitaker-users-guide.mddylint.tomltests/makefile_test_target.rstests/workflow_ci.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/shared-actions(auto-detected)
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/developers-guide.md`:
- Around line 518-523: Update the documentation sentence describing serial
execution to refer to “the #[serial] markers on tests in the three binaries”
rather than implying the markers are applied to binaries, while preserving the
surrounding explanation and references.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 87da3732-0404-46e1-95a6-591acab8b4c3
📒 Files selected for processing (10)
.config/nextest.toml.github/workflows/ci.ymlAGENTS.mdMakefiledocs/developers-guide.mddocs/snapshot-testing-in-netsuke-using-insta.mddocs/whitaker-users-guide.mddylint.tomltests/makefile_test_target.rstests/workflow_ci.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/shared-actions(auto-detected)
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)
492-493: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the malformed failure sentence.
Replace “Either pass failing fails
make test” with “If either pass fails,make testfails.”Triage:
[type:grammar]As per path instructions, grammatical findings require a
Triage:paragraph with the grammar tag.🤖 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 `@docs/developers-guide.md` around lines 492 - 493, Correct the malformed sentence in the developer guide to state that if either pass fails, make test fails, and retain the required Triage paragraph with the grammar tag.Source: Path instructions
🤖 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.
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 492-493: Correct the malformed sentence in the developer guide to
state that if either pass fails, make test fails, and retain the required Triage
paragraph with the grammar tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2d3f4e8b-6869-4d35-80ef-e47630c9fc2b
📒 Files selected for processing (1)
docs/developers-guide.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/shared-actions(auto-detected)
Route `make test` through `cargo nextest run` and add a companion `doctest` pass, so `make test` remains the single canonical entry point while gaining per-test process isolation. `cargo test --all-targets` never ran doctests, so documentation examples were previously ungated; the new `doctest` target restores that coverage. Add `.config/nextest.toml` with a single narrow `serial-env` test group covering the three environment-mutating integration binaries. No blanket retries; a conservative slow-timeout surfaces hangs without failing legitimately slow suites. nextest spells build parallelism `--build-jobs` and reserves `-j` for test concurrency, so `NEXTEST_BUILD_JOBS` is kept separate from `BUILD_JOBS` to avoid silently reinterpreting a `-j` value.
Install cargo-nextest unconditionally in the `build-test` job, before the `Test` step, so every matrix leg can run `make test`. `NEXTEST_VERSION` is the single source of truth for the pin. Extend the CI contract tests with the nextest install wiring and add a Makefile contract test covering the `test` composite, the nextest and doctest recipes, and the `serial-env` group in `.config/nextest.toml`. The install-action reference is asserted by shape (a 40-hex commit SHA) rather than a literal commit. Asserting the exact SHA would make every Dependabot bump fail until a human edited the constant, which is the lockstep problem "Workflow pins and Dependabot" in the developers' guide already rules out for shared-actions callers. Coverage keeps its own llvm-cov runner and the mutation workflow keeps `--all-features` parity, so neither changes implicitly.
AGENTS.md (and CRUSH.md, a symlink to it) claimed `make test` was `cargo test --workspace`; replace that with the actual nextest and doctest passes, and name the runner, its configuration file, and the local install command in the Testing section. Add a "Test execution" section to the developers' guide covering the two passes, the `.config/nextest.toml` scope, the `serial-env` group and why it is narrow, the no-blanket-retry and timeout policy, how the group relates to the retained `#[serial]` markers and the EnvLock/CwdGuard utilities, and why the coverage and mutation runners are unchanged. Update the behavioural testing strategy to describe nextest's process-per-test model as reinforcing the per-scenario isolation policy, and point the snapshot guide at the nextest-aware `cargo insta` flags.
Read the Makefile and nextest configuration through a capability-scoped `cap_std::fs_utf8::Dir` rooted at the repository, with `camino` paths, instead of `std::fs` and `std::path`. The contract test now needs no `no_std_fs_operations` exclusion at all. Re-import the Whitaker user's guide, which documents the new `excluded_paths` option and the fact that the lint now honours in-source `#[allow]`. Correct the developers' guide, which still claimed the lint ignores lint-level attributes. Rewrite dylint.toml around `excluded_paths`. The application crate was exempt wholesale; only `netsuke::stdlib::which::lookup` and `netsuke::runner::process::file_io` actually need ambient access, so the rest of `netsuke` is now under the capability policy. The behavioural step definitions, CLI integration tests, and the shared workflow-reading helper are scoped the same way, which retires the crate-level entries for `bdd_tests`, `cli_tests`, `workflow_ci`, and `workflow_release`. `dependabot_config_tests` and `logging_stderr_tests` no longer trip the lint at all; their entries are removed. Crate-level entries are kept only where the ambient I/O lives in the crate root, where a path entry would be no narrower.
Point the local install guidance at the CI pin. The docs claimed local runs matched CI while installing whatever version was current; they now read `NEXTEST_VERSION` out of `.github/workflows/ci.yml` rather than copying the number, so the two cannot drift. Stop recommending `#[allow(no_std_fs_operations)]`. This repository denies `clippy::allow_attributes`, so that attribute will not compile here; permanent exceptions belong in `dylint.toml` and an in-source exemption must be a temporary `#[expect(..., reason = "…")]`. The vendored Whitaker guide carries the same correction, flagged as a local deviation so a future re-import does not silently drop it. Assert `--all-targets` in the nextest recipe, and parse `.config/nextest.toml` with the `toml` crate instead of matching substrings: the test now checks that `serial-env` serializes its members and that one override actually binds all three binaries to that group. Verified it fails when the binding is broken.
`#[serial]` annotates test functions; the guide implied it was applied to the binaries themselves.
`make markdownlint` is not one of the CI steps, so this MD012 violation reached main unnoticed. It is not in a region this branch authored, but the branch's gate run trips over it, so fix it here.
e117313 to
93e6b25
Compare
The commands passed `--version "$NEXTEST_VERSION"` but nothing in the file defined that variable; it deferred to the developers' guide for the extraction. A reader following AGENTS.md alone would have run `--version ""`. Inline the same read-the-pin-from-the-workflow form the developers' guide uses, so both files obtain the version from CI's `NEXTEST_VERSION` and pass it explicitly to both install commands.
|
@coderabbitai Have the following now been resolved? docs/developers-guide.md (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Relevant sentence matches ---'
rg -n -C 3 -e 'Either pass failing fails' -e 'If either pass fails' docs/developers-guide.md || true
printf '%s\n' '--- Lines 480-505 ---'
nl -ba docs/developers-guide.md | sed -n '480,505p'Length of output: 1019
The line-display utility was unavailable in the sandbox, but You are interacting with an AI system. |
`main` adopted cargo-nextest as the default runner in `#463`/`#473`, splitting `make test` into a `test-nextest` pass and a separate `doctest` pass. That left this branch stating, as a composition rule, that nextest "is not used in this repository" — now false — and left `make dev-test` on `cargo test` while the gate it stands in for had moved to a runner with different process isolation. Point `dev-test` at `cargo nextest run --all-targets --all-features`, so the accelerated loop and the gate exercise the same runner under the same `.config/nextest.toml`, including its `serial-env` group. The acceleration is unaffected: `RUSTUP_TOOLCHAIN` and `cargo --config` are Cargo-level rather than runner-level, so they compose with nextest unchanged. Verified end to end — 1188 tests pass under Cranelift and mold. Use `NEXTEST_BUILD_JOBS` rather than `BUILD_JOBS`, for the reason main records when it introduced the split: nextest reserves `-j` for test concurrency, so a Cargo-shaped `-j` would silently become a thread count. `dev-test` deliberately omits the doctest pass, `cargo test --doc` being a separate and comparatively quick runner. The guide says so rather than leaving the omission to be discovered, and continues to direct developers at `make test` before proposing a change. Update the Make-target test to assert the new command line, so the contract stays checked rather than assumed.
Closes #463.
make testnow runs the whole non-doctest Rust suite through cargo-nextest and gates doctests in a companion pass.make testremains the single canonical entry point for both local development and CI.What changed
Runner
make testis now a composite of two targets, mirroring the existinglint: lint-clippy lint-whitakerpattern:make test-nextest—RUSTFLAGS="-D warnings" cargo nextest run --all-targets --all-featuresmake doctest—RUSTFLAGS="-D warnings" cargo test --doc --all-featurescargo test --all-targetsdid not run them either, so documentation examples were not gated at all before this change. They are now, and all 44 pass.-j; nextest reserves-jfor test concurrency and spells build parallelism--build-jobs. The Makefile therefore keepsBUILD_JOBSandNEXTEST_BUILD_JOBSas separate variables rather than silently reinterpreting one as the other.Configuration
.config/nextest.tomlis deliberately small:serial-envtest group (max-threads = 1) scoped by filterset to exactly three binaries —manifest_env_tests,ninja_env_tests,env_path_tests— which mutatePATH,NINJA_ENV, and ad hocNETSUKE_*variables. Everything else stays fully parallel.One thing worth flagging honestly, and which the config comment and the developers' guide both state rather than obscure: because nextest runs every test in its own process, the
serial-envgroup is not load-bearing for the tests that exist today — process isolation already prevents env and cwd interference. It is retained because it states the serialization contract once so both runners agree, and so the constraint is not silently lost if a future test in those binaries reaches for genuinely shared state such as a fixed on-disk path. The#[serial]markers and theserial_testdependency are untouched, since the coverage workflow still drives an in-process runner where they are load-bearing.CI
NEXTEST_VERSION(0.9.133) is a job-level env var inbuild-test, the single source of truth for the pin.taiki-e/install-actionruns unconditionally afterSetup Rustand beforeTest, because every matrix leg runsmake test.coverage-main.ymland the coverage step inci.ymldelegate to thegenerate-coverageshared action (its owncargo llvm-covrun, nevermake test), andmutation-testing.ymlkeeps--all-featuresparity with the newmake test.Contract tests
tests/workflow_ci.rsgains coverage of the nextest install wiring:NEXTEST_VERSIONis an exactmajor.minor.patchpin, the installer resolves its tool spec from it, the step carries noif:guard, and it is ordered before theTeststep, which still runsmake test.tests/makefile_test_target.rsis new and pins the Makefile contract itself: thetestcomposite, both recipes, warnings denied on each, and theserial-envgroup in.config/nextest.toml.One deliberate deviation from the plan, called out for review: the plan asked the CI contract test to assert the literal
taiki-e/install-actionSHA. It asserts the shape instead — a 40-character lowercase-hex commit SHA. Asserting the literal value would make every Dependabot bump fail the test until a human edited the constant, which is exactly the lockstep problem "Workflow pins and Dependabot" indocs/developers-guide.mdalready rules out for shared-actions callers. The pin is still enforced; only the manual-edit treadmill is avoided. Happy to switch to a literal assertion if you would rather have the tighter pin.Documentation
AGENTS.md(andCRUSH.md, which is a symlink to it, not a byte-identical copy) claimedmake testwascargo test --workspace. Corrected, with the runner, its config file, and the local install command named in the Testing section.docs/developers-guide.mdgains a## Test executionsection covering both passes, the config's scope, theserial-envgroup, the retry and timeout policy, how it relates to the retained#[serial]markers and theEnvLock/CwdGuardutilities, and why the coverage and mutation runners are untouched. Local install guidance is added under## Quality gates.cargo test --test bdd_testsbecomescargo nextest run --test bdd_tests.cargo insta test --test-runner nextestflow.Capability-scoped filesystem access
Review feedback: the contract test read repository files through
std::fs/std::path, bypassing the repository's capability-oriented filesystem policy. It now uses acap_std::fs_utf8::Diropened at the repository root, withcaminopaths, so it cannot reach outside the checkout — and consequently needs nono_std_fs_operationsexclusion at all.Whitaker has since gained an
excluded_pathsoption that scopes an exemption to a module and its descendants, and itsno_std_fs_operationslint now honours in-source#[allow]. The user's guide is re-imported anddylint.tomlrewritten around it:netsukeapplication crate was exempt wholesale. Onlynetsuke::stdlib::which::lookup(PATH probing and cross-directory symlink canonicalization, whichcap_stdcannot express) andnetsuke::runner::process::file_io(temporary-file sync) actually need ambient access. The rest of the application crate is now under the capability policy.bdd_tests,cli_tests,workflow_ci, andworkflow_release.dependabot_config_testsandlogging_stderr_testsno longer trip the lint at all; their entries are removed as stale.The developers' guide also had a stale claim that the lint ignores in-source
allow/expectattributes; corrected.Verification
All gates green:
make check-fmt— passmake lint(Clippy + Whitaker Dylint) — passmake test— 1153 nextest tests passed, 0 skipped; 44 doctests passed, 20 ignoredmake markdownlint(including spelling) — passmake nixie— passmake test-workflow-contracts— 6 passedcoderabbit review --agentreturned 0 findings.References
🤖 Generated with Claude Code
Summary by Sourcery
Adopt cargo-nextest as the default non-doctest test runner while keeping doctests in a separate pass, and formalize this contract across CI, Makefile targets, configuration, tests, and documentation.
New Features:
Enhancements:
CI:
Documentation:
Tests:
Chores: