fix: honour a config rule's filterset instead of selecting every test - #54
fix: honour a config rule's filterset instead of selecting every test#54cargo-affected-bot wants to merge 6 commits into
Conversation
The `[*.metadata.affected]` rule path had only happy-path functional coverage. A typo'd filterset that nextest's parser rejects, or one that is valid but resolves to zero tests, would re-open the very coverage blind spot rules exist to close — exactly the silent-degradation shape CLAUDE.md's "fail loudly" principle is meant to prevent. Add two scenarios: - a bogus filterset (`&&`) must produce a non-zero exit with `filterset` in the error message - a valid filterset that resolves to zero tests must succeed but emit the no-tests warning naming the matched path Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cargo-affected-bot
left a comment
There was a problem hiding this comment.
The new config_rule_warns_when_filterset_matches_nothing test fails on linux + macos, and the failure looks like a true positive that exposes a real bug in nextest_list rather than a flake.
cargo nextest list --message-format json -E '<filter>' does not filter the testcases map — it includes every test with a filter-match field indicating whether each one matched. Probed locally with nextest 0.9.x against a two-test crate and -E 'test(=golden_matches)':
nextest-probe::probe golden_matches {'status': 'matches'}
nextest-probe::probe other_test {'status': 'mismatch', 'reason': 'expression'}
src/collect.rs ingests testcases without consulting filter-match.status, so the loop at lines 983–993 admits every listed test regardless of the filter. The only call site that passes a non-None filter is resolve_config_hits in src/config.rs, so the consequence is that [*.metadata.affected] rules effectively ignore their filterset — any rule whose glob matches a changed path force-selects every test in the workspace, not just the tests the filterset names. Existing tests in this file don't catch it because write_golden_project ships a one-test crate, so "all tests" and "the filterset's tests" happen to coincide.
This is consistent with what CI shows for the failing case: filterset test(=no_such_test_anywhere) against a one-test crate, the run reports 1 config and selects golden_matches (config), and the no-tests warning never fires.
The minimal fix is to skip mismatched cases inside the for (name, case) in cases loop in nextest_list — something like:
let filter_match_ok = case
.get("filter-match")
.and_then(|v| v.get("status"))
.and_then(|v| v.as_str())
.map_or(true, |s| s != "mismatch");
if !filter_match_ok {
continue;
}That keeps the no-filter call sites (run.rs, status.rs, the collect.rs self-call) byte-identical (status is "matches" when no -E is passed) and makes the config.rs path respect the filterset. The new config_rule_warns_when_filterset_matches_nothing test then becomes a regression pin for both the warning and the broader over-selection bug.
Since this widens the PR's scope from "tests only" to "tests + fix for the bug they exposed", flagging for maintainer call:
- Land the fix in this PR (test + fix are an atomic unit demonstrating the regression), or
- Split — open a separate fix PR and either drop the failing test from this one or land it gated on the fix.
Happy to push either shape.
Self-review, posted as a COMMENT.
`cargo nextest list -E <expr>` does not restrict the listing — it emits every testcase tagged with `filter-match`. `resolve_config_hits` read the listing as if `-E` had filtered it, so any `[*.metadata.affected]` rule whose globs matched a changed path force-selected the entire workspace instead of the tests its filterset names. Carry the filterset's rejections on `Listing` and subtract them. Keyed on `reason: "expression"` rather than on `status`, because nextest reports an `#[ignore]`d test as `mismatch`/`ignored` even with no `-E` at all.
|
Picking up the scope question from the review above, since no preference was recorded and the PR has been red since June: landing the fix here rather than splitting. The test and the fix are an atomic unit — Two things changed versus the review's proposal. The review suggested skipping mismatched cases on Second, the review's claim that the bug widens config-rule selection to the whole workspace needed a test that can actually see it — the existing Also merged |
nextest reports `ignored` as the mismatch reason in preference to `expression`, so an `#[ignore]`d test the filterset also rejects is tagged `mismatch`/`ignored` and never lands in `filterset_mismatched`. Resolving a rule as "listing minus filterset-rejected" therefore left every ignored test in, which kept the resolved set non-empty and silenced the typo'd-filterset warning in any workspace with one. Ignored tests also can't be the answer: `nextest run` skips them. Pins it by giving `config_rule_warns_when_filterset_matches_nothing` an ignored test; verified failing against the pre-fix expression.
cargo-affected-bot
left a comment
There was a problem hiding this comment.
Self-review, posted as a COMMENT.
This collides with #41, and not just textually. #41 (open since May, green, mergeable) rewrites the same for (name, case) in cases loop in nextest_list and the same Listing struct — it replaces ignored with an excluded set keyed on filter-match.status == "mismatch" across every reason, and rewires the three exclusion sites in selection.rs onto it. This PR instead keeps ignored and adds filterset_mismatched keyed on reason == "expression". With #41's excluded in place, resolve_config_hits reduces to tests - excluded and filterset_mismatched has nothing left to do; with this PR in place, #41 has to decide whether the new field survives its unification. Whichever merges second needs real rework rather than a conflict resolution, so it's worth deciding which shape wins before either lands.
The absent-filter-match fallback is unreachable, and if it ever fired it would silently restore exactly the bug this PR fixes. require_nextest gates on MIN_NEXTEST_VERSION = "0.9.116", and nextest-metadata's RustTestCaseSummary declares pub filter_match: FilterMatch — non-Option, so always serialized. I checked it out at the cargo-nextest-0.9.116 tag and it's there, and the field has been in nextest-metadata/src/test_list.rs since Feb 2022. So let Some(fm) = … else { return false } can't be hit by any nextest this tool will accept. If it somehow were, every testcase would read as filterset-matched, every rule would force-select the whole workspace again, and nothing would say so. The line directly above it in the same loop hard-errors on a missing ignored flag, and CLAUDE.md's "prefer failing loudly over silently degrading / do not add fallback paths" points the same way. Inline suggestions below (the call site needs ? too).
write_two_test_golden_project is a near-verbatim copy of write_golden_project — ~48 lines differing only in the package name, one extra lib.rs fn, and the extra test file. config_rule_warns_when_filterset_matches_nothing, added in this same PR for the same need, takes the cheaper route: call write_golden_project(dir) and write the extra tests/ignored.rs on top of it. The narrowing test can do the same with a tests/unrelated.rs asserting on config_rule_sample::GREETING (it still gets a coverage row, and it still has no link to golden.txt), which drops the second helper entirely.
…lper - `filter_expression_rejected` returns `Result<bool>` and errors when a testcase has no `filter-match`, matching the `ignored` flag two lines up. The absent-field branch was unreachable at MIN_NEXTEST_VERSION and would have silently restored the whole-workspace over-selection if it ever fired. - `config_rule_filterset_narrows_to_named_tests` builds on `write_golden_project` plus an extra `tests/unrelated.rs`, dropping the ~48-line near-verbatim `write_two_test_golden_project` copy.
|
Both code points from the review are applied in 551e059; the #41 collision is a maintainer call and is the one thing left open. Loud failure on absent
On #41 — reading its diff, its Full suite green locally after the change: 112 unit + 44 functional. Verification notes
|
`cargo fmt --check` failed on 551e059 — rustfmt wants the two-line boolean inside `Ok(...)` rewrapped. Binding `status`/`reason` first keeps the expression readable and is stable under rustfmt.
Summary
[*.metadata.affected]rules ignore theirfiltersetentirely: any rule whose globs match a changed path force-selects every test in the workspace, not the tests the filterset names. This PR fixes that and pins it with functional coverage, alongside the two loud-failure scenarios it originally carried.The direction of the bug is safe (over-selection, never under-selection), but it discards the whole point of the tool on any diff that touches a configured input — a
**/*.snaprule turns every snapshot edit into a full-suite run — and it made the "filterset selected no tests" guard unreachable in any workspace with at least one test.Root cause
cargo nextest list --message-format json -E <expr>does not restricttestcases. It lists every test and tags each with afilter-matchobject. Probed against a three-test crate with nextest 0.9.140:nextest_listingestedtestcaseswithout consulting that tag, andresolve_config_hitsread the resultingListing::testsas "the tests the filterset selects".Fix
Listinggainsfilterset_mismatched— the testcases this listing's own-Erejected — andresolve_config_hitssubtracts it. Two deliberate choices:tests, not removed from it.testsis documented as the complete set;collect --diffprunes DB rows against it, so narrowing it under some arguments and not others would be a trap.reason == "expression", not onstatus == "mismatch". As the probe above shows, an#[ignore]d test is reported asmismatch/ignoredeven with no-Eat all, so keying onstatuswould silently pull ignored tests into the excluded set on every listing and break--diff's prune. Other reasons (string,default-filter) are left in too — they are not this filterset's verdict, and leaving them keeps any error on the over-selecting side.Tests
config_rule_filterset_narrows_to_named_testsis the regression pin: a two-test crate with a rule naming only one of them. The existingwrite_golden_projectships a single test, so "the filterset's tests" and "every test in the workspace" coincide there — which is exactly why nothing caught this. Verified failing against the pre-fix expression (2 config, both tests selected) and passing after.The two loud-failure scenarios this PR started as are unchanged in intent:
config_rule_bogus_filterset_fails_loudly— an unparseable filterset (&&) must exit non-zero withfiltersetin the message. Passed before the fix too; it pins the error-propagation plumbing betweenconfig.rs,nextest_list, andmain.rs.config_rule_warns_when_filterset_matches_nothing— a valid filterset resolving to zero tests must warn and select nothing. This is the test that went red on linux + macos in CI and surfaced the bug; it now passes, with an addedselection=0/1assertion so it pins the selection and not just the warning text.Full suite green locally: 112 unit + 44 functional.
Scope note
The self-review on this PR flagged that a failing test had exposed a real bug and asked whether to land the fix here or split it out. Landing it here, since the test and the fix are an atomic unit — the test has no meaning without the fix, and the fix has no regression pin without the test. Happy to split if you'd rather review them apart.
Also folds in a two-line correction to
config.rs's module doc, which claimed editing a rule invalidates the coverage cache.fingerprint.rsstrips[*.metadata]before hashing, so rules are cache-neutral — asREADME.md,CLAUDE.md, andconfig_rule_selects_test_for_non_rust_input_change(which adds a rule aftercollectand reuses the cache) all say.Relationship to #41
Adjacent but disjoint. #41 keys new-test detection off
filter-match.statusfor the post---passthrough and reshapesListing::ignoredinto anexcludedset; it doesn't touchconfig.rs, so this bug survives it. Whichever lands first, the other rebases onto it — under #41's shape this fix collapses to reusing itsexcludedset.