Restore standard filter integration tests (#520) - #521
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:
SummaryRestore the Cargo-discovered
WalkthroughThe change adds a guard for orphaned integration-test module trees. It updates stdlib filter fixtures to use isolated filesystem access, closure-based configuration, improved diagnostics, and dedicated ChangesIntegration test maintenance
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (16 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideRestores and hardens the std_filter integration-test target by re-wiring it as a top-level Cargo test, tightening its command/network/filesystem capabilities, extracting some scenarios into more focused modules, and adding a guard that ensures tests/*/mod.rs module trees are actually wired to Cargo-discovered integration tests. Flow diagram for Cargo integration-test discovery and wiring guardflowchart TD
cargo[Cargo integration_test discovery]
tests_dir[tests directory]
wiring_tests[integration_test_wiring_tests]
std_filter_tests[std_filter_tests.rs]
std_filter_tree[tests/std_filter_tests/mod.rs tree]
cargo --> tests_dir
tests_dir --> std_filter_tests
tests_dir --> wiring_tests
wiring_tests --> std_filter_tree
wiring_tests -->|asserts| std_filter_tests
std_filter_tree -->|must have| std_filter_tests
File-Level Changes
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: 92c144b229
ℹ️ 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".
92c144b to
7463a7e
Compare
7463a7e to
7d7e5f8
Compare
7d7e5f8 to
fc16b72
Compare
fc16b72 to
d1d540a
Compare
7d88b4b to
01abf53
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
01abf53 to
812597d
Compare
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. src/stdlib/command/execution.rs Comment on lines +103 to +113 run_configured_command(
SHELL,
|cmd| {
cmd.args(SHELL_ARGS).arg(command);
},
ChildInvocation {
input,
context,
operation: CommandOperation::Shell,
},
)❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. test_support/src/dev_fast/sandbox/utilities.rs Comment on file //! Host-utility discovery for the hermetic dev-fast sandbox.
❌ New issue: String Heavy Function Arguments |
4c03002 to
3e8ae45
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Restore the Cargo-discovered target for the dormant standard filter suite, repair its compilation and behavioural drift, and isolate its filesystem fixtures behind capabilities. Remove the basic expanduser and fetch-cache cases duplicated by the stdlib BDD scenarios. Remove first-match, all-match, missing-command, and workspace fallback which cases duplicated by the BDD suite and stdlib_which_tests. Add a regression guard that rejects tests/*/mod.rs trees without a matching top-level Cargo integration-test source.
Require explicit path attributes to match an active source line instead of accepting quoted path text anywhere in an integration-test source. Cover a commented-out path attribute and module declaration so an orphaned module tree cannot be mistaken for a Cargo target.
`which_filter_skips_heavy_directories` built its environment with `fallible::stdlib_env_with_path`, which derives the resolver workspace from `StdlibConfig::from_current_dir()`. The fixture's `target/helper` therefore sat outside the searched workspace entirely, so the `not_found` assertion held whether or not the skip policy existed. Renaming the fixture directory to a name absent from `WORKSPACE_SKIP_DIRS` still passed, confirming the test asserted nothing about the behaviour it named. Build an explicit `StdlibConfig` rooted at the fixture workspace, matching the wiring already used by `which_resolver_honours_workspace_root_override` and `test_cwd_mode_resolution`, and pass the empty PATH through `with_path_override`. The test now fails when the fixture directory is not on the skip list and passes when it is.
Extract `assert_command_error_message` in the `stdlib::command::error` test module so `spawn_errors_include_source` and `timeout_errors_report_duration` share the `ErrorKind::InvalidOperation` and rendered-message assertions. Each test keeps its own `CommandFailure` construction and localized expected message, and both remain separate tests with unchanged coverage. The helper takes `expected` by reference rather than by value: at `-D warnings` `clippy::needless_pass_by_value` rejects the owned `String`, and consuming it in place pushed the module past Whitaker's 400-line `module-max-lines` cap. Also add `only_unwired_module_trees_are_reported`, a proptest over generated module-tree names and every declaration form (active `mod`, active `#[path]`, commented variants of both, and absent). `orphaned_module_trees` asserts a universal invariant that the three fixed fixtures only sampled. The property test discriminates where they do not: dropping the `line.trim()` call leaves all three fixtures passing but fails the property test, because none of the fixtures indents a declaration.
The four `with_suffix` tests each opened with the same `fallible::filter_workspace()?` line. Extract it into an rstest `#[fixture]` returning `Result<Workspace>` and inject it, propagating setup failures with `?` at the use site. This matches the idiom already used by `network_functions.rs` in the same test binary, where `http_policy` is a fallible fixture consumed as `Result<NetworkPolicy>`. rstest invokes a non-`#[once]` fixture per test case, so each test still owns a distinct `TempDir`; running the four under a single process confirms four distinct workspace roots. Test names, assertions, and coverage are unchanged.
The split-build-dir paragraph read "the `#[once]` `test_support_rlib` fixture builds concurrently in the other test; sharing a target directory between the two races on the uplifted rlibs and fails", which left "races" and "fails" without a clear subject and implied a single peer test. The fixture is shared by two tests, `stub_env_default_does_not_compile` and `stub_env_builders_compile_under_the_same_harness`, so "the other test" was also inaccurate. Name all three participants and split the clause into two complete sentences, so the reader can see which test races which build.
`orphaned_module_trees` matched only a bare `mod <name>;`, so a tree declared `pub mod <name>;` or `pub(crate) mod <name>;` was reported as orphaned even though Cargo compiles it. Match all three forms alongside the existing `#[path = "<name>/mod.rs"]` attribute. Matching stays on whole trimmed lines, so indentation is still tolerated and a commented-out declaration still leaves the tree orphaned; a prefix or substring test would lose the latter. Extend the generated `Declaration` model with `PubConventional` and `PubCrateConventional`, both marked as wiring and rendered as valid Rust, so the property test exercises them. Add `visibility_qualified_declarations_wire_module_trees`, a case-based test pinning each active form as wiring and each commented form as orphaned. Reverting the matcher to the bare form fails exactly the `pub mod` and `pub(crate) mod` cases plus the property test, and leaves the commented cases passing.
df6ffe4 to
dd0fa39
Compare
|
@coderabbitai Have the following now been resolved? docs/developers-guide.md (1)
“race” and “fail”. Name the two tests and use a complete sentence. Proposed wording-`#[once]` `test_support_rlib` fixture builds concurrently in the other test;
-sharing a target directory between the two races on the uplifted rlibs and
-fails with version-skew errors (`E0460`).
+`#[once]` `test_support_rlib` fixture builds concurrently in the other test.
+Sharing a target directory makes the two tests race on the uplifted rlibs and
+causes version-skew errors (`E0460`).Triage: paragraph with 🤖 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' '== Current revision =='
git rev-parse --short HEAD
printf '%s\n' '== Reported documentation range =='
sed -n '1890,1940p' docs/developers-guide.md | nl -ba -v1890
printf '%s\n' '== Fixture references in the guide =='
rg -n -C 5 '\btest_support_rlib\b|target directory|uplifted rlibs|E0460' docs/developers-guide.md
printf '%s\n' '== Referenced test identifiers =='
rg -n -C 4 '\btest_support_rlib\b' . -g '*.rs' -g '*.md' -g '*.toml'Length of output: 921 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current revision =='
git rev-parse --short HEAD
printf '%s\n' '== Reported documentation range =='
sed -n '1890,1940p' docs/developers-guide.md
printf '%s\n' '== Fixture references in the guide =='
rg -n -C 5 '\btest_support_rlib\b|target directory|uplifted rlibs|E0460' docs/developers-guide.md || true
printf '%s\n' '== Referenced test identifiers =='
rg -n -C 4 '\btest_support_rlib\b' . -g '*.rs' -g '*.md' -g '*.toml' || trueLength of output: 12086
Mark this finding as resolved. At revision
The current text uses complete sentences. It states that the
No further change, AI-agent prompt, or follow-up issue is required. Triage: You are interacting with an AI system. |
|
@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
`hash_filter_legacy_algorithms_disabled` matched the raw rendered diagnostic
against "enable feature 'legacy-digests'". Fluent wraps string placeables in
U+2068 FSI and U+2069 PDI, so the rendered message reads
Unsupported hash algorithm '<FSI>sha1<PDI>' (enable feature
'<FSI>legacy-digests<PDI>').
and the plain substring test failed even though the diagnostic was correct.
Wrap the error in `normalize_fluent_isolates` before the `contains` check, as
the sibling assertions in this file already do for the unsupported-algorithm
message. The expected text and the error-kind assertion are unchanged, so the
check is no weaker: substituting a wrong feature name still fails it.
The test is gated `#[cfg(not(feature = "legacy-digests"))]`, so the
`--all-features` gates compile it out and never caught this. Validated under
default features, where the whole target passes 62/62, as well as under
`--all-features` at 65/65.
The existing guards read declaration text out of the sources, so they can only
police what the sources say. A manifest-level mistake leaves every source
untouched and slips past all of them.
Add `cargo_discovers_every_top_level_integration_test_source`, which runs
`cargo metadata --no-deps` through `env!("CARGO")` and asserts that the set of
discovered integration-test targets equals the set of top-level `tests/*.rs`
files, in both directions. A missing entry is a source Cargo never compiles; an
extra one is a target resolving outside `tests/`, which would invalidate the
sibling guards' path assumptions.
Restoring `autotests = false` with a partial `[[test]]` list fails the new test
and names all 65 undiscovered sources, so it discriminates on the case it
exists for. It costs about 20ms: `cargo metadata` neither builds nor uses the
network.
It does not catch #520 itself. A tree carrying its own `mod.rs` and no
top-level source contributes to neither set, leaving the equality intact;
`module_trees_are_wired_to_cargo_test_targets` still owns that direction, as a
faithful replay of the #520 layout confirms. The doc comment records the split
so neither guard is mistaken for covering the other.
Summary
std_filter_testsintegration-test binary andrepair nine months of compilation, localization, isolation, and behavioural
drift.
expanduser, fetch-cache, andwhichcases whose coverage isduplicated by
stdlib.featureorstdlib_which_tests.tests/*/mod.rstrees.Closes #520
Validation
make check-fmtmake markdownlintmake nixiemake test(1,370 nextest tests and all doctests passed)make typecheckmake lint(Rustdoc, Clippy, and Whitaker passed)mbake validate Makefilecargo nextest list --list-type binaries-onlyincludesnetsuke::std_filter_testsandnetsuke::integration_test_wiring_testscoderabbit review --agent(zero findings, before and after the stack-baserebase)
The Windows-only setup now uses injected
DefaultEnv.os_string("PATH"); theprevious
#[expect(clippy::disallowed_methods)]is no longer needed. A Linuxcross-target check was attempted, but
ringrequires the unavailable MSVClib.exebefore Netsuke's Windows test code can be checked on this host.References
Stack created with GitHub Stacks CLI • Give Feedback 💬