Use cargo-nextest for Rust tests#68
Conversation
697f371 to
0a71aa2
Compare
e352b6a to
1edf031
Compare
Signed-off-by: lucarlig <luca.carlig@ibm.com>
1edf031 to
a384b40
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Code Review
Overview
This PR migrates Rust tests from cargo test to cargo-nextest across all plugins, adds a mutation-testing CI job using cargo-mutants, and introduces .config/nextest.toml and .cargo/mutants.toml configs. The implementation is thorough and the validation section in the PR description is detailed. CI jobs are still running (most checks are pending), so the below focuses on code-level issues.
Issues Requiring Changes
1. cargo install from source is slow — use a binary installer
In every CI job (build-test matrix × 15 jobs, coverage, mutation-testing), cargo-nextest is compiled from source:
cargo install cargo-nextest --version 0.9.133 --lockedThis adds significant compile time per job (nextest is a large binary). The nextest team provides pre-built binaries and an official install action. Using cargo-binstall is the conventional fast path:
- name: Install cargo-nextest
run: |
curl -LsSf https://get.nexte.st/0.9.133/linux | tar zxf - -C ${CARGO_HOME:-~/.cargo}/bin
cargo nextest --versionOr via the official action:
- uses: taiki-e/install-action@v2
with:
tool: cargo-nextest@0.9.133Same concern applies to cargo-mutants in the mutation-testing job.
2. Python::initialize() added to a Rust test needs justification
In plugins/rust/python-package/rate_limiter/src/plugin.rs:
Python::initialize();
Python::attach(|py| -> PyResult<()> {Adding Python::initialize() inside a test function is unusual and not present in other plugins. If nextest is running tests in separate processes (which is its default), this may be masking a test isolation issue rather than fixing an underlying problem. Please:
- Add a comment explaining why this is needed specifically for nextest
- Verify other plugins don't have the same issue (pii_filter, retry_with_backoff, etc. use similar PyO3 patterns but weren't updated)
3. has_mutation_cargo_packages uses fragile string comparison
if [[ "${mutation_cargo_packages}" == "[]" ]]; then
has_mutation_cargo_packages_output="false"This is a string equality check against the literal []. While it works when Python serializes an empty list, it's fragile (whitespace, ordering). More robust:
if python3 -c "import json, sys; sys.exit(0 if json.loads('${mutation_cargo_packages}') else 1)" 2>/dev/null; then
has_mutation_cargo_packages_output="true"
else
has_mutation_cargo_packages_output="false"
fiThis also parallels the existing pattern for has_plugins.
4. The Python validation one-liner has grown past readability
The inline Python one-liner in the validate-and-detect step has grown to ~600 characters, and this PR extends it further with mutation_cargo_packages and mutation_jobs validation. The mutation_jobs validation in particular uses a non-obvious generator-throw pattern:
[(_ for _ in ()).throw(AssertionError()) for job in mutation_jobs if not (...)]This is hard to audit for correctness. Consider extracting this to a small Python script in tools/ (similar to plugin_catalog.py). This is an existing debt, but extending a known problem further makes it the right time to address it.
Suggestions (Non-blocking)
-
No doctest guard: Since nextest skips Rust doctests by design, if a future contributor adds doctest blocks they will silently not run in CI. A minimal
cargo test --docstep inbuild-test(even just--no-runas a compile check) would catch this. A TODO comment or follow-up issue would also suffice. -
nextest-version pinning in
.config/nextest.toml:nextest-version = "0.9.133"enforces a minimum version, which is good. This aligns with the lockedcargo installversion. -
retry_with_backoff/Makefile: Unlike the other plugins,CARGO_PACKAGE :=is not added in this diff (onlyNEXTEST_PROFILE ?=). This is presumably because it was already defined earlier in that file — worth confirming the variable is set before thenextest run -p $(CARGO_PACKAGE)line to avoid a silent empty-pflag.
Use binary installs for cargo-nextest/cargo-mutants, extract CI selection validation into a maintainable Python script, and document why Python::initialize() is required in the rate_limiter test under nextest process isolation. Signed-off-by: lucarlig <luca.carlig@ibm.com>
|
@msureshkumar88 Thanks for the detailed review — I’ve pushed follow-up changes to address the requested items. Implemented:
Validation run locally:
If you want, I can also add a small follow-up for doctest guard ( |
Install cargo-nextest via the official nexte.st binary script on Linux and keep a cargo-install fallback for non-Linux matrix jobs so IBM org action allowlists are no longer required. Signed-off-by: lucarlig <luca.carlig@ibm.com>
Keep the IBM-allowed nextest installer path while restoring canonical step names and single-step install blocks so workflow-shape tests pass without reintroducing disallowed actions. Signed-off-by: lucarlig <luca.carlig@ibm.com>
Follow-up ReviewChecking whether all four required changes from the previous review are addressed, and whether new issues were introduced. Required Changes — Status✅ Issue 3:
|
Install cargo-nextest from official macOS binaries to avoid source builds on macOS runners while keeping the existing Windows fallback. Signed-off-by: lucarlig <luca.carlig@ibm.com>
🤖 Automated Verification Results for PR #68Verification Status: ✅ PASSED (6/6 checks) I've created and executed an automated verification script to confirm that this PR correctly implements cargo-nextest for Rust tests. Verification Summary
Key Findings✅ Configuration Files
✅ All Plugin Makefiles Updated
✅ CI Workflow Integration
✅ Additional Improvements
Verification ArtifactsThe following files have been created for verification:
Recommendation✅ APPROVE - All verification checks pass. The PR correctly implements cargo-nextest for Rust test execution and is ready for merge. This verification was performed by an automated script that checks configuration files, Makefiles, CI workflows, and tooling setup. |
|
I pushed commit
This is now wired in:
Windows still uses the |
Follow-up: Issue 1 ResolvedCommit Verified in diff:
All four required changes from the original review are now fully resolved. Approving. |
msureshkumar88
left a comment
There was a problem hiding this comment.
All four required changes resolved:
- ✅ Issue 1: Binary installer — macOS now uses official
get.nexte.stbinaries (arm64 + x86_64); Windows fallback acceptable without taiki-e - ✅ Issue 2:
Python::initialize()comment added; other plugins verified - ✅ Issue 3:
has_mutation_cargo_packagesstring check replaced with proper Python truthiness - ✅ Issue 4: Inline validation one-liner extracted to
tools/validate_ci_selection.py
Approved.
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Summary
cargo llvm-cov nextest --no-reportwrapper for the Rust coverage phasecargo nextest run --benchesfor CI benchmark verification--in-diffCloses #66
Validation
Notes