Make edge-harvest consistency test deterministic#169
Draft
leynos wants to merge 1 commit into
Draft
Conversation
`build_with_edges_has_consistent_count` compared two same-seed parallel builds and asserted their harvested edge counts fell within a tolerance. Insertion order on Rayon's shared global pool is perturbed by whatever else the process is running, so the variance grows with concurrently executing tests. Under `cargo nextest` (one process per test) the pool is uncontended and the test passed; under plain `cargo test`, as run by cargo-mutants for its unmutated baseline, the sibling `rstest` case alone was enough to push the spread past the tolerance (observed 24 vs 10 against a tolerance of 5, failing roughly two runs in ten), blocking every scheduled mutation run. Run each build inside its own single-threaded Rayon pool instead. Insertions then proceed in node order on one worker with a deterministic per-worker RNG, so two same-seed builds must produce identical harvests. This replaces the tolerance heuristic (including its coverage-instrumentation widening) with an exact equality assertion that is independent of test-runner process model, scheduler behaviour, and machine load. The parallel construction path remains exercised by `build_with_edges_returns_valid_edges`, whose assertions are scheduling-independent invariants. Verified with three consecutive green `cargo test --all-features --workspace` runs (the cargo-mutants baseline configuration) and a green `make test` (nextest) run. Closes #155.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #155.
The first scheduled mutation-testing run failed its unmutated baseline
because
hnsw::tests::edge_harvest::build_with_edges_has_consistent_count::case_1passes under CI's
cargo nextest(one process per test) but fails underthe plain
cargo testthat cargo-mutants uses. This PR removes thetest's dependence on process isolation so the baseline is stable.
Diagnosis
The test built the same seeded index twice on Rayon's shared global
pool and asserted the two harvested edge counts fell within a ±50%
tolerance. Insertion order — and therefore the HNSW graph structure and
the edges harvested — depends on Rayon scheduling, and the variance
grows with contention on the pool from concurrently running tests.
Under nextest each test owns its process, so the pool is uncontended
and the spread stays small. Under plain
cargo test, tests share oneprocess; even the sibling
rstestcase alone was enough to breach thetolerance:
runs, e.g.
edge counts should be similar: 24 vs 10 (tolerance: 5).So the failure is contention-dependent rather than seed- or
RNG-related, and the full 527-test binary that cargo-mutants runs makes
it far more likely still.
Fix
chutoro-core/src/hnsw/tests/edge_harvest/mod.rs— each build now runs inside its own single-threaded Rayon pool
(
ThreadPoolBuilder::num_threads(1)+pool.install). Insertions thenproceed in node order on one worker with a deterministic per-worker
RNG, so two same-seed builds must produce identical harvests. The
tolerance heuristic (including its coverage-instrumentation widening)
is replaced with exact equality of both the edge count and the full
EdgeHarvest, which is strictly stronger and independent of the testrunner's process model, scheduler behaviour, and machine load.
The genuinely parallel construction path remains exercised by
build_with_edges_returns_valid_edges, whose assertions arescheduling-independent invariants, and by the coverage tests in
chutoro-core/src/hnsw/tests/edge_harvest/coverage.rs.No caller-configuration change is needed: the workflow's
extra-args: "--all-features --test-workspace=true"is correct; onlythe test was at fault.
Validation
cargo test --all-features --workspace(the cargo-mutants baselineconfiguration): three consecutive runs, all green, no failures.
make test(nextest,--all-features): 993 passed, 1 skipped.make typecheck,make check-fmt,make markdownlint: green.make lint-clippy: green.make lint-whitakerwith the locally installed Whitaker suite failson
no_unwrap_or_else_panicinchutoro-core/src/hnsw/tests/property/graph_topology_tests/tests.rs:273,a file untouched by this diff and unchanged since Implement CPU HNSW candidate-edge harvest topologies, fixtures, and tests #71. This appears
to be a newer local suite than CI's pinned
whitaker-installer0.2.5; reported here rather than fixed, as it is pre-existing.