diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fbf9c5339..722db2fca 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,7 +21,7 @@ jobs: # not shadow .cargo/config.toml and strip -Zpolonius=next; tools such as # cargo-llvm-cov append their own flags to this value. RUSTFLAGS: -D warnings -Zpolonius=next - WHITAKER_INSTALLER_VERSION: '0.2.6' + WHITAKER_INSTALLER_VERSION: '0.2.7' # Single source of truth for the cargo-nextest pin. `make test` runs the # non-doctest suite through nextest, so the job installs it up front. NEXTEST_VERSION: '0.9.133' diff --git a/Cargo.toml b/Cargo.toml index fea0f998a..87cc418b3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -180,7 +180,10 @@ rustix = { version = "1.0.8", features = ["fs"] } [workspace] # `test_support` is a path dependency that Cargo would otherwise auto-include as a -# member. It is excluded here to keep its gate coverage unchanged in this focused -# change; folding it into the workspace is tracked separately as it requires -# clearing its own lint backlog. +# member. It stays excluded so its gate coverage is decided deliberately rather +# than inherited; folding it into the workspace is tracked separately. Because +# the exclusion also puts the crate out of reach of workspace-root cargo +# invocations, `make lint-whitaker` runs the Whitaker suite a second time +# against `test_support/dylint.toml`, and `make test-nextest` and `make doctest` +# each run a second time against `$(TEST_SUPPORT_MANIFEST)`. exclude = ["test_support"] diff --git a/Makefile b/Makefile index c56203d07..9ab0348be 100644 --- a/Makefile +++ b/Makefile @@ -66,6 +66,9 @@ RUSTDOC_FLAGS ?= --cfg docsrs -D warnings VERUS_FLAGS ?= VERUS_INSTALL_FLAGS ?= WHITAKER ?= whitaker +# `test_support` is excluded from the root workspace, so root-level cargo +# invocations cannot reach it; the test and lint recipes target it explicitly. +TEST_SUPPORT_MANIFEST ?= test_support/Cargo.toml export PATH := $(HOME)/.cargo/bin:$(HOME)/.local/bin:$(HOME)/.bun/bin:$(PATH) @@ -81,9 +84,13 @@ test: test-nextest doctest ## Run every Rust test with warnings treated as error test-nextest: ## Run all non-doctest Rust tests through cargo-nextest RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(CARGO) nextest run --all-targets --all-features $(NEXTEST_BUILD_JOBS) + # `test_support` is excluded from the root workspace, so the run above cannot + # reach its own tests. Run them separately, as lint-whitaker does. + RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(CARGO) nextest run --all-targets --all-features --manifest-path "$(TEST_SUPPORT_MANIFEST)" $(NEXTEST_BUILD_JOBS) doctest: ## Run doctests, which cargo-nextest cannot execute RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(CARGO) test --doc --all-features $(BUILD_JOBS) + RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(CARGO) test --doc --all-features --manifest-path "$(TEST_SUPPORT_MANIFEST)" $(BUILD_JOBS) test-workflow-contracts: ## Validate the mutation-testing caller contract uv run --with 'pytest>=8' --with 'pyyaml>=6' pytest tests/workflow_contracts -q @@ -98,9 +105,15 @@ lint: lint-clippy lint-whitaker ## Run Clippy and the Whitaker Dylint suite with lint-clippy: ## Run rustdoc and Clippy with warnings denied RUSTDOCFLAGS="$(RUSTDOC_FLAGS)" RUSTFLAGS="$${RUSTFLAGS-} $(POLONIUS_FLAGS)" $(CARGO) doc --no-deps RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(CARGO) clippy $(CLIPPY_FLAGS) + # `test_support` is excluded from the root workspace, so the run above cannot + # reach it. Lint it separately, as lint-whitaker does. + RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(CARGO) clippy --manifest-path "$(TEST_SUPPORT_MANIFEST)" $(CLIPPY_FLAGS) lint-whitaker: ## Run the Whitaker Dylint suite with warnings denied RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(WHITAKER) --all -- --all-targets --all-features + # `test_support` is excluded from the root workspace, so the run above cannot + # reach it. Lint it separately against its own dylint.toml. + cd test_support && RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(WHITAKER) --all -- --all-targets --all-features fmt: ## Format Rust and Markdown sources $(CARGO) fmt --all @@ -183,6 +196,9 @@ dev-build: dev-fast-check ## Build the debug binary with Cranelift and mold dev-test: dev-fast-check ## Run the nextest pass with Cranelift and mold RUSTUP_TOOLCHAIN=$(DEV_FAST_TOOLCHAIN) $(CARGO) --config "$$DEV_FAST_CONFIG" nextest run --all-targets --all-features $(NEXTEST_BUILD_JOBS) + # Mirrors test-nextest: `test_support` is excluded from the root workspace, + # so the run above cannot reach its tests. + RUSTUP_TOOLCHAIN=$(DEV_FAST_TOOLCHAIN) $(CARGO) --config "$$DEV_FAST_CONFIG" nextest run --all-targets --all-features --manifest-path "$(TEST_SUPPORT_MANIFEST)" $(NEXTEST_BUILD_JOBS) bench-build: dev-fast-check ## Time clean and incremental debug builds for both paths @CARGO="$(CARGO)" scripts/bench-build.sh diff --git a/docs/developers-guide.md b/docs/developers-guide.md index b869faeb9..39507b852 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -138,33 +138,110 @@ configuration does and does not cover. (`whitaker --all -- --all-targets --all-features`). Install Whitaker through the standalone installer described in the [Whitaker user's guide](whitaker-users-guide.md) so local linting matches -continuous integration (CI); `make lint-clippy` runs the Clippy-only subset. +continuous integration (CI); `make lint-clippy` runs the Clippy-only subset. CI +pins the installer version in `WHITAKER_INSTALLER_VERSION` in +`.github/workflows/ci.yml`. Install that same version locally so local runs +match CI; read the pin from the workflow rather than copying the number, so the +two cannot drift: + +```bash +WHITAKER_INSTALLER_VERSION="$(sed -n \ + "s/.*WHITAKER_INSTALLER_VERSION: '\(.*\)'.*/\1/p" \ + .github/workflows/ci.yml)" +cargo install --locked whitaker-installer \ + --version "$WHITAKER_INSTALLER_VERSION" +# or, for a prebuilt binary: +cargo binstall --no-confirm --locked \ + "whitaker-installer@$WHITAKER_INSTALLER_VERSION" +``` + +`whitaker-installer` and the lint libraries are separate artefacts with +separate versions. `WHITAKER_INSTALLER_VERSION` pins the installer — the tool +that stages libraries — and nothing else. The installer keeps its own checkout +of the Whitaker repository under `~/.local/share/whitaker`, updates it with +`git pull`, and stages the libraries from its default branch. Lint behaviour +therefore tracks Whitaker HEAD. + +**Running the lint libraries at HEAD is deliberate.** Netsuke follows the suite +as it develops, so new lints and fixes arrive without a version bump here. Do +not add a `[workspace.metadata.dylint]` block pinning `whitaker_suite` to a +`tag` or `rev`. The [Whitaker user's guide](whitaker-users-guide.md) documents +that form, and it is the right answer for a project wanting reproducible lint +results, but adopting it here would reverse a standing decision rather than fix +a defect. + +The cost is worth stating plainly: a change upstream can alter lint results +between two runs with no change in this repository, and a local checkout that +has not been restaged will disagree with CI, which stages fresh on every job. +Restaging is what reconciles them. + +What the module-scoped exemptions in `dylint.toml` actually depend on is +[Whitaker PR #315][whitaker-pr-315], which added the `excluded_paths` option, +so the staged libraries must be recent enough to include it. Libraries staged +from an older checkout ignore `excluded_paths` silently — the exemptions stop +applying with no error, and the lint reports the modules they covered. Re-run +`whitaker-installer` to restage from HEAD. If that checkout has been left on a +detached HEAD, the install fails at its `git pull`; put it back on the default +branch and re-run. + +[whitaker-pr-315]: https://github.com/leynos/whitaker/pull/315 + Whitaker is configured by `dylint.toml` at the repository root, where each sanctioned ambient-filesystem scope for `no_std_fs_operations` carries a -documented rationale. +documented rationale. `docs/whitaker-users-guide.md` is a near-verbatim import +of the [upstream Whitaker user's guide][whitaker-upstream-guide]; refresh it +from that URL rather than editing it in place, preserving the "Netsuke +deviation from upstream" callout, and record Netsuke-specific policy here and in +`dylint.toml`. + +[whitaker-upstream-guide]: https://raw.githubusercontent.com/leynos/whitaker/refs/heads/main/docs/users-guide.md Prefer `excluded_paths` over `excluded_crates`: a path entry exempts one module and its descendants, whereas a crate entry exempts a whole compilation unit. The application crate is scoped this way — only `netsuke::stdlib::which::lookup` (executable discovery through `PATH` and cross-directory symlink canonicalization, which `cap_std` cannot express) and -`netsuke::runner::process::file_io` (temporary-file synchronization), and +`netsuke::runner::process::file_io::ambient_sync` (temporary-file +synchronization, scoped to the submodule holding only that `sync_all` so the +rest of `file_io` keeps writing through `cap_std` handles), and `netsuke::cli::discovery::paths` (canonicalizing an ambient `--directory` to match OrthoConfig's layer paths) are exempt; the rest of `netsuke` stays under the capability policy. The behavioural step definitions, CLI integration tests, and shared workflow-reading helper that stage fixtures ambiently are scoped the same way. A crate-level entry is justified only when the ambient access lives in the crate root itself, where a path entry would be no narrower — that covers -the Cargo build script, the `test_support` fixture crate, and the enumerated -integration-test crates. - -Permanent exceptions belong in `dylint.toml`, scoped as narrowly as the lint -allows. The lint does honour in-source lint attributes, but this repository -denies `clippy::allow_attributes`, so `#[allow(no_std_fs_operations)]` will not -compile here; an in-source exemption must be a *temporary*, item-level -`#[expect(no_std_fs_operations, reason = "…")]` that states the reason and the -route back to compliance. Prefer migrating to `cap_std` over any of these; -reach for an exclusion only when the operation is irreducibly ambient. +the Cargo build script and the enumerated integration-test crates. + +`test_support` is excluded from the root Cargo workspace, so the root +`dylint.toml` cannot reach it and `whitaker --all` at the repository root never +lints it. `make lint-whitaker` therefore runs the suite a second time from +`test_support/`, where `test_support/dylint.toml` supplies that crate's policy: +a single `excluded_paths` entry for `test_support::fs`, the module wrapping the +ambient fixture operations. Every other module routes through it — `exec`, +`manifest`, and the crate-root regression tests directly, and `check_ninja` and +`fake_ninja` via `exec::write_exec_with_content` — so a new direct `std::fs` +call anywhere else in the crate still fails the lint. + +Exceptions belong in `dylint.toml`, scoped as narrowly as the lint allows. +Neither `#[allow(no_std_fs_operations)]` nor +`#[expect(no_std_fs_operations, reason = "…")]` suppresses this lint in the +Whitaker build this repository pins, so no in-source attribute is usable here +(this repository also denies `clippy::allow_attributes`, so +`#[allow(no_std_fs_operations)]` will not even compile). A `dylint.toml` entry +is the only working mechanism: a narrowly scoped `excluded_paths` entry for a +bounded module, or, where the ambient access lives at the crate root and a path +entry would be no narrower, an `excluded_crates` entry. Prefer migrating to +`cap_std` over adding an exclusion; reach for an exclusion only when the +operation is irreducibly ambient. + +To confirm the exclusions have not silently widened, add a temporary +`std::fs::metadata` call to an unexcluded module — for example +`src/stdlib/which/cache.rs`, a sibling of the excluded `lookup` module, or the +body of `src/runner/process/file_io.rs` outside `ambient_sync` — then run +`make lint-whitaker`. Both sites must still be reported; revert the probe +afterwards. The same check applies to `test_support`: a `std::fs` call in, say, +`test_support/src/exec.rs` must be reported even though `test_support::fs` is +exempt. When command output is long, preserve exit codes and logs: @@ -530,15 +607,16 @@ and `-Clink-arg=-fuse-ld=mold`. Kani's own toolchain and the LLVM backend. The same applies to Verus. - **Test runner.** `make dev-test` is the accelerated counterpart of `make test-nextest`, not of `make test`: it runs the same - `cargo nextest run --all-targets --all-features`, and so is governed by the - same [`.config/nextest.toml`](#nextest-configuration), including the - `serial-env` group. It omits the `doctest` pass, because `cargo test --doc` - is a separate and comparatively quick runner; run `make test` before - proposing a change. The acceleration is applied through `RUSTUP_TOOLCHAIN` and - `cargo --config`, both Cargo-level rather than runner-level, which is why - they compose with nextest unchanged. Note the target uses - `NEXTEST_BUILD_JOBS`, not `BUILD_JOBS`: nextest reserves `-j` for test - concurrency, so a Cargo-shaped `-j` would silently become a thread count. + `cargo nextest run --all-targets --all-features`, over the root workspace and + then `test_support`, and so is governed by the same + [`.config/nextest.toml`](#nextest-configuration), including the `serial-env` + group. It omits the `doctest` pass, because `cargo test --doc` is a separate + and comparatively quick runner; run `make test` before proposing a change. + The acceleration is applied through `RUSTUP_TOOLCHAIN` and `cargo --config`, + both Cargo-level rather than runner-level, which is why they compose with + nextest unchanged. Note the target uses `NEXTEST_BUILD_JOBS`, not + `BUILD_JOBS`: nextest reserves `-j` for test concurrency, so a Cargo-shaped + `-j` would silently become a thread count. - **rust-analyzer.** No rust-analyzer configuration is committed, so the language server uses the repository toolchain and the default backend. Opting rust-analyzer into Cranelift is a personal, machine-local choice; it needs a @@ -629,14 +707,18 @@ The fixtures live in `test_support::dev_fast`: two starting points. `BuildScenario` is a sandbox where `make dev-fast-check` passes — pinned `mold` on the install prefix, a `rustup` reporting the Cranelift component, and a `RecordingCargo` installed — and is shared by the - Make-target and benchmark suites. `InstallerScenario` is a sandbox with a - published `FakeRelease` and a usable `rustup`, letting a test concentrate on - the linker half of the installer; the installer and checksum suites share it. - The module also exports `TEST_MOLD_VERSION`, deliberately not a real `mold` - version so a test that accidentally reaches the network fails rather than - silently succeeding against an upstream artefact, and `WRONG_SHA256`. - `InstallerFixture` groups the installer's pin path, checksum path, and - release URL, and renders them via `script_env()`. + Make-target and benchmark suites. `BuildScenario::run(target)` returns the + single Cargo invocation a target must produce; `run_all(target)` returns + every invocation in order, for targets that invoke Cargo more than once, such + as `dev-test`, which runs the root pass and then `test_support`. + `InstallerScenario` is a sandbox with a published `FakeRelease` and a usable + `rustup`, letting a test concentrate on the linker half of the installer; the + installer and checksum suites share it. The module also exports + `TEST_MOLD_VERSION`, deliberately not a real `mold` version so a test that + accidentally reaches the network fails rather than silently succeeding + against an upstream artefact, and `WRONG_SHA256`. `InstallerFixture` groups + the installer's pin path, checksum path, and release URL, and renders them via + `script_env()`. A scenario earns its place here once a second suite needs it, and not before; suite-specific conveniences stay with their suite — the installer tests keep @@ -672,10 +754,15 @@ Prefer a model that predicts an outcome over a table that restates one. Where an invariant lives in a shell script, the cost is a process per case, so keep the corpus small and the strategy structural. -A `#[cfg(test)]` unit test added inside `test_support` will not run as part of -`make test`, because `Cargo.toml` excludes `test_support` from the workspace. -Put assertions about the fixtures themselves in the `tests/dev_fast_*.rs` -integration crates instead, where the gate will actually exercise them. +`Cargo.toml` excludes `test_support` from the workspace, but `make test` still +exercises it: `test-nextest` and `doctest` each run a second time with +`--manifest-path "$(TEST_SUPPORT_MANIFEST)"` (see +[Test execution](#test-execution)), so a `#[cfg(test)]` unit test added inside +`test_support` is covered by `make test`. `lint-clippy` and `lint-whitaker` run +their passes against `test_support/Cargo.toml` the same way. Prefer putting +assertions about the fixtures themselves in the `tests/dev_fast_*.rs` +integration crates when the assertion belongs with the suite that consumes the +fixture, rather than with the fixture's own unit tests. ### Benchmark evidence @@ -864,13 +951,24 @@ Cargo home plus Kani support-file home. `RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)"` (the Makefile re-states the Polonius flag because a set `RUSTFLAGS` overrides `.cargo/config.toml`, and the `$${RUSTFLAGS:+$$RUSTFLAGS }` prefix preserves - any `RUSTFLAGS` inherited from the caller). This runs every unit, integration, - `rstest`, and `rstest-bdd` test. + any `RUSTFLAGS` inherited from the caller). `test_support` is excluded from + the root Cargo workspace, so this root-level invocation cannot reach its + tests; the target therefore runs `cargo nextest` a second time against + `test_support/Cargo.toml`, mirroring how `make lint-whitaker` runs the + Whitaker suite twice for the same reason (see above). Together the two + invocations run every unit, integration, `rstest`, and `rstest-bdd` test + across both the root workspace and `test_support`. - `make doctest` — `cargo test --doc --all-features`, with the same `RUSTFLAGS`. nextest cannot execute doctests, so they need their own pass. Note that the previous `cargo test --all-targets` invocation never ran doctests either; the separate target is what makes a broken documentation - example fail the gate. + example fail the gate. For the same reason as `test-nextest`, `doctest` also + runs a second time against `test_support/Cargo.toml` to cover its doctests. + +The `test_support/Cargo.toml` path used for the second pass comes from the +overridable `TEST_SUPPORT_MANIFEST` Makefile variable (default +`test_support/Cargo.toml`); point it elsewhere with, for example, +`make test TEST_SUPPORT_MANIFEST=path/to/Cargo.toml`. If either pass fails, `make test` fails. Run the individual targets when iterating, but treat `make test` as the gate. @@ -970,6 +1068,26 @@ writes that content and applies executable permissions only on Unix. `write_exec` is the minimal-script convenience wrapper; `write_exec_with_content` is the shared primitive for custom behaviour. +The helpers take `&Utf8Path` and return `Utf8PathBuf`, matching the camino +types used throughout Netsuke. Callers that already hold camino paths pass them +directly. `tempfile::TempDir::path()` still yields an OS-native `&Path`, so +callers convert at that boundary with `exec::utf8_path`, the single conversion +point. `utf8_path` returns a `Result` rather than panicking: it names the +offending path in the error (`path is not valid UTF-8: {path}`), and callers +propagate it with their own context, as `fake_ninja` and +`fake_ninja_check_build_file` do. + +```rust +let temp = TempDir::new()?; +let root = exec::utf8_path(temp.path()).context("temporary directory")?; +let stub = write_exec(root, "tool")?; +``` + +Because a camino path cannot represent a non-UTF-8 path, the fake-executable +factories now fail on a temporary directory whose path is not valid UTF-8, +rather than succeeding as they previously did. The `test_support` test +`fake_ninja_helpers_reject_non_utf8_temp_directories` pins this behaviour. + ### User-facing documentation examples Every fenced example in `README.md`, `docs/users-guide.md`, and @@ -1174,6 +1292,122 @@ pattern documented in the `no_color_env` is shared across output-preference and theme tests that exercise optional `NO_COLOR` lookup behaviour. +### `test_support::fs` + +`test_support::fs` (`test_support/src/fs.rs`) is the crate's single +ambient-filesystem boundary. Fixture code routes filesystem access through it +rather than reaching for `std::fs` directly; Whitaker enforces this (see +[Quality gates](#quality-gates)) for every other module in the crate. + +Most wrappers forward to their `std::fs` namesake unchanged. These are worth +calling out because their behaviour, platform support, or reason for existing +is not obvious from the name: + +- `is_dir(path) -> bool` mirrors `Path::is_dir`: it follows symlinks, and an + absent or unreadable path returns `false` rather than surfacing the + underlying metadata error. Fixture code must use this wrapper for directory + predicates rather than calling `std::fs::metadata(...).is_dir()` or + `Path::is_dir` directly. `test_support/src/manifest.rs` is an existing caller: + `ensure_manifest_exists` uses it both to reject a directory where a manifest + file is expected, and to accept a destination directory that is already + present. +- `is_executable_file(path) -> bool` (Unix only) is `true` when the path is a + regular file with any execute bit set, and `false` for an absent or + unreadable path. It is the inverse of `set_mode`, and exists for probing a + sandbox `PATH` the way an executable lookup would. +- `copy(from, to) -> io::Result` forwards to `std::fs::copy`, returning + the number of bytes copied and propagating its failure. The `dev_fast` + release fixtures use it to place a built archive under its versioned name. +- `modified(path) -> io::Result` returns the file's modification + time. It propagates both the metadata failure and the platform's failure to + report a timestamp, so it is `io::Result` rather than an `Option`. The + `dev_fast` staging fixtures use it to assert a file was or was not rebuilt. +- `write_with_mtime(path, contents, mtime) -> io::Result<()>` (Unix only) + creates or truncates `path`, writes `contents`, and sets the modification + time to `mtime`, propagating whichever step fails. The staging fixtures use + it to backdate a file so a later build sees it as stale. + +`write_with_mtime` is the reason `test_support/dylint.toml` carries no +`dev_fast` exemption. Backdating a fixture needs one open file for both the +write and the timestamp, which reads like an irreducibly ambient operation that +has to happen at the call site. Taking the timestamp as an argument keeps the +handle inside this module instead: the caller never sees a `File`, so the +ambient boundary stays where the lint expects it. Prefer that shape — pass in +what the operation needs and keep the handle here — over widening an exclusion +to a module that wants a raw `File`. + +### Shared Makefile contract helpers + +`tests/support/makefile.rs` is a shared module for integration tests that +assert facts about the repository's `Makefile` — for example, that a target +declares a given prerequisite or recipe. It provides five helpers: + +- `repo_root() -> Result` opens the repository root + through `cap_std::fs_utf8::Dir` and `ambient_authority()`, so a contract test + cannot read outside the checkout. +- `read_repo_file(relative: &Utf8Path) -> Result` reads a file under + the repository root via that capability-scoped directory. +- `parse_rule(line: &str) -> Option<(&str, Vec<&str>)>` parses a single + `target: prerequisites` line. It returns `None` for recipe or continuation + lines, comments, `.PHONY`-style directives, and variable assignments (`:=` is + caught by testing whether the text after the colon starts with `=`). It + strips trailing `##` help comments from the prerequisite list. +- `target_prerequisites(contents: &str, target: &str) -> Option>` + finds a target's rule line and returns its prerequisites. +- `target_recipe(contents: &str, target: &str) -> Option` returns + `Some("")` for a target with no recipe and `None` for an absent target. Blank + lines inside a recipe are traversed but dropped, so a recipe split by a blank + line is returned whole. + +Because every file under `tests/` compiles as an independent crate, there is no +library through which to share this module, and `tests/support/` is a +subdirectory that Cargo does not auto-discover as a test target. Consumers +include it with: + +```rust +#[path = "support/makefile.rs"] +mod makefile; +``` + +This mirrors the shape of `tests/common/mod.rs`, which the workflow-contract +crates include with `mod common;`. The module carries its own `#[cfg(test)]` +unit tests covering every helper, so a consumer that needs only part of the +surface does not trip `dead_code`; these tests run once per including crate. + +Scope and reuse policy: this module exists only for static Makefile contract +tests and capability-scoped reads from the repository root. It must not grow +into a general test-utility bag — fixture construction, process invocation, and +environment control belong in the `test_support` crate, which is versioned, +linted, and documented as such. A helper earns a place here only when more than +one contract test needs the same reading or parsing behaviour. Nothing in it +runs Make, runs Cargo, or writes anything. + +### `EnLocalizer` field ordering + +`EnLocalizer` (`test_support/src/localizer.rs`) holds both the localizer +override guard and the global localizer mutex guard: + +```rust +pub struct EnLocalizer { + _guard: LocalizerGuard, + _lock: MutexGuard<'static, ()>, +} +``` + +The declaration order is load-bearing: struct fields drop in declaration order, +so `LocalizerGuard` must be declared before the mutex guard. That keeps the +mutex held while `LocalizerGuard` restores the process-global localizer, so a +test waiting on the lock cannot acquire it, install its own override, and +capture this test's override as its "previous" state. + +`en_localizer()` recovers a poisoned `LOCALIZER_TEST_LOCK` with +`PoisonError::into_inner` rather than propagating the poison: a poisoned lock +only means an earlier test panicked while holding it, and `set_en_localizer` +re-establishes the global state unconditionally, so the recovered guard is +still safe to use. See the +[locale-pinned snapshot tests](snapshot-testing-in-netsuke-using-insta.md#locale-pinned-snapshot-tests) +section for the fixture's intended usage. + ### `EnvLock` `test_support::env_lock::EnvLock` is a global mutex that serializes all diff --git a/docs/whitaker-users-guide.md b/docs/whitaker-users-guide.md index 4c1f112d1..13db586be 100644 --- a/docs/whitaker-users-guide.md +++ b/docs/whitaker-users-guide.md @@ -113,6 +113,16 @@ libraries = [ ] ``` +> **Netsuke deviation from upstream — preserve when re-importing this guide.** +> Netsuke does not pin the lint libraries and carries no +> `[workspace.metadata.dylint]` block. It installs them at Whitaker HEAD +> through `whitaker-installer`, deliberately, so the suite's improvements +> arrive without a version bump. `WHITAKER_INSTALLER_VERSION` in +> `.github/workflows/ci.yml` pins the installer binary, which is a different +> artefact from the libraries this section pins — the installer's version says +> nothing about which lints get staged. Adopting the form above would reverse a +> standing decision; see "Quality gates" in `docs/developers-guide.md`. + ### Rolling release downloads Whitaker publishes a `rolling` pre-release tag that is continuously updated and @@ -648,10 +658,16 @@ policy. > lint-level attributes. > > **Netsuke deviation from upstream — preserve when re-importing this guide.** -> This repository denies `clippy::allow_attributes`, so -> `#[allow(no_std_fs_operations)]` will not compile here. Use a narrowly scoped -> `excluded_paths` entry in `dylint.toml` for a permanent exemption, or a -> temporary `#[expect(no_std_fs_operations, reason = "…")]` on the item. +> In the Whitaker build this repository pins, neither +> `#[allow(no_std_fs_operations)]` nor +> `#[expect(no_std_fs_operations, reason = "…")]` suppresses this lint, so the +> Tip above does not apply here; `#[expect(...)]` additionally fails the build +> with an unfulfilled-lint-expectation error under `-D warnings`. This +> repository also denies `clippy::allow_attributes`, so +> `#[allow(no_std_fs_operations)]` will not even compile. The only sanctioned +> mechanism is a `dylint.toml` entry: a narrowly scoped `excluded_paths` entry +> for a bounded module, or an `excluded_crates` entry where the ambient access +> lives at the crate root. **How to fix:** Replace `std::fs` with `cap_std`: diff --git a/dylint.toml b/dylint.toml index f0b565322..7d25b347d 100644 --- a/dylint.toml +++ b/dylint.toml @@ -13,11 +13,16 @@ excluded_paths = [ # Application boundary. `netsuke` probes executables in directories supplied # by the ambient PATH and follows cross-directory symlinks when - # canonicalising them; `cap_std` refuses symlinks that leave the directory, + # canonicalizing them; `cap_std` refuses symlinks that leave the directory, # so these operations cannot be capability-scoped. "netsuke::stdlib::which::lookup", # Syncs the ambient temporary file the runner writes its Ninja file to. - "netsuke::runner::process::file_io", + # `tempfile` creates that file in the ambient system temporary directory, so + # no directory handle covers it and the open file descriptor is the narrowest + # authority for `sync_all`. Scoped to the `ambient_sync` submodule that holds + # only that call, so the rest of `file_io` — which writes through `cap_std` + # handles — stays under the policy. + "netsuke::runner::process::file_io::ambient_sync", # Canonicalises a caller-supplied `--directory` so it can be compared with # the layer paths `ortho_config` records, which that crate canonicalises # through `std::fs`. The comparison has to mirror it exactly, and the @@ -52,21 +57,19 @@ excluded_paths = [ # - build_script_build: the Cargo build script writes generated man pages and # audit artefacts to ambient paths supplied by Cargo (`OUT_DIR`, `target/`), # where capability-based handles offer no benefit. -# - test_support: the shared test-fixture crate; fixture management uses -# ambient tempdir access by design (see test_support/src/fs.rs), matching -# the "test support utilities" exclusion in the Whitaker user's guide. The -# crate is currently excluded from the workspace, so Whitaker does not lint -# it; the entry is kept so folding it in does not silently reopen the -# question. # - integration test crates (tests/*.rs): fixture management writes manifests, # workspaces, and captured output with ambient tempdir access, which the # Whitaker user's guide lists as a sanctioned exclusion. # # Add a new entry here only when the ambient I/O really is at the crate root; # otherwise add a path above, and prefer migrating to `cap_std` over either. +# +# `test_support` is deliberately absent. It is excluded from the workspace, so +# this file cannot reach it; `make lint-whitaker` lints it separately against +# `test_support/dylint.toml`, which narrows its exemption to the single +# `test_support::fs` boundary module rather than exempting the whole crate. excluded_crates = [ "build_script_build", - "test_support", "advanced_usage_tests", "assert_cmd_tests", "manifest_glob_tests", diff --git a/src/runner/process/file_io.rs b/src/runner/process/file_io.rs index 56c965ee2..98b3c0687 100644 --- a/src/runner/process/file_io.rs +++ b/src/runner/process/file_io.rs @@ -33,14 +33,26 @@ pub fn create_temp_ninja_file(content: &NinjaContent) -> AnyResult io::Result<()> { - tmp.as_file().sync_all() +mod ambient_sync { + //! Ambient-authority durability boundary for the temporary Ninja file. + //! + //! `tempfile` places the file in the ambient system temporary directory, so + //! no `cap_std::fs::Dir` handle covers it and the already-open file + //! descriptor is the narrowest authority available for the sync. This module + //! is deliberately the only part of `file_io` outside the capability policy; + //! it is named in `dylint.toml` under `[no_std_fs_operations] + //! excluded_paths` so the rest of the module stays enforced. + + use super::{NamedTempFile, io}; + + /// Sync a temporary Ninja file to disk before handing its path to Ninja. + pub(super) fn sync_temp_ninja_file(tmp: &NamedTempFile) -> io::Result<()> { + tmp.as_file().sync_all() + } } +use ambient_sync::sync_temp_ninja_file; + pub fn write_text_file_utf8(dir: &cap_fs::Dir, path: &Utf8Path, content: &str) -> AnyResult<()> { if let Some(parent) = path.parent().filter(|p| !p.as_str().is_empty()) { dir.create_dir_all(parent.as_str()).with_context(|| { diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 37d593eec..3b63a7af5 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -47,6 +47,13 @@ type StatusObserver<'a> = &'a mut dyn FnMut(u32, u32, &str); // testing surface without exporting them in release builds. #[cfg(doctest)] pub mod doc { + //! Re-exports of otherwise-private `process` items for doctests only. + //! + //! Doctests compile as a separate crate and cannot reach `pub(crate)` or + //! private items in `process`, so this module surfaces the redaction + //! helpers and a handful of Ninja-invocation functions under `cfg(doctest)` + //! to give doc examples something to call without widening the crate's + //! release-build API. pub use super::redaction::{ CommandArg, is_sensitive_arg, redact_argument, redact_sensitive_args, }; diff --git a/src/stdlib/command/quote.rs b/src/stdlib/command/quote.rs index d58263009..923770eb9 100644 --- a/src/stdlib/command/quote.rs +++ b/src/stdlib/command/quote.rs @@ -91,6 +91,10 @@ pub(super) fn quote(arg: &str) -> Result { #[cfg(all(windows, test))] mod tests { + //! Unit tests for the Windows `cmd.exe` quoting rules implemented by + //! `quote` in the parent module. Gated on `windows` because it exercises + //! the `cfg(windows)` branch of `quote`, so it does not run on other + //! platforms; see `non_windows_tests` below for the Unix counterpart. use super::*; use anyhow::{Result, ensure}; diff --git a/src/stdlib/which/cache.rs b/src/stdlib/which/cache.rs index d709b795d..f8c847f59 100644 --- a/src/stdlib/which/cache.rs +++ b/src/stdlib/which/cache.rs @@ -276,7 +276,7 @@ mod tests { let target = cwd.join("target"); test_support::fs::create_dir_all(target.as_std_path())?; - test_support::write_exec(target.as_std_path(), "tool")?; + test_support::write_exec(target.as_path(), "tool")?; let capacity = NonZeroUsize::new(64).expect("non-zero cache capacity"); // Use path_override to set empty PATH instead of mutating global env diff --git a/src/stdlib/which/lookup/tests.rs b/src/stdlib/which/lookup/tests.rs index b7af78ed4..32f6eace6 100644 --- a/src/stdlib/which/lookup/tests.rs +++ b/src/stdlib/which/lookup/tests.rs @@ -39,7 +39,7 @@ fn search_workspace_returns_executable_and_skips_non_exec( #[from(workspace)] workspace_res: Result, ) -> Result<()> { let workspace = workspace_res?; - let exec = write_exec(workspace.root().as_std_path(), "tool")?; + let exec = write_exec(workspace.root(), "tool")?; let non_exec = workspace.root().join("tool2"); test_fs::write(non_exec.as_std_path(), b"not exec").context("write non exec")?; @@ -59,10 +59,10 @@ fn search_workspace_collects_all_matches( #[from(workspace)] workspace_res: Result, ) -> Result<()> { let workspace = workspace_res?; - let first = write_exec(workspace.root().as_std_path(), "tool")?; + let first = write_exec(workspace.root(), "tool")?; let subdir = workspace.root().join("bin"); test_fs::create_dir_all(subdir.as_std_path()).context("mkdir bin")?; - let second = write_exec(subdir.as_std_path(), "tool")?; + let second = write_exec(&subdir, "tool")?; let path_value = std::ffi::OsString::from(workspace.root().as_str()); let snapshot = EnvSnapshot::capture(Some(workspace.root()), Some(path_value.as_os_str())) @@ -85,7 +85,7 @@ fn search_workspace_skips_heavy_directories( let workspace = workspace_res?; let heavy = workspace.root().join("target"); test_fs::create_dir_all(heavy.as_std_path()).context("mkdir target")?; - write_exec(heavy.as_std_path(), "tool")?; + write_exec(&heavy, "tool")?; let path_value = std::ffi::OsString::from(workspace.root().as_str()); let snapshot = EnvSnapshot::capture(Some(workspace.root()), Some(path_value.as_os_str())) diff --git a/test_support/dylint.toml b/test_support/dylint.toml new file mode 100644 index 000000000..e006f3ef5 --- /dev/null +++ b/test_support/dylint.toml @@ -0,0 +1,36 @@ +# Whitaker lint configuration for the `test_support` crate. +# +# `test_support` is deliberately excluded from the root workspace (see the +# `[workspace] exclude` note in `../Cargo.toml`), so `whitaker --all` at the +# repository root cannot reach it and the root `dylint.toml` does not apply. +# `make lint-whitaker` therefore runs the suite a second time from this +# directory, where this file supplies the crate's own policy. +# +# Reuse policy: the same rule as the root configuration. Do not add an entry to +# silence a new diagnostic. Route fixture filesystem access through +# `test_support::fs`, which is the crate's ambient boundary module, and reach +# for an entry here only when the operation cannot be expressed through a thin +# wrapper at all. +[no_std_fs_operations] + +# `test_support::fs` is the general-purpose ambient boundary. Test fixtures +# stage workspaces in ambient temporary directories, where a `cap_std::fs::Dir` +# handle adds ceremony without isolation value, so that module wraps the +# operations fixtures need and the rest of the crate goes through it: `exec`, +# `manifest`, and the crate-root regression tests call it directly, while +# `check_ninja` and `fake_ninja` reach it via +# `exec::write_exec_with_content`. +# +# `dev_fast` is not listed. Its sandbox, staging, release, and cargo-log +# modules stage throwaway PATH/HOME trees for the build-acceleration tests, and +# every operation they need has a wrapper here — including the ones that once +# looked irreducible. Backdating a fixture needs one open file for the write and +# the timestamp, but `fs::write_with_mtime` takes the timestamp as an argument +# and keeps the handle inside this module; reading an mtime never opened a file +# at all, and `fs::modified` returns the `SystemTime` directly. +# +# The lesson is worth keeping: reach for an entry below only after trying to +# express the operation as a wrapper that returns plain data. A wrapper that +# hands a `std::fs::File` back would just move the diagnostic to its caller, but +# that is an argument against that wrapper shape, not for an exemption. +excluded_paths = ["test_support::fs"] diff --git a/test_support/src/check_ninja.rs b/test_support/src/check_ninja.rs index c262a2d03..eec465f12 100644 --- a/test_support/src/check_ninja.rs +++ b/test_support/src/check_ninja.rs @@ -4,7 +4,7 @@ use anyhow::{Context, Result}; use std::path::{Path, PathBuf}; use tempfile::TempDir; -use crate::exec::write_exec_with_content; +use crate::exec::{utf8_path, write_exec_with_content}; /// Represents a Ninja tool name (e.g., "clean", "compdb"). #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -64,9 +64,13 @@ impl ShellFlag { /// both so callers keep the directory alive for the script's lifetime. fn write_fake_ninja_script(script: &str, context: &str) -> Result<(TempDir, PathBuf)> { let dir = TempDir::new().with_context(|| format!("{context}: create temp dir"))?; - let path = write_exec_with_content(dir.path(), "ninja", script) - .with_context(|| format!("{context}: write script"))?; - Ok((dir, path)) + let path = { + let root = + utf8_path(dir.path()).with_context(|| format!("{context}: temporary directory"))?; + write_exec_with_content(root, "ninja", script) + .with_context(|| format!("{context}: write script"))? + }; + Ok((dir, path.into_std_path_buf())) } /// Create a fake Ninja that validates the build file path provided via `-f`. diff --git a/test_support/src/command_helper.rs b/test_support/src/command_helper.rs index 0f03bb120..9705c100e 100644 --- a/test_support/src/command_helper.rs +++ b/test_support/src/command_helper.rs @@ -53,7 +53,8 @@ const LARGE_OUTPUT_SOURCE: &str = concat!( /// .expect("utf8 path"); /// let dir = Dir::open_ambient_dir(&root, ambient_authority()) /// .expect("open temp dir"); -/// let exe = compile_uppercase_helper(&dir, &root, "cmd_upper"); +/// let exe = compile_uppercase_helper(&dir, &root, "cmd_upper") +/// .expect("compile helper"); /// assert!(exe.as_std_path().exists()); /// ``` pub fn compile_uppercase_helper(dir: &Dir, root: &Utf8PathBuf, name: &str) -> Result { @@ -75,7 +76,8 @@ pub fn compile_uppercase_helper(dir: &Dir, root: &Utf8PathBuf, name: &str) -> Re /// .expect("utf8 path"); /// let dir = Dir::open_ambient_dir(&root, ambient_authority()) /// .expect("open temp dir"); -/// let exe = compile_failure_helper(&dir, &root, "cmd_fail"); +/// let exe = compile_failure_helper(&dir, &root, "cmd_fail") +/// .expect("compile helper"); /// assert!(exe.as_std_path().exists()); /// ``` pub fn compile_failure_helper(dir: &Dir, root: &Utf8PathBuf, name: &str) -> Result { @@ -114,7 +116,8 @@ pub fn compile_large_output_helper( /// &root, /// "cmd", /// "fn main() {}\n", -/// ); +/// ) +/// .expect("compile helper"); /// assert!(exe.as_std_path().exists()); /// ``` pub fn compile_rust_helper( @@ -123,7 +126,7 @@ pub fn compile_rust_helper( name: &str, source: &str, ) -> Result { - dir.write(&format!("{name}.rs"), source.as_bytes()) + dir.write(format!("{name}.rs"), source.as_bytes()) .with_context(|| format!("write helper source {name}.rs"))?; let src_path = root.join(format!("{name}.rs")); diff --git a/test_support/src/cwd_guard.rs b/test_support/src/cwd_guard.rs index 44260627e..e817fa585 100644 --- a/test_support/src/cwd_guard.rs +++ b/test_support/src/cwd_guard.rs @@ -29,6 +29,8 @@ impl Drop for CwdGuard { #[cfg(test)] mod tests { + //! Unit tests for the working-directory guard. + use super::*; use crate::env_lock::EnvLock; use rstest::{fixture, rstest}; @@ -39,27 +41,45 @@ mod tests { EnvLock::acquire() } + /// The environment lock paired with the directory captured under it. + type LockedOriginalDir = (EnvLock, io::Result); + + /// Capture the directory that is current before a test mutates it. + /// + /// Fixtures arrange state rather than assert, so this propagates the + /// `current_dir` failure instead of panicking; each test body unwraps it. + /// + /// The lock is returned rather than merely taken as a parameter: a + /// by-value parameter is dropped when this fixture returns, which would + /// release the lock before the test body runs and leave the body's + /// `set_current_dir` racing other tests. Handing the guard back keeps the + /// process-wide lock held until the test body ends. #[fixture] - fn original_dir(_env_lock: EnvLock) -> std::path::PathBuf { - std::env::current_dir().expect("current_dir") + fn original_dir(env_lock: EnvLock) -> LockedOriginalDir { + let captured = std::env::current_dir(); + (env_lock, captured) } #[rstest] #[case(CwdGuard::acquire)] #[case(CwdGuard::new)] fn constructor_captures_current_directory( - original_dir: std::path::PathBuf, + original_dir: LockedOriginalDir, #[case] ctor: fn() -> io::Result, ) { + let (_env_lock, captured) = original_dir; + let expected = captured.expect("current_dir"); let guard = ctor().expect("CwdGuard constructor"); assert_eq!( - guard.0, original_dir, + guard.0, expected, "guard should capture the directory that was current at acquire time" ); } #[rstest] - fn drop_restores_original_directory(original_dir: std::path::PathBuf) { + fn drop_restores_original_directory(original_dir: LockedOriginalDir) { + let (_env_lock, captured) = original_dir; + let original_dir = captured.expect("current_dir"); let temp = tempfile::tempdir().expect("tempdir"); { diff --git a/test_support/src/dev_fast/cargo_log.rs b/test_support/src/dev_fast/cargo_log.rs index dd248825a..56227985c 100644 --- a/test_support/src/dev_fast/cargo_log.rs +++ b/test_support/src/dev_fast/cargo_log.rs @@ -8,10 +8,10 @@ use anyhow::{Context, Result, bail}; use camino::{Utf8Path, Utf8PathBuf}; use std::collections::HashMap; -use std::fs; use std::io::ErrorKind; use super::Sandbox; +use crate::fs; /// Separator between records in the log. Chosen so it cannot collide with an /// argument or a path. @@ -78,7 +78,7 @@ impl RecordingCargo { /// failure is propagated: a permission or I/O error must not masquerade as /// "cargo did not run", which is exactly the conclusion some tests draw. pub fn invocations(&self) -> Result> { - let text = match fs::read_to_string(self.log.as_std_path()) { + let text = match fs::read_to_string(&self.log) { Ok(text) => text, Err(error) if error.kind() == ErrorKind::NotFound => return Ok(Vec::new()), Err(error) => { diff --git a/test_support/src/dev_fast/release.rs b/test_support/src/dev_fast/release.rs index 5b4a708b5..8656f1b0c 100644 --- a/test_support/src/dev_fast/release.rs +++ b/test_support/src/dev_fast/release.rs @@ -10,10 +10,10 @@ use anyhow::{Context, Result, ensure}; use camino::{Utf8Path, Utf8PathBuf}; -use std::fs; use std::process::Command; use super::Sandbox; +use crate::fs; /// A published fake release, ready for the installer to fetch. pub struct FakeRelease { @@ -62,8 +62,7 @@ impl FakeRelease { /// Write a version pin naming this release, and return its path. pub fn write_version_pin(&self, sandbox: &Sandbox) -> Result { let path = sandbox.home().join("MOLD_VERSION"); - fs::write(path.as_std_path(), format!("{}\n", self.version)) - .context("write test version pin")?; + fs::write(&path, format!("{}\n", self.version)).context("write test version pin")?; Ok(path) } @@ -83,7 +82,7 @@ impl FakeRelease { fn write_checksum_file(&self, sandbox: &Sandbox, contents: &str) -> Result { let path = sandbox.home().join("SHA256SUMS"); - fs::write(path.as_std_path(), contents).context("write test checksum file")?; + fs::write(&path, contents).context("write test checksum file")?; Ok(path) } } @@ -91,12 +90,8 @@ impl FakeRelease { /// Lay out the tarball's versioned root containing `bin/mold`, so a correct /// `--strip-components` lands the binary directly in the install prefix. fn stage_release_tree(root: &Utf8Path) -> Result<()> { - fs::create_dir_all(root.join("bin").as_std_path()).context("stage fake release tree")?; - fs::write( - root.join("bin/mold").as_std_path(), - "#!/bin/sh\necho fake\n", - ) - .context("write staged mold") + fs::create_dir_all(root.join("bin")).context("stage fake release tree")?; + fs::write(root.join("bin/mold"), "#!/bin/sh\necho fake\n").context("write staged mold") } fn build_archive( @@ -137,8 +132,8 @@ fn publish_under_version_path( version: &str, ) -> Result<()> { let versioned = directory.join(format!("v{version}")); - fs::create_dir_all(versioned.as_std_path()).context("create versioned release path")?; - fs::copy(archive.as_std_path(), versioned.join(name).as_std_path()) + fs::create_dir_all(&versioned).context("create versioned release path")?; + fs::copy(archive, versioned.join(name)) .context("publish fake release under its version path")?; Ok(()) } diff --git a/test_support/src/dev_fast/sandbox.rs b/test_support/src/dev_fast/sandbox.rs index f1cda2c26..2b4a0c95b 100644 --- a/test_support/src/dev_fast/sandbox.rs +++ b/test_support/src/dev_fast/sandbox.rs @@ -4,14 +4,13 @@ //! than by prepending fakes to the ambient `PATH`. use anyhow::{Context, Result, bail}; use camino::{Utf8Path, Utf8PathBuf}; -use std::fs; use std::io::ErrorKind; -use std::os::unix::fs::PermissionsExt; use std::process::{Command, Output}; use tempfile::{TempDir, tempdir}; use super::MakeInvocation; use crate::exec::write_exec_with_content; +use crate::fs; /// Utilities the scripts and `make` legitimately need. Kept explicit so a new /// dependency surfaces as a test failure rather than silently resolving to @@ -78,8 +77,8 @@ impl Sandbox { root, repo, }; - fs::create_dir_all(sandbox.bin().as_std_path()).context("create sandbox bin")?; - fs::create_dir_all(sandbox.home().as_std_path()).context("create sandbox home")?; + fs::create_dir_all(sandbox.bin()).context("create sandbox bin")?; + fs::create_dir_all(sandbox.home()).context("create sandbox home")?; sandbox.link_utilities()?; Ok(sandbox) } @@ -103,11 +102,8 @@ impl Sandbox { for utility in SANDBOX_UTILITIES { let source = which(utility).with_context(|| format!("locate `{utility}` for the sandbox"))?; - std::os::unix::fs::symlink( - source.as_std_path(), - self.bin().join(utility).as_std_path(), - ) - .with_context(|| format!("link `{utility}` into the sandbox"))?; + fs::symlink(&source, self.bin().join(utility)) + .with_context(|| format!("link `{utility}` into the sandbox"))?; } Ok(()) } @@ -117,21 +113,20 @@ impl Sandbox { /// `body` is a shell fragment without a shebang; this adds one, so call /// sites stay focused on the behaviour they are faking. pub fn write_fake(&self, dir: &Utf8Path, name: &str, body: &str) -> Result { - fs::create_dir_all(dir.as_std_path()).with_context(|| format!("create {dir}"))?; + fs::create_dir_all(dir).with_context(|| format!("create {dir}"))?; // Unlink first. The utility allowlist symlinks real binaries into this // directory, and writing to a symlink follows it — faking a utility // that is already linked would otherwise truncate the host's copy of // it. Only file permissions have stood between that and a broken // system. let target = dir.join(name); - match fs::remove_file(target.as_std_path()) { + match fs::remove_file(&target) { Ok(()) => {} Err(error) if error.kind() == ErrorKind::NotFound => {} Err(error) => return Err(error).with_context(|| format!("replace {target}")), } let script = format!("#!/bin/sh\n{body}\n"); - let path = write_exec_with_content(dir.as_std_path(), name, &script)?; - Utf8PathBuf::try_from(path).context("fake executable path must be UTF-8") + write_exec_with_content(dir, name, &script) } /// A `mold` reporting the given version, formatted as the real one does. @@ -182,7 +177,7 @@ impl Sandbox { /// read failure is propagated rather than reported as "it did not run". pub fn rustup_invocations(&self) -> Result> { let log = self.rustup_log(); - match fs::read_to_string(log.as_std_path()) { + match fs::read_to_string(&log) { Ok(text) => Ok(text.lines().map(str::to_owned).collect()), Err(error) if error.kind() == ErrorKind::NotFound => Ok(Vec::new()), Err(error) => Err(error).with_context(|| format!("read {log}")), @@ -295,9 +290,7 @@ fn which(utility: &str) -> Result { } fn is_executable_file(path: &Utf8Path) -> bool { - fs::metadata(path.as_std_path()) - .map(|metadata| metadata.is_file() && metadata.permissions().mode() & 0o111 != 0) - .unwrap_or(false) + fs::is_executable_file(path) } /// Combined stdout and stderr, for asserting on diagnostics regardless of the diff --git a/test_support/src/dev_fast/scenario.rs b/test_support/src/dev_fast/scenario.rs index 35d395322..092b4c52b 100644 --- a/test_support/src/dev_fast/scenario.rs +++ b/test_support/src/dev_fast/scenario.rs @@ -134,7 +134,62 @@ impl BuildScenario { /// Run `target`, pointing `CARGO` at the recording fake, and return the /// single invocation it must have produced. + /// + /// Use [`run_all`](Self::run_all) for a target that invokes Cargo more than + /// once, such as `dev-test`'s root and `test_support` passes. + /// + /// # Examples + /// + /// ```rust,no_run + /// use anyhow::Result; + /// use test_support::dev_fast::BuildScenario; + /// + /// fn run() -> Result<()> { + /// let scenario = BuildScenario::prepare()?; + /// let invocation = scenario.run("dev-build")?; + /// assert!(invocation.contains_sequence(&["build", "--bin", "netsuke"])); + /// assert!(!invocation.toolchain().is_empty()); + /// Ok(()) + /// } + /// ``` pub fn run(&self, target: &str) -> Result { + self.run_recording(target)?; + self.cargo.sole_invocation() + } + + /// Run `target` and return every invocation it produced, in order. + /// + /// Use this instead of [`run`](Self::run) when the target invokes Cargo + /// more than once — `dev-test`, for example, runs a root pass and then a + /// `test_support` pass, and `run` would fail rather than pick one. + /// + /// # Examples + /// + /// ```rust,no_run + /// use anyhow::Result; + /// use test_support::dev_fast::BuildScenario; + /// + /// fn run() -> Result<()> { + /// let scenario = BuildScenario::prepare()?; + /// let invocations = scenario.run_all("dev-test")?; + /// assert_eq!(invocations.len(), 2); + /// for invocation in &invocations { + /// assert!(invocation.contains_sequence(&["nextest", "run"])); + /// } + /// Ok(()) + /// } + /// ``` + pub fn run_all(&self, target: &str) -> Result> { + self.run_recording(target)?; + let invocations = self.cargo.invocations()?; + ensure!( + !invocations.is_empty(), + "make {target} should invoke cargo at least once" + ); + Ok(invocations) + } + + fn run_recording(&self, target: &str) -> Result<()> { let invocation = MakeInvocation::new(target).variable("CARGO", self.cargo.executable()); let output = self.sandbox.run_make(&invocation)?; ensure!( @@ -142,6 +197,6 @@ impl BuildScenario { "make {target} should succeed, got `{}`", combined(&output) ); - self.cargo.sole_invocation() + Ok(()) } } diff --git a/test_support/src/dev_fast/staging.rs b/test_support/src/dev_fast/staging.rs index 8601f068d..b1544e6d5 100644 --- a/test_support/src/dev_fast/staging.rs +++ b/test_support/src/dev_fast/staging.rs @@ -9,12 +9,10 @@ use anyhow::{Context, Result}; use camino::Utf8Path; -use std::fs; -use std::fs::File; -use std::os::unix::fs::FileExt; use std::time::SystemTime; use super::Sandbox; +use crate::fs; impl Sandbox { /// Write a fixture file, creating its parent directory. @@ -26,7 +24,7 @@ impl Sandbox { /// crate instead of widening the Whitaker exclusion list. pub fn write_file(&self, path: &Utf8Path, contents: &str) -> Result<()> { self.create_parent(path)?; - fs::write(path.as_std_path(), contents).with_context(|| format!("write {path}")) + fs::write(path, contents).with_context(|| format!("write {path}")) } /// Write a fixture file and backdate it to `mtime`, in seconds since the @@ -42,11 +40,8 @@ impl Sandbox { mtime: SystemTime, ) -> Result<()> { self.create_parent(path)?; - let file = File::create(path.as_std_path()).with_context(|| format!("create {path}"))?; - file.write_all_at(contents.as_bytes(), 0) - .with_context(|| format!("write {path}"))?; - file.set_modified(mtime) - .with_context(|| format!("backdate {path}")) + fs::write_with_mtime(path, contents, mtime) + .with_context(|| format!("write and backdate {path}")) } /// Read a file staged in the sandbox. @@ -55,7 +50,7 @@ impl Sandbox { /// on recorded output would otherwise read "the command was never run" as /// "the command recorded nothing". pub fn read_file(&self, path: &Utf8Path) -> Result { - fs::read_to_string(path.as_std_path()).with_context(|| format!("read {path}")) + fs::read_to_string(path).with_context(|| format!("read {path}")) } /// A file's modification time, in whole seconds since the Unix epoch. @@ -63,10 +58,7 @@ impl Sandbox { /// Whole seconds because the callers compare against a deliberately /// backdated stamp, not against each other. pub fn mtime_seconds(&self, path: &Utf8Path) -> Result { - let modified = fs::metadata(path.as_std_path()) - .with_context(|| format!("stat {path}"))? - .modified() - .with_context(|| format!("read mtime of {path}"))?; + let modified = fs::modified(path).with_context(|| format!("read mtime of {path}"))?; let since_epoch = modified .duration_since(SystemTime::UNIX_EPOCH) .with_context(|| format!("{path} predates the Unix epoch"))?; @@ -75,7 +67,7 @@ impl Sandbox { /// Create a directory and any missing parents. pub fn create_dir(&self, path: &Utf8Path) -> Result<()> { - fs::create_dir_all(path.as_std_path()).with_context(|| format!("create {path}")) + fs::create_dir_all(path).with_context(|| format!("create {path}")) } fn create_parent(&self, path: &Utf8Path) -> Result<()> { diff --git a/test_support/src/env.rs b/test_support/src/env.rs index 7cebedb8d..dcc9979e9 100644 --- a/test_support/src/env.rs +++ b/test_support/src/env.rs @@ -292,7 +292,7 @@ pub fn override_ninja_env(env: &impl EnvMut, path: &Path) -> NinjaEnvGuard { // races while the guard is alive. unsafe { env.set_var(NINJA_ENV, path.as_os_str()) }; NinjaEnvGuard { - inner: EnvGuard::with_env_and_lock(NINJA_ENV, original, StdEnv::default(), false), + inner: EnvGuard::with_env_and_lock(NINJA_ENV, original, StdEnv, false), _lock: lock, } } diff --git a/test_support/src/env_guard.rs b/test_support/src/env_guard.rs index 52d15ac24..890210c65 100644 --- a/test_support/src/env_guard.rs +++ b/test_support/src/env_guard.rs @@ -57,12 +57,12 @@ pub struct EnvGuard { impl EnvGuard { /// Create a guard for `key` using [`StdEnv`]. pub fn new(key: impl Into>, original: Option) -> Self { - Self::with_env_and_lock(key, original, StdEnv::default(), true) + Self::with_env_and_lock(key, original, StdEnv, true) } /// Create a guard that skips locking on drop. pub fn new_unlocked(key: impl Into>, original: Option) -> Self { - Self::with_env_and_lock(key, original, StdEnv::default(), false) + Self::with_env_and_lock(key, original, StdEnv, false) } } diff --git a/test_support/src/env_lock.rs b/test_support/src/env_lock.rs index 4f242f0cd..d8494b07e 100644 --- a/test_support/src/env_lock.rs +++ b/test_support/src/env_lock.rs @@ -64,19 +64,81 @@ impl Drop for EnvLock { #[cfg(test)] mod tests { + //! Unit tests for the environment mutation lock. + use super::*; + use rstest::{fixture, rstest}; + use std::sync::PoisonError; + use std::thread; + + /// Serialises this module's tests against each other. + /// + /// Only the poisoning test needs this: it sets and clears the sticky, + /// process-global poison flag, and two tests doing that concurrently would + /// observe each other. The held/released assertions below are thread-local + /// and need no serialisation. nextest isolates every test in its own + /// process and never sees any of this, but plain `cargo test` shares one, + /// and the crate must not be flaky under either. + static TEST_SERIAL: Mutex<()> = Mutex::new(()); + + /// Hold the module's serialisation lock for the whole test. + /// + /// A fixture rather than a `let` binding so the guard is a parameter, and + /// so drops after every local the test declares. + /// + /// Recovers from poisoning: a panicking test leaves the flag set, and that + /// must not cascade into every later test in the module. + #[fixture] + fn serialised() -> MutexGuard<'static, ()> { + TEST_SERIAL.lock().unwrap_or_else(PoisonError::into_inner) + } - fn assert_underlying_lock_is_held(message: &str) { - assert!(ENV_LOCK.try_lock().is_err(), "{message}"); + // Macros rather than helper functions so a failure reports the calling + // test's line number. + // + // These probe this thread's `ENV_LOCK_STATE`, not `ENV_LOCK.try_lock()`. + // The global probe was racy and did fail in practice: `ENV_LOCK` is + // acquired by `env::set_var`, `env::with_isolated_path`, + // `EnvGuard::drop` and other *library* functions, so any test anywhere + // calling the public API holds it transitively. A concurrent holder made + // `try_lock` return `WouldBlock`, and the released-assertion reported + // "ENV_LOCK is still held" for a guard this thread had correctly dropped. + // Serialising this module's tests could not fix that, because the + // competing acquisitions are not in this module and not in tests at all. + // + // The thread-local probe is both race-free and more precise. Holding a + // `MutexGuard` means the mutex is locked, so `guard.is_some()` is exactly + // "this thread holds `ENV_LOCK`" and `guard.is_none()` is exactly "this + // thread released it" — which is the `Drop` contract under test. Another + // thread's unrelated guard is now correctly invisible. + macro_rules! assert_underlying_lock_is_held { + ($message:expr $(,)?) => { + ENV_LOCK_STATE.with(|state| { + assert!( + state.borrow().guard.is_some(), + "{}: this thread holds no ENV_LOCK guard", + $message + ); + }); + }; } - fn assert_underlying_lock_is_released(message: &str) { - let lock = ENV_LOCK.try_lock().expect(message); - drop(lock); + macro_rules! assert_underlying_lock_is_released { + ($message:expr $(,)?) => { + ENV_LOCK_STATE.with(|state| { + assert!( + state.borrow().guard.is_none(), + "{}: this thread still holds an ENV_LOCK guard", + $message + ); + }); + }; } - #[test] - fn reentrant_env_lock_nested_acquire_and_release() { + #[rstest] + fn reentrant_env_lock_nested_acquire_and_release( + #[from(serialised)] _serial: MutexGuard<'static, ()>, + ) { { let _outer = EnvLock::acquire(); let _inner = EnvLock::acquire(); @@ -85,33 +147,81 @@ mod tests { let outer = EnvLock::acquire(); { let _inner = EnvLock::acquire(); - assert_underlying_lock_is_held( + assert_underlying_lock_is_held!( "ENV_LOCK should remain locked while nested EnvLock guards are alive", ); } - assert_underlying_lock_is_held( + assert_underlying_lock_is_held!( "ENV_LOCK should remain locked until the outer EnvLock guard is dropped", ); drop(outer); - assert_underlying_lock_is_released( + assert_underlying_lock_is_released!( "ENV_LOCK should be unlocked after final EnvLock guard is dropped", ); } - #[test] - fn reentrant_env_lock_stays_locked_when_outer_drops_first() { + /// Poison `ENV_LOCK` the way a panicking test would: hold the guard across + /// a panic on another thread. + /// + /// The panic hook is left alone. `panic::set_hook` is process-wide, so + /// swapping it out would suppress the report any concurrently panicking + /// test relies on. One line of stderr noise is the cheaper cost. + fn poison_env_lock() { + let poisoner = thread::spawn(|| { + let _guard = ENV_LOCK.lock(); + panic!("deliberately poisoning ENV_LOCK"); + }); + assert!( + poisoner.join().is_err(), + "the poisoning thread should have panicked" + ); + assert!(ENV_LOCK.is_poisoned(), "ENV_LOCK should now be poisoned"); + } + + #[rstest] + fn env_lock_recovers_from_a_poisoned_mutex( + #[from(serialised)] _serial: MutexGuard<'static, ()>, + ) { + poison_env_lock(); + + // `acquire` must recover through `PoisonError::into_inner` rather than + // propagating: a panic here would fail every later test taking the lock. + let guard = EnvLock::acquire(); + assert_underlying_lock_is_held!( + "a recovered EnvLock should still hold the underlying mutex", + ); + + drop(guard); + assert_underlying_lock_is_released!( + "a recovered EnvLock should release the mutex when dropped", + ); + + // Leave the static as it was found. The flag is sticky and the mutex is + // shared with every other test, so a deliberate poisoning must not + // outlive the test that caused it. + ENV_LOCK.clear_poison(); + assert!( + !ENV_LOCK.is_poisoned(), + "the poison flag should be cleared before leaving the test" + ); + } + + #[rstest] + fn reentrant_env_lock_stays_locked_when_outer_drops_first( + #[from(serialised)] _serial: MutexGuard<'static, ()>, + ) { let outer = EnvLock::acquire(); let inner = EnvLock::acquire(); drop(outer); - assert_underlying_lock_is_held( + assert_underlying_lock_is_held!( "ENV_LOCK should remain locked while an inner EnvLock guard is alive", ); drop(inner); - assert_underlying_lock_is_released( + assert_underlying_lock_is_released!( "ENV_LOCK should be unlocked after the final out-of-order guard drops", ); } diff --git a/test_support/src/exec.rs b/test_support/src/exec.rs index c3bf8b8cc..3e63d0bb4 100644 --- a/test_support/src/exec.rs +++ b/test_support/src/exec.rs @@ -4,28 +4,89 @@ //! tests can exercise PATH resolution without depending on real binaries. //! Callers own the containing directory's lifetime to keep the stub on disk. //! +//! Paths are camino UTF-8 types throughout, matching the rest of Netsuke. +//! `tempfile` yields OS-native paths, so callers convert at that boundary with +//! [`utf8_path`], which reports a non-UTF-8 path rather than discarding it. +//! //! # Examples //! //! ```rust //! use tempfile::TempDir; -//! use test_support::write_exec; +//! use test_support::exec::{utf8_path, write_exec}; //! //! let temp = TempDir::new().expect("tempdir"); -//! let path = write_exec(temp.path(), "tool").expect("stub executable"); +//! let root = utf8_path(temp.path()).expect("temporary directory is UTF-8"); +//! let path = write_exec(root, "tool").expect("stub executable"); //! assert!(path.exists()); //! ``` -use anyhow::{Context, Result}; -use std::{ - fs, - path::{Path, PathBuf}, -}; +use crate::fs; +use anyhow::{Context, Result, bail}; +use camino::{Utf8Component, Utf8Path, Utf8PathBuf}; +use std::path::Path; -#[cfg(unix)] -use std::os::unix::fs::PermissionsExt; +/// Borrow an OS-native path as UTF-8, naming the path when it is not. +/// +/// This is the single conversion boundary between `tempfile`'s OS-native paths +/// and the camino types the stub helpers take. It returns an error rather than +/// panicking so callers propagate the failure with their own context. +/// +/// # Errors +/// +/// Returns an error identifying `path` when it is not valid UTF-8. +/// +/// # Examples +/// +/// ```rust +/// use tempfile::TempDir; +/// use test_support::exec::utf8_path; +/// +/// let temp = TempDir::new().expect("tempdir"); +/// let root = utf8_path(temp.path()).expect("temporary directory is UTF-8"); +/// assert!(root.is_absolute()); +/// ``` +/// +/// A path that is not valid UTF-8 is reported rather than silently lost: +/// +/// ```rust +/// # #[cfg(unix)] +/// # { +/// use std::{ffi::OsString, os::unix::ffi::OsStringExt, path::PathBuf}; +/// use test_support::exec::utf8_path; +/// +/// let raw = PathBuf::from(OsString::from_vec(b"/tmp/not-\xff-utf8".to_vec())); +/// let err = utf8_path(&raw).expect_err("path is not UTF-8"); +/// assert!(err.to_string().contains("not valid UTF-8")); +/// # } +/// ``` +pub fn utf8_path(path: &Path) -> Result<&Utf8Path> { + Utf8Path::from_path(path) + .with_context(|| format!("path is not valid UTF-8: {}", path.display())) +} /// Write a minimal executable file named `name` inside `root`. -pub fn write_exec(root: &Path, name: &str) -> Result { +/// +/// # Errors +/// +/// Returns an error when `name` is not a single file-name component, or +/// when the stub cannot be written or marked executable. +/// +/// # Examples +/// +/// ```rust +/// use tempfile::TempDir; +/// use test_support::exec::{utf8_path, write_exec}; +/// +/// let temp = TempDir::new().expect("tempdir"); +/// let root = utf8_path(temp.path()).expect("temporary directory is UTF-8"); +/// let path = write_exec(root, "tool").expect("stub executable"); +/// assert!(test_support::fs::exists(&path)); +/// # #[cfg(unix)] +/// # { +/// assert!(test_support::fs::is_executable_file(&path)); +/// # } +/// ``` +pub fn write_exec(root: &Utf8Path, name: &str) -> Result { write_exec_with_content(root, name, "#!/bin/sh\n") } @@ -36,35 +97,138 @@ pub fn write_exec(root: &Path, name: &str) -> Result { /// executable on Unix. Callers provide platform-appropriate content (for /// example a POSIX shell script on Unix or a batch file on Windows). /// +/// # Errors +/// +/// Returns an error when `name` is not a single file-name component, or +/// when the stub cannot be written or marked executable. +/// /// # Examples /// /// ```rust /// use tempfile::TempDir; -/// use test_support::exec::write_exec_with_content; +/// use test_support::exec::{utf8_path, write_exec_with_content}; /// /// let temp = TempDir::new().expect("tempdir"); -/// let path = write_exec_with_content(temp.path(), "tool", "#!/bin/sh\nexit 3\n") +/// let root = utf8_path(temp.path()).expect("temporary directory is UTF-8"); +/// let path = write_exec_with_content(root, "tool", "#!/bin/sh\nexit 3\n") /// .expect("stub executable"); -/// assert!(path.exists()); +/// assert!(test_support::fs::exists(&path)); /// ``` -pub fn write_exec_with_content(root: &Path, name: &str, content: &str) -> Result { +pub fn write_exec_with_content(root: &Utf8Path, name: &str, content: &str) -> Result { + single_file_name(name)?; let path = root.join(name); fs::write(&path, content).with_context(|| format!("write exec stub {name}"))?; make_executable(&path)?; Ok(path) } +/// Reject a stub name that is anything but a single file-name component. +/// +/// The stub writers join `name` onto `root`, so a name carrying a separator, a +/// parent component, or a root would put the stub somewhere other than the +/// caller's temporary directory — silently, because the write would still +/// succeed. Every caller passes a literal today, which makes this a property of +/// the call sites; checking here makes it a property of the helper instead. +/// +/// The component must equal `name` outright, which also rejects a trailing +/// separator: `components()` normalises `"tool/"` to a lone `Normal("tool")`, +/// so comparing against the original string is what catches it. +fn single_file_name(name: &str) -> Result<()> { + let mut components = Utf8Path::new(name).components(); + match (components.next(), components.next()) { + (Some(Utf8Component::Normal(component)), None) if component == name => Ok(()), + _ => bail!("exec stub name must be a single file name, got {name:?}"), + } +} + /// Mark an existing file as executable by setting its Unix permission bits. +/// +/// # Errors +/// +/// Returns an error when the permission bits cannot be set. +/// +/// # Examples +/// +/// ```rust +/// # #[cfg(unix)] +/// # { +/// use tempfile::TempDir; +/// use test_support::exec::{make_executable, utf8_path}; +/// use test_support::fs::is_executable_file; +/// +/// let temp = TempDir::new().expect("tempdir"); +/// let root = utf8_path(temp.path()).expect("temporary directory is UTF-8"); +/// let path = root.join("tool"); +/// test_support::fs::write(&path, "#!/bin/sh\n").expect("write stub"); +/// assert!(!is_executable_file(&path)); +/// make_executable(&path).expect("mark executable"); +/// assert!(is_executable_file(&path)); +/// # } +/// ``` #[cfg(unix)] -pub fn make_executable(path: &Path) -> Result<()> { - let mut perms = fs::metadata(path).context("stat exec stub")?.permissions(); - perms.set_mode(0o755); - fs::set_permissions(path, perms).context("chmod exec stub")?; +pub fn make_executable(path: &Utf8Path) -> Result<()> { + fs::set_mode(path, 0o755).context("chmod exec stub")?; Ok(()) } /// No-op on non-Unix platforms, where executability is not a permission bit. +/// +/// # Errors +/// +/// Never returns an error; the signature matches the Unix variant. #[cfg(not(unix))] -pub fn make_executable(_path: &Path) -> Result<()> { +pub fn make_executable(_path: &Utf8Path) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + //! Coverage for the stub-name guard. + //! + //! The guard exists so a name can never place the stub outside the root it + //! was given, so the rejected cases are the interesting ones. + + use super::{single_file_name, write_exec_with_content}; + use crate::exec::utf8_path; + use rstest::rstest; + use tempfile::TempDir; + + #[rstest] + #[case::plain("tool")] + #[case::dotted("ninja.cmd")] + #[case::leading_dot(".hidden")] + fn accepts_a_single_file_name(#[case] name: &str) { + assert!( + single_file_name(name).is_ok(), + "{name:?} should be accepted" + ); + } + + #[rstest] + #[case::empty("")] + #[case::current_dir(".")] + #[case::parent("..")] + #[case::separator("nested/tool")] + #[case::leading_separator("/tool")] + #[case::traversal("../tool")] + #[case::dot_prefixed("./tool")] + #[case::trailing_separator("tool/")] + fn rejects_anything_else(#[case] name: &str) { + assert!( + single_file_name(name).is_err(), + "{name:?} should be rejected" + ); + } + + #[test] + fn write_exec_with_content_rejects_a_traversing_name() { + let temp = TempDir::new().expect("tempdir"); + let root = utf8_path(temp.path()).expect("temporary directory is UTF-8"); + let error = write_exec_with_content(root, "../escaped", "#!/bin/sh\n") + .expect_err("a traversing name should be rejected"); + assert!( + format!("{error:#}").contains("single file name"), + "unexpected error: {error:#}" + ); + } +} diff --git a/test_support/src/fs.rs b/test_support/src/fs.rs index 55a5132c8..4a53113df 100644 --- a/test_support/src/fs.rs +++ b/test_support/src/fs.rs @@ -4,16 +4,22 @@ //! `cap_std` handles, enforced by Whitaker's `no_std_fs_operations` lint. //! Test fixtures, however, routinely stage workspaces in ambient temporary //! directories where a capability handle adds ceremony without isolation -//! value. This module confines that ambient access to `test_support`, which -//! `dylint.toml` excludes from the lint — the same pattern Whitaker itself -//! uses for its `whitaker_common` test utilities. +//! value. This module is the crate's single ambient boundary: `test_support/` +//! carries its own `dylint.toml` naming `test_support::fs` as the only entry +//! under `[no_std_fs_operations] excluded_paths`, so a direct `std::fs` call in +//! any other module still fails the lint. //! //! Scope and reuse policy: test fixtures and assertions only; production code -//! must keep using `cap_std`. +//! must keep using `cap_std`. Other `test_support` modules that need fixture +//! I/O call through here rather than reaching for `std::fs` themselves: +//! `exec`, `manifest`, and the crate-root regression tests do so directly, +//! while `check_ninja` and `fake_ninja` reach it via +//! [`crate::exec::write_exec_with_content`]. use std::fs; use std::io; use std::path::Path; +use std::time::SystemTime; /// Write `contents` to `path`, creating or truncating the file. /// @@ -91,6 +97,110 @@ pub fn exists(path: impl AsRef) -> bool { fs::metadata(path).is_ok() } +/// Return `true` when `path` is a directory (following symlinks). +/// +/// Mirrors `Path::is_dir`: an unreadable or absent path reports `false` rather +/// than surfacing the metadata error. +/// +/// # Examples +/// +/// ``` +/// let dir = tempfile::tempdir().expect("create tempdir"); +/// let file = dir.path().join("file"); +/// test_support::fs::write(&file, "contents").expect("write file"); +/// assert!(test_support::fs::is_dir(dir.path())); +/// assert!(!test_support::fs::is_dir(&file)); +/// assert!(!test_support::fs::is_dir(dir.path().join("absent"))); +/// ``` +#[must_use] +pub fn is_dir(path: impl AsRef) -> bool { + fs::metadata(path).is_ok_and(|metadata| metadata.is_dir()) +} + +/// Copy `from` to `to`, returning the number of bytes copied. +/// +/// # Errors +/// +/// Propagates the underlying `std::fs::copy` failure. +/// +/// # Examples +/// +/// ``` +/// let dir = tempfile::tempdir().expect("create tempdir"); +/// let from = dir.path().join("source.txt"); +/// let to = dir.path().join("dest.txt"); +/// test_support::fs::write(&from, "hello").expect("write source"); +/// let bytes = test_support::fs::copy(&from, &to).expect("copy file"); +/// assert_eq!(bytes, 5); +/// assert_eq!(test_support::fs::read(&to).expect("read dest"), b"hello"); +/// ``` +pub fn copy(from: impl AsRef, to: impl AsRef) -> io::Result { + fs::copy(from, to) +} + +/// Return the modification time of the file at `path`. +/// +/// # Errors +/// +/// Propagates the underlying metadata failure, or the platform's failure to +/// report a modification time. +/// +/// # Examples +/// +/// ``` +/// use std::time::{Duration, SystemTime}; +/// +/// let dir = tempfile::tempdir().expect("create tempdir"); +/// let path = dir.path().join("fixture.txt"); +/// // A second of slack: some filesystems truncate timestamps to whole +/// // seconds, which would put the recorded mtime just behind `before`. +/// let before = SystemTime::now() - Duration::from_secs(1); +/// test_support::fs::write(&path, "hello").expect("write fixture"); +/// let mtime = test_support::fs::modified(&path).expect("read mtime"); +/// assert!(mtime >= before); +/// ``` +pub fn modified(path: impl AsRef) -> io::Result { + fs::metadata(path)?.modified() +} + +/// Write `contents` to `path` and set its modification time to `mtime`. +/// +/// Backdating a fixture needs the same open file for the write and the +/// timestamp, so both happen here rather than through separate calls. The +/// handle never leaves this function: returning it would push the ambient +/// operation out to the caller, which is what this module exists to prevent. +/// +/// # Errors +/// +/// Propagates the create, write, or timestamp failure. +/// +/// # Examples +/// +/// ``` +/// # #[cfg(unix)] +/// # { +/// use std::time::{Duration, UNIX_EPOCH}; +/// +/// let dir = tempfile::tempdir().expect("create tempdir"); +/// let path = dir.path().join("fixture.txt"); +/// let mtime = UNIX_EPOCH + Duration::from_secs(1_700_000_000); +/// test_support::fs::write_with_mtime(&path, "hello", mtime).expect("write with mtime"); +/// assert_eq!(test_support::fs::modified(&path).expect("read mtime"), mtime); +/// assert_eq!(test_support::fs::read(&path).expect("read contents"), b"hello"); +/// # } +/// ``` +#[cfg(unix)] +pub fn write_with_mtime( + path: impl AsRef, + contents: impl AsRef<[u8]>, + mtime: SystemTime, +) -> io::Result<()> { + use std::os::unix::fs::FileExt; + let file = fs::File::create(path)?; + file.write_all_at(contents.as_ref(), 0)?; + file.set_modified(mtime) +} + /// Return the length in bytes of the file at `path`. /// /// # Errors @@ -114,6 +224,30 @@ pub fn set_mode(path: impl AsRef, mode: u32) -> io::Result<()> { fs::set_permissions(path, permissions) } +/// Return `true` when `path` is a regular file with any execute bit set. +/// +/// The inverse of [`set_mode`], for probing a sandbox `PATH` the way an +/// executable lookup would. An unreadable or absent path reports `false`. +/// +/// # Examples +/// +/// ``` +/// let dir = tempfile::tempdir().expect("create tempdir"); +/// let path = dir.path().join("tool"); +/// test_support::fs::write(&path, "#!/bin/sh\n").expect("write stub"); +/// assert!(!test_support::fs::is_executable_file(&path)); +/// test_support::fs::set_mode(&path, 0o755).expect("mark executable"); +/// assert!(test_support::fs::is_executable_file(&path)); +/// assert!(!test_support::fs::is_executable_file(dir.path())); +/// ``` +#[cfg(unix)] +#[must_use] +pub fn is_executable_file(path: impl AsRef) -> bool { + use std::os::unix::fs::PermissionsExt; + fs::metadata(path) + .is_ok_and(|metadata| metadata.is_file() && metadata.permissions().mode() & 0o111 != 0) +} + /// Create a symbolic link at `link` pointing to `target` on Unix. /// /// # Errors diff --git a/test_support/src/http.rs b/test_support/src/http.rs index faa60f7e1..27b50b941 100644 --- a/test_support/src/http.rs +++ b/test_support/src/http.rs @@ -21,7 +21,7 @@ use std::{cell::RefCell, thread_local}; #[cfg(test)] thread_local! { - static DURATION_WARNINGS: RefCell> = RefCell::new(Vec::new()); + static DURATION_WARNINGS: RefCell> = const { RefCell::new(Vec::new()) }; } /// Configuration for HTTP fixtures, including timeouts used during polling. @@ -314,171 +314,6 @@ fn take_duration_warnings() -> Vec { DURATION_WARNINGS.with(|warnings| warnings.borrow_mut().drain(..).collect()) } +#[path = "http_tests.rs"] #[cfg(test)] -mod tests { - use super::{ - ENV_HTTP_ACCEPT_TIMEOUT_MS, ENV_HTTP_POLL_INTERVAL_MS, ENV_HTTP_READ_TIMEOUT_MS, - HttpServerConfig, accept_connection, duration_from_env, take_duration_warnings, - }; - - use crate::{EnvVarGuard, env_lock::EnvLock}; - use std::{ - net::TcpListener, - panic, - time::{Duration, Instant}, - }; - - #[test] - fn from_env_applies_overrides() { - let _lock = EnvLock::acquire(); - assert!( - take_duration_warnings().is_empty(), - "warnings buffer should start empty" - ); - let accept = EnvVarGuard::set(ENV_HTTP_ACCEPT_TIMEOUT_MS, "1500"); - let read = EnvVarGuard::set(ENV_HTTP_READ_TIMEOUT_MS, "750"); - let poll = EnvVarGuard::set(ENV_HTTP_POLL_INTERVAL_MS, "25"); - - let config = HttpServerConfig::from_env(); - assert_eq!(config.accept_timeout, Duration::from_millis(1500)); - assert_eq!(config.read_timeout, Duration::from_millis(750)); - assert_eq!(config.poll_interval, Duration::from_millis(25)); - assert!( - take_duration_warnings().is_empty(), - "no warnings expected for valid overrides" - ); - - drop(poll); - drop(read); - drop(accept); - } - - #[test] - fn from_env_clamps_zero_poll_interval() { - let _lock = EnvLock::acquire(); - assert!( - take_duration_warnings().is_empty(), - "warnings buffer should start empty" - ); - let poll = EnvVarGuard::set(ENV_HTTP_POLL_INTERVAL_MS, "0"); - - let config = HttpServerConfig::from_env(); - assert_eq!(config.poll_interval, Duration::from_millis(1)); - assert!( - take_duration_warnings().is_empty(), - "parsing a zero poll interval should not warn", - ); - - drop(poll); - } - - #[test] - fn duration_from_env_returns_default_for_missing() { - let _lock = EnvLock::acquire(); - assert!( - take_duration_warnings().is_empty(), - "warnings buffer should start empty" - ); - let guard = EnvVarGuard::remove(ENV_HTTP_ACCEPT_TIMEOUT_MS); - let duration = duration_from_env(ENV_HTTP_ACCEPT_TIMEOUT_MS, Duration::from_secs(3)); - assert_eq!(duration, Duration::from_secs(3)); - assert!( - take_duration_warnings().is_empty(), - "missing variables should not log warnings" - ); - drop(guard); - } - - #[test] - fn duration_from_env_reports_invalid_values() { - let _lock = EnvLock::acquire(); - assert!( - take_duration_warnings().is_empty(), - "warnings buffer should start empty" - ); - let guard = EnvVarGuard::set(ENV_HTTP_ACCEPT_TIMEOUT_MS, "not-a-number"); - let duration = duration_from_env(ENV_HTTP_ACCEPT_TIMEOUT_MS, Duration::from_secs(3)); - assert_eq!(duration, Duration::from_secs(3)); - let warnings = take_duration_warnings(); - assert_eq!(warnings.len(), 1); - assert!( - warnings[0].contains(ENV_HTTP_ACCEPT_TIMEOUT_MS), - "warning should mention the variable name" - ); - assert!( - warnings[0].contains("not-a-number"), - "warning should include the invalid value" - ); - drop(guard); - } - - #[test] - fn duration_from_env_trims_whitespace() { - let _lock = EnvLock::acquire(); - assert!( - take_duration_warnings().is_empty(), - "warnings buffer should start empty" - ); - let guard = EnvVarGuard::set(ENV_HTTP_READ_TIMEOUT_MS, " 2500 "); - let duration = duration_from_env(ENV_HTTP_READ_TIMEOUT_MS, Duration::from_secs(3)); - assert_eq!(duration, Duration::from_millis(2500)); - assert!( - take_duration_warnings().is_empty(), - "whitespace-only padding should not trigger warnings", - ); - drop(guard); - } - - #[test] - fn accept_connection_respects_accept_timeout() { - let listener = TcpListener::bind(("127.0.0.1", 0)).expect("bind listener"); - listener - .set_nonblocking(true) - .expect("set listener non-blocking"); - - let accept_timeout = Duration::from_millis(20); - let poll_interval = Duration::from_millis(200); - let start = Instant::now(); - let deadline = start + accept_timeout; - - let result = panic::catch_unwind(|| { - let _ = accept_connection(&listener, deadline, poll_interval, accept_timeout); - }); - let panic_payload = - result.expect_err("accept_connection should panic when no client connects"); - - let elapsed = start.elapsed(); - assert!( - elapsed >= accept_timeout, - "panic should not occur before the accept timeout (elapsed {:?}, timeout {:?})", - elapsed, - accept_timeout, - ); - assert!( - elapsed <= accept_timeout + poll_interval + Duration::from_millis(50), - "panic overshot accept timeout by more than one poll interval: elapsed={:?}, accept_timeout={:?}, poll_interval={:?}", - elapsed, - accept_timeout, - poll_interval, - ); - - let panic_ref = panic_payload.as_ref(); - let panic_text = panic_ref - .downcast_ref::() - .cloned() - .or_else(|| { - panic_ref - .downcast_ref::<&'static str>() - .map(|s| s.to_string()) - }) - .unwrap_or_else(|| format!("{panic_payload:?}")); - assert!( - panic_text.contains(&format!("accept_timeout={:?}", accept_timeout)), - "panic message should embed the accept timeout: {panic_text}", - ); - assert!( - panic_text.contains(&format!("poll_interval={:?}", poll_interval)), - "panic message should embed the poll interval: {panic_text}", - ); - } -} +mod tests; diff --git a/test_support/src/http_tests.rs b/test_support/src/http_tests.rs new file mode 100644 index 000000000..d3e7d0d1b --- /dev/null +++ b/test_support/src/http_tests.rs @@ -0,0 +1,166 @@ +//! Unit tests for the lightweight test HTTP server. + +use super::{ + ENV_HTTP_ACCEPT_TIMEOUT_MS, ENV_HTTP_POLL_INTERVAL_MS, ENV_HTTP_READ_TIMEOUT_MS, + HttpServerConfig, accept_connection, duration_from_env, take_duration_warnings, +}; + +use crate::{EnvVarGuard, env_lock::EnvLock}; +use std::{ + net::TcpListener, + panic, + time::{Duration, Instant}, +}; + +#[test] +fn from_env_applies_overrides() { + let _lock = EnvLock::acquire(); + assert!( + take_duration_warnings().is_empty(), + "warnings buffer should start empty" + ); + let accept = EnvVarGuard::set(ENV_HTTP_ACCEPT_TIMEOUT_MS, "1500"); + let read = EnvVarGuard::set(ENV_HTTP_READ_TIMEOUT_MS, "750"); + let poll = EnvVarGuard::set(ENV_HTTP_POLL_INTERVAL_MS, "25"); + + let config = HttpServerConfig::from_env(); + assert_eq!(config.accept_timeout, Duration::from_millis(1500)); + assert_eq!(config.read_timeout, Duration::from_millis(750)); + assert_eq!(config.poll_interval, Duration::from_millis(25)); + assert!( + take_duration_warnings().is_empty(), + "no warnings expected for valid overrides" + ); + + drop(poll); + drop(read); + drop(accept); +} + +#[test] +fn from_env_clamps_zero_poll_interval() { + let _lock = EnvLock::acquire(); + assert!( + take_duration_warnings().is_empty(), + "warnings buffer should start empty" + ); + let poll = EnvVarGuard::set(ENV_HTTP_POLL_INTERVAL_MS, "0"); + + let config = HttpServerConfig::from_env(); + assert_eq!(config.poll_interval, Duration::from_millis(1)); + assert!( + take_duration_warnings().is_empty(), + "parsing a zero poll interval should not warn", + ); + + drop(poll); +} + +#[test] +fn duration_from_env_returns_default_for_missing() { + let _lock = EnvLock::acquire(); + assert!( + take_duration_warnings().is_empty(), + "warnings buffer should start empty" + ); + let guard = EnvVarGuard::remove(ENV_HTTP_ACCEPT_TIMEOUT_MS); + let duration = duration_from_env(ENV_HTTP_ACCEPT_TIMEOUT_MS, Duration::from_secs(3)); + assert_eq!(duration, Duration::from_secs(3)); + assert!( + take_duration_warnings().is_empty(), + "missing variables should not log warnings" + ); + drop(guard); +} + +#[test] +fn duration_from_env_reports_invalid_values() { + let _lock = EnvLock::acquire(); + assert!( + take_duration_warnings().is_empty(), + "warnings buffer should start empty" + ); + let guard = EnvVarGuard::set(ENV_HTTP_ACCEPT_TIMEOUT_MS, "not-a-number"); + let duration = duration_from_env(ENV_HTTP_ACCEPT_TIMEOUT_MS, Duration::from_secs(3)); + assert_eq!(duration, Duration::from_secs(3)); + let warnings = take_duration_warnings(); + assert_eq!(warnings.len(), 1); + assert!( + warnings[0].contains(ENV_HTTP_ACCEPT_TIMEOUT_MS), + "warning should mention the variable name" + ); + assert!( + warnings[0].contains("not-a-number"), + "warning should include the invalid value" + ); + drop(guard); +} + +#[test] +fn duration_from_env_trims_whitespace() { + let _lock = EnvLock::acquire(); + assert!( + take_duration_warnings().is_empty(), + "warnings buffer should start empty" + ); + let guard = EnvVarGuard::set(ENV_HTTP_READ_TIMEOUT_MS, " 2500 "); + let duration = duration_from_env(ENV_HTTP_READ_TIMEOUT_MS, Duration::from_secs(3)); + assert_eq!(duration, Duration::from_millis(2500)); + assert!( + take_duration_warnings().is_empty(), + "whitespace-only padding should not trigger warnings", + ); + drop(guard); +} + +#[test] +fn accept_connection_respects_accept_timeout() { + let listener = TcpListener::bind(("127.0.0.1", 0)).expect("bind listener"); + listener + .set_nonblocking(true) + .expect("set listener non-blocking"); + + let accept_timeout = Duration::from_millis(20); + let poll_interval = Duration::from_millis(200); + let start = Instant::now(); + let deadline = start + accept_timeout; + + let result = panic::catch_unwind(|| { + let _ = accept_connection(&listener, deadline, poll_interval, accept_timeout); + }); + let panic_payload = result.expect_err("accept_connection should panic when no client connects"); + + let elapsed = start.elapsed(); + assert!( + elapsed >= accept_timeout, + "panic should not occur before the accept timeout (elapsed {:?}, timeout {:?})", + elapsed, + accept_timeout, + ); + assert!( + elapsed <= accept_timeout + poll_interval + Duration::from_millis(50), + "panic overshot accept timeout by more than one poll interval: elapsed={:?}, accept_timeout={:?}, poll_interval={:?}", + elapsed, + accept_timeout, + poll_interval, + ); + + let panic_ref = panic_payload.as_ref(); + let panic_text = panic_ref + .downcast_ref::() + .cloned() + .or_else(|| { + panic_ref + .downcast_ref::<&'static str>() + .map(|s| s.to_string()) + }) + .unwrap_or_else(|| format!("{panic_payload:?}")); + assert!( + panic_text.contains(&format!("accept_timeout={:?}", accept_timeout)), + "panic message should embed the accept timeout: {panic_text}", + ); + assert!( + panic_text.contains(&format!("poll_interval={:?}", poll_interval)), + "panic message should embed the poll interval: {panic_text}", + ); +} diff --git a/test_support/src/lib.rs b/test_support/src/lib.rs index 611edd4e0..7b677bd22 100644 --- a/test_support/src/lib.rs +++ b/test_support/src/lib.rs @@ -19,6 +19,9 @@ pub mod check_ninja; pub mod command_helper; pub mod cwd_guard; +/// Helpers for the `dev-fast` build-acceleration target tests: a hermetic +/// PATH/HOME sandbox, staged fake releases, a recording `cargo`, and the +/// Make invocation wrappers those tests drive. Unix-only. #[cfg(unix)] pub mod dev_fast; pub mod env; @@ -114,40 +117,95 @@ impl std::error::Error for ProbesError {} pub fn fake_ninja(exit_code: u8) -> Result<(TempDir, PathBuf)> { let dir = TempDir::new().context("fake_ninja: create temporary directory")?; - #[cfg(unix)] - let path = exec::write_exec_with_content( - dir.path(), - "ninja", - &format!("#!/bin/sh\nexit {exit_code}\n"), - ) - .context("fake_ninja: write script")?; - #[cfg(windows)] - let path = exec::write_exec_with_content( - dir.path(), - "ninja.cmd", - &format!("@echo off\r\nexit /B {exit_code}\r\n"), - ) - .context("fake_ninja: write batch file")?; - - Ok((dir, path)) + let path = { + let root = exec::utf8_path(dir.path()).context("fake_ninja: temporary directory")?; + #[cfg(unix)] + let path = + exec::write_exec_with_content(root, "ninja", &format!("#!/bin/sh\nexit {exit_code}\n")) + .context("fake_ninja: write script")?; + #[cfg(windows)] + let path = exec::write_exec_with_content( + root, + "ninja.cmd", + &format!("@echo off\r\nexit /B {exit_code}\r\n"), + ) + .context("fake_ninja: write batch file")?; + path + }; + + Ok((dir, path.into_std_path_buf())) } #[cfg(all(test, unix))] mod tests { - //! Regression coverage for the fake-executable helpers on non-UTF-8 - //! temporary directories: exercises [`super::fake_ninja`] and - //! [`super::check_ninja::fake_ninja_check_build_file`] with a temp - //! directory rooted under a path containing invalid UTF-8 bytes, - //! confirming both stubs are created on OS-native paths. + //! Coverage for the fake-executable helpers. + //! + //! Both the ordinary path and the UTF-8 boundary. On the ordinary path each + //! factory must leave an executable file behind at the path it returns — + //! callers put that path on `PATH` and expect it to run. + //! + //! At the boundary: the stub helpers take camino paths, so a temporary + //! directory whose path is not valid UTF-8 cannot be represented. Both + //! factories must surface that as a contextual error naming the offending + //! path rather than panicking or silently substituting a lossy conversion. + //! + //! The scripts are never executed here. These tests assert on what the + //! helpers wrote, and the module is `unix`-gated, so the executable-bit + //! check needs no further conditional. use super::{ EnvVarGuard, TempDir, check_ninja::fake_ninja_check_build_file, env_lock::EnvLock, fake_ninja, }; + use crate::fs; use anyhow::{Context, Result}; - use std::{ffi::OsString, fs, os::unix::ffi::OsStringExt}; + use std::{ffi::OsString, os::unix::ffi::OsStringExt}; + + /// Assert `error` names the UTF-8 boundary rather than a downstream failure. + fn assert_reports_non_utf8(error: &anyhow::Error, helper: &str) { + let rendered = format!("{error:#}"); + assert!( + rendered.contains("not valid UTF-8"), + "{helper} should report the UTF-8 boundary, got: {rendered}" + ); + } + + /// Assert the helper left an executable file at `path`. + /// + /// The caller keeps its `TempDir` alive across this call; the directory is + /// removed on drop, which would make both checks fail. + fn assert_executable_script(path: &std::path::Path, helper: &str) { + assert!( + fs::exists(path), + "{helper} should leave a script at {}", + path.display() + ); + assert!( + fs::is_executable_file(path), + "{helper} should mark {} executable", + path.display() + ); + } + + #[test] + fn fake_ninja_writes_an_executable_script() -> Result<()> { + let (dir, script) = fake_ninja(0)?; + assert_executable_script(&script, "fake_ninja"); + // Explicit, because the assertions are only meaningful while the + // temporary directory still exists. + drop(dir); + Ok(()) + } #[test] - fn fake_ninja_helpers_support_non_utf8_temp_directories() -> Result<()> { + fn fake_ninja_check_build_file_writes_an_executable_script() -> Result<()> { + let (dir, script) = fake_ninja_check_build_file()?; + assert_executable_script(&script, "fake_ninja_check_build_file"); + drop(dir); + Ok(()) + } + + #[test] + fn fake_ninja_helpers_reject_non_utf8_temp_directories() -> Result<()> { let parent = TempDir::new().context("create parent temporary directory")?; let non_utf8_root = parent.path().join(OsString::from_vec(b"tmp-\xff".to_vec())); fs::create_dir(&non_utf8_root).context("create non-UTF-8 temporary directory")?; @@ -155,16 +213,12 @@ mod tests { let _env_lock = EnvLock::acquire(); let _tmpdir = EnvVarGuard::set("TMPDIR", non_utf8_root.as_os_str()); - let (_exit_dir, exit_script) = fake_ninja(0)?; - let (_check_dir, check_script) = fake_ninja_check_build_file()?; + let exit_err = fake_ninja(0).expect_err("fake_ninja should reject a non-UTF-8 tempdir"); + assert_reports_non_utf8(&exit_err, "fake_ninja"); - assert!(exit_script.starts_with(&non_utf8_root)); - assert!(check_script.starts_with(&non_utf8_root)); - assert!(exit_script.exists(), "fake_ninja should create its script"); - assert!( - check_script.exists(), - "fake_ninja_check_build_file should create its script" - ); + let check_err = fake_ninja_check_build_file() + .expect_err("fake_ninja_check_build_file should reject a non-UTF-8 tempdir"); + assert_reports_non_utf8(&check_err, "fake_ninja_check_build_file"); Ok(()) } } diff --git a/test_support/src/localizer.rs b/test_support/src/localizer.rs index 9d57da335..87242dcfd 100644 --- a/test_support/src/localizer.rs +++ b/test_support/src/localizer.rs @@ -21,32 +21,136 @@ pub fn set_en_localizer() -> LocalizerGuard { localization::set_localizer_for_tests(Arc::from(localizer)) } -/// RAII bundle holding both the global localiser test lock and the English +/// RAII bundle holding both the global localizer test lock and the English /// locale guard for the lifetime of a test. /// /// Construct via the [`en_localizer`] rstest fixture. Both guards are /// released when this value is dropped. +/// +/// Field order is load-bearing: struct fields drop in declaration order, so +/// `_guard` must precede `_lock`. The localizer restoration in +/// `LocalizerGuard::drop` writes the global localizer, which is exactly what +/// the test lock serializes; releasing the lock first would let a waiting +/// thread install its own override and capture this test's override as its +/// "previous", so that thread would later restore the wrong value. pub struct EnLocalizer { - _lock: MutexGuard<'static, ()>, _guard: LocalizerGuard, + _lock: MutexGuard<'static, ()>, } -/// Rstest fixture that acquires the global localiser test lock and installs -/// the English localiser, returning an [`EnLocalizer`] RAII bundle. +/// Rstest fixture that acquires the global localizer test lock and installs +/// the English localizer, returning an [`EnLocalizer`] RAII bundle. /// -/// Bind the returned value immediately in each test body: +/// Bind the returned value immediately in each test body, since dropping it +/// straight away would release the localizer before assertions run. /// -/// ```rust,ignore -/// #[rstest] -/// fn my_test(en_localizer: EnLocalizer) { +/// # Examples +/// +/// `#[rstest]` expands a test taking `en_localizer: EnLocalizer` into a +/// zero-argument `#[test]` function that only `cargo test` can invoke, so it +/// is shown here for the usage pattern and defined but not called directly. +/// The shared `assert_localized` helper carries the actual assertion, and is +/// called both from the illustrated test and directly below so this example +/// still exercises real, meaningful output when run as a doctest. +/// +/// ```rust +/// use netsuke::localization::{keys::CLI_ABOUT, message}; +/// use rstest::rstest; +/// use test_support::localizer::{en_localizer, EnLocalizer}; +/// +/// fn assert_localized(en_localizer: EnLocalizer) { /// let _en_localizer = en_localizer; -/// // … assertions … +/// +/// let resolved = message(CLI_ABOUT).to_string(); +/// +/// // A resolved message differs from the raw key; a match would mean the +/// // Fluent catalogue failed to load and lookup fell back to the key. +/// assert_ne!(resolved, CLI_ABOUT); +/// } +/// +/// #[rstest] +/// fn resolves_localized_cli_about(en_localizer: EnLocalizer) { +/// assert_localized(en_localizer); /// } +/// +/// assert_localized(en_localizer()); /// ``` #[fixture] pub fn en_localizer() -> EnLocalizer { + // A poisoned lock means an earlier test panicked while holding it. The lock + // guards nothing but the ordering of localizer installation, and + // `set_en_localizer` below re-establishes the global state unconditionally, + // so recovering the guard is safe. Panicking here would instead fail every + // subsequent test that takes this fixture. `crate::env_lock` recovers from + // poisoning the same way. + let lock = localizer_test_lock().unwrap_or_else(PoisonError::into_inner); EnLocalizer { - _lock: localizer_test_lock().expect("localizer test lock poisoned"), _guard: set_en_localizer(), + _lock: lock, + } +} + +#[cfg(test)] +mod tests { + //! Coverage for the poisoned-lock recovery in the [`en_localizer`] fixture. + + use super::{LOCALIZER_TEST_LOCK, en_localizer, localizer_test_lock}; + use std::thread; + + /// Poison the lock the way a panicking test would: hold the guard across a + /// panic on another thread. + /// + /// The panic hook is deliberately left alone. `panic::set_hook` is + /// process-wide, so swapping it out for the duration would suppress — or, + /// if two threads raced on take/restore, permanently replace — the hook any + /// concurrently panicking test relies on to report itself. The stderr noise + /// from one deliberate panic is the cheaper cost. + /// + /// The whole `Result` is bound rather than unwrapped: the guard lives + /// inside either variant, so holding it across the panic poisons the mutex + /// without this helper — which Whitaker does not recognise as test code — + /// needing an `expect`. + fn poison_localizer_test_lock() { + let poisoner = thread::spawn(|| { + let _guard = localizer_test_lock(); + panic!("deliberately poisoning LOCALIZER_TEST_LOCK"); + }); + assert!( + poisoner.join().is_err(), + "the poisoning thread should have panicked" + ); + } + + #[test] + fn en_localizer_recovers_from_a_poisoned_lock() { + poison_localizer_test_lock(); + assert!( + LOCALIZER_TEST_LOCK + .get() + .is_some_and(std::sync::Mutex::is_poisoned), + "the lock should be poisoned before exercising recovery" + ); + + // The fixture must recover the guard rather than propagate the poison; + // panicking here would fail every later test that takes the fixture. + let bundle = en_localizer(); + drop(bundle); + + // Recovery does not clear the poison flag, so a second call must also + // succeed rather than depending on the first having reset it. + drop(en_localizer()); + + // Leave the static as it was found. The flag is sticky and the lock is + // shared with every other test that takes this fixture, so a deliberate + // poisoning must not outlive the test that caused it. + if let Some(lock) = LOCALIZER_TEST_LOCK.get() { + lock.clear_poison(); + } + assert!( + LOCALIZER_TEST_LOCK + .get() + .is_some_and(|lock| !lock.is_poisoned()), + "the poison flag should be cleared before leaving the test" + ); } } diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index 7f6827029..e61f71132 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -1,5 +1,6 @@ //! Helpers for constructing manifest fixtures in tests. +use crate::fs; use camino::{Utf8Path, Utf8PathBuf}; use cap_std::{ambient_authority, fs_utf8}; use std::io; @@ -39,7 +40,7 @@ pub fn manifest_yaml(body: &str) -> String { pub fn ensure_manifest_exists(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result { let manifest_path = resolve_manifest_path(temp_dir, cli_file)?; - if manifest_path.is_dir() { + if fs::is_dir(&manifest_path) { return Err(io::Error::new( io::ErrorKind::IsADirectory, format!( @@ -49,7 +50,7 @@ pub fn ensure_manifest_exists(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::R )); } - if manifest_path.exists() { + if fs::exists(&manifest_path) { return Ok(manifest_path); } @@ -120,11 +121,11 @@ fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::R } fn ensure_parent_directory(manifest_path: &Utf8Path, dest_dir: &Utf8Path) -> io::Result<()> { - if dest_dir.exists() { + if fs::exists(dest_dir) { // If the path exists but is not a directory, report a clear error that // includes the final manifest path. Returning AlreadyExists mirrors the // semantics that the desired directory “exists” but is unusable. - if dest_dir.is_dir() { + if fs::is_dir(dest_dir) { return Ok(()); } return Err(io::Error::new( @@ -139,13 +140,10 @@ fn ensure_parent_directory(manifest_path: &Utf8Path, dest_dir: &Utf8Path) -> io: let base = find_existing_ancestor(dest_dir, manifest_path)?; let relative = dest_dir.strip_prefix(base).map_err(|_| { - io::Error::new( - io::ErrorKind::Other, - format!( - "Failed to derive relative path for {} from ancestor {}", - dest_dir, base, - ), - ) + io::Error::other(format!( + "Failed to derive relative path for {} from ancestor {}", + dest_dir, base, + )) })?; let dir = fs_utf8::Dir::open_ambient_dir(base, ambient_authority()).map_err(|e| { @@ -177,7 +175,7 @@ fn find_existing_ancestor<'a>( ancestors.next(); // Skip self ancestors - .find(|candidate| candidate.exists()) + .find(|candidate| fs::exists(candidate)) .ok_or_else(|| { io::Error::new( io::ErrorKind::NotFound, @@ -191,10 +189,11 @@ fn find_existing_ancestor<'a>( #[cfg(test)] mod tests { + //! Unit tests for manifest fixture creation. + use super::*; - use anyhow::{Context, Result, anyhow}; + use anyhow::{Context, Result}; use camino::Utf8Path; - use std::fs; use std::io; use tempfile::TempDir; @@ -247,25 +246,26 @@ mod tests { let cli_file = Utf8Path::new("missing/subdir/manifest.yml"); let expected_path = temp_path.join(cli_file); assert!( - !expected_path.exists(), + !fs::exists(&expected_path), "precondition: path should not exist" ); let manifest_path = ensure_manifest_exists(temp_path, cli_file).context("create manifest when missing")?; assert_eq!(manifest_path, expected_path); - assert!(manifest_path.exists(), "manifest file should exist"); + assert!(fs::exists(&manifest_path), "manifest file should exist"); assert!( - manifest_path - .parent() - .ok_or_else(|| anyhow::anyhow!("manifest path missing parent"))? - .exists(), + fs::exists( + manifest_path + .parent() + .ok_or_else(|| anyhow::anyhow!("manifest path missing parent"))? + ), "parent directory should be created" ); // Sanity check that content was written, not an empty file. - let contents = std::fs::read_to_string(manifest_path.as_std_path()) - .context("read manifest contents")?; + let contents = + fs::read_to_string(manifest_path.as_std_path()).context("read manifest contents")?; assert!( contents.contains("netsuke_version:"), "unexpected manifest contents: {contents}" diff --git a/test_support/src/path_guard.rs b/test_support/src/path_guard.rs index 71498f78f..b62787da4 100644 --- a/test_support/src/path_guard.rs +++ b/test_support/src/path_guard.rs @@ -26,7 +26,7 @@ impl PathGuard { /// Returns a guard that restores the variable when dropped. pub fn new(original: Option) -> Self { Self { - inner: EnvGuard::with_env_and_lock("PATH", original, StdEnv::default(), true), + inner: EnvGuard::with_env_and_lock("PATH", original, StdEnv, true), } } } diff --git a/test_support/src/stdlib_assert.rs b/test_support/src/stdlib_assert.rs index 61f3bf5f1..8e88e9a0d 100644 --- a/test_support/src/stdlib_assert.rs +++ b/test_support/src/stdlib_assert.rs @@ -14,6 +14,8 @@ pub fn stdlib_output_or_error<'a>(output: Option<&'a str>, error: Option<&str>) #[cfg(test)] mod tests { + //! Unit tests for the standard library assertion helpers. + use super::*; #[test] diff --git a/tests/dev_fast_make_target_tests.rs b/tests/dev_fast_make_target_tests.rs index 800e12210..6bf2bbdd1 100644 --- a/tests/dev_fast_make_target_tests.rs +++ b/tests/dev_fast_make_target_tests.rs @@ -7,7 +7,11 @@ //! linker. A fake `cargo` records each invocation so those become checked facts //! rather than assumptions. //! -//! Every case is hermetic: no network, and no real mold, rustup, or Cargo. +//! Every case is hermetic: no network, and no real mold or rustup. The +//! exception is `cargo_resolves_the_fragment_to_the_intended_settings`, +//! which runs the real Cargo via `env!("CARGO")` because only the real +//! Cargo can confirm how it resolves the `tools/dev-fast/config.toml` +//! fragment. Every other case exercises the recording fake `cargo`. #![cfg(all(unix, target_os = "linux"))] @@ -27,51 +31,76 @@ const TEST_MOLD_VERSION: &str = "9.9.9"; struct BuildTarget { name: &'static str, subcommand: &'static [&'static str], + /// How many Cargo invocations the recipe must produce. + /// + /// Pinned per target rather than merely "at least one": `dev-test` runs the + /// root pass and then `test_support`, and dropping either would otherwise + /// leave the per-invocation checks below trivially satisfied by whichever + /// pass survived. + invocations: usize, } #[rstest] -#[case::dev_build(BuildTarget { name: "dev-build", subcommand: &["build", "--bin", "netsuke"] })] +#[case::dev_build(BuildTarget { + name: "dev-build", + subcommand: &["build", "--bin", "netsuke"], + invocations: 1, +})] #[case::dev_test( BuildTarget { name: "dev-test", // Mirrors `make test-nextest`, so the accelerated loop and the gate run // the same runner under the same `.config/nextest.toml`. subcommand: &["nextest", "run", "--all-targets", "--all-features"], + invocations: 2, } )] fn build_targets_select_the_pinned_toolchain_and_fragment( #[case] target: BuildTarget, ) -> Result<()> { let scenario = BuildScenario::prepare()?; - let invocation = scenario.run(target.name)?; - - ensure!( - invocation.toolchain() == pinned_toolchain()?, - "`{}` should select the pinned nightly, got `{}`", - target.name, - invocation.toolchain() - ); - ensure!( - invocation.contains_sequence(&["--config", DEV_FAST_CONFIG_PATH]), - "`{}` should pass the fragment, got `{:?}`", - target.name, - invocation.arguments() - ); - ensure!( - invocation.contains_sequence(target.subcommand), - "`{}` should run `{:?}`, got `{:?}`", - target.name, - target.subcommand, - invocation.arguments() - ); - // The linker is resolved by PATH order, so leading the prefix is the whole - // mechanism by which the pinned mold, and not a system one, gets used. + let invocations = scenario.run_all(target.name)?; ensure!( - invocation.path_starts_with(&scenario.prefix_bin()), - "`{}` should lead PATH with the install prefix, got `{}`", + invocations.len() == target.invocations, + "`{}` should invoke cargo {} time(s), recorded {}", target.name, - invocation.path() + target.invocations, + invocations.len() ); + + // Every invocation, not just the first: `dev-test` runs the root pass and + // then `test_support`, and the toolchain, fragment, and PATH contract has + // to hold for both. + for invocation in invocations { + ensure!( + invocation.toolchain() == pinned_toolchain()?, + "`{}` should select the pinned nightly, got `{}`", + target.name, + invocation.toolchain() + ); + ensure!( + invocation.contains_sequence(&["--config", DEV_FAST_CONFIG_PATH]), + "`{}` should pass the fragment, got `{:?}`", + target.name, + invocation.arguments() + ); + ensure!( + invocation.contains_sequence(target.subcommand), + "`{}` should run `{:?}`, got `{:?}`", + target.name, + target.subcommand, + invocation.arguments() + ); + // The linker is resolved by PATH order, so leading the prefix is the + // whole mechanism by which the pinned mold, and not a system one, gets + // used. + ensure!( + invocation.path_starts_with(&scenario.prefix_bin()), + "`{}` should lead PATH with the install prefix, got `{}`", + target.name, + invocation.path() + ); + } Ok(()) } @@ -169,8 +198,10 @@ fn a_drifting_mold_invokes_cargo_not_at_all(#[case] target: &str) -> Result<()> #[case::unstable_flag("unstable.codegen-backend", "unstable.codegen-backend = true")] #[case::linux_rustflags( "target", - "target.'cfg(target_os = \"linux\")'.rustflags = \ - [\"-Zpolonius=next\", \"-Clink-arg=-fuse-ld=mold\"]" + concat!( + "target.'cfg(target_os = \"linux\")'.rustflags = ", + "[\"-Zpolonius=next\", \"-Clink-arg=-fuse-ld=mold\"]", + ) )] fn cargo_resolves_the_fragment_to_the_intended_settings( #[case] query: &str, diff --git a/tests/documentation_examples_e2e_tests.rs b/tests/documentation_examples_e2e_tests.rs index 14a8a531a..f4f4af4f9 100644 --- a/tests/documentation_examples_e2e_tests.rs +++ b/tests/documentation_examples_e2e_tests.rs @@ -22,7 +22,7 @@ fn executable_path(stub_directory: &Utf8Path) -> Result { } fn write_stub(directory: &Utf8Path, name: &str, script: &str) -> Result<()> { - write_exec_with_content(directory.as_std_path(), name, script) + write_exec_with_content(directory, name, script) .with_context(|| format!("write {name} stub"))?; Ok(()) } @@ -358,7 +358,7 @@ fn stdlib_host_context_example_uses_controlled_process_state() -> Result<()> { "done\n" ), )?; - write_exec(stub_directory.as_std_path(), "guide-tool")?; + write_exec(&stub_directory, "guide-tool")?; let path = executable_path(&stub_directory)?; let run = run_netsuke_in_with_env( workspace.path(), diff --git a/tests/localization_tests.rs b/tests/localization_tests.rs index 0835aeb9d..c97ed6fac 100644 --- a/tests/localization_tests.rs +++ b/tests/localization_tests.rs @@ -14,11 +14,21 @@ use test_support::fluent::normalize_fluent_isolates; /// /// The test lock ensures localization tests run serially, and the localizer /// guard restores the previous localizer when dropped. +/// +/// Both fields are underscore-prefixed because they exist only to be dropped: +/// nothing reads them, and the prefix states that without suppressing the +/// `dead_code` lint. `test_support::localizer::EnLocalizer` is named the same +/// way for the same reason. +/// +/// Field order is load-bearing: struct fields drop in declaration order, so +/// `_localizer` must precede `_lock`. `LocalizerGuard::drop` writes the global +/// localizer, which is what `_lock` serializes; releasing the lock first would +/// let a waiting test install its own override and capture this test's +/// override as its "previous", so that test would later restore the wrong +/// value. struct LocalizerTestGuards { - #[expect(dead_code, reason = "Held for lifetime, not accessed directly")] - lock: MutexGuard<'static, ()>, - #[expect(dead_code, reason = "Held for lifetime, not accessed directly")] - localizer: LocalizerGuard, + _localizer: LocalizerGuard, + _lock: MutexGuard<'static, ()>, } /// Create localizer guards for a given locale. @@ -32,8 +42,8 @@ fn localizer_guards(locale: &str) -> Result { let localizer = cli_localization::build_localizer(Some(locale)); let guard = localization::set_localizer_for_tests(Arc::from(localizer)); Ok(LocalizerTestGuards { - lock, - localizer: guard, + _localizer: guard, + _lock: lock, }) } diff --git a/tests/makefile_test_target.rs b/tests/makefile_test_target.rs index ce8029ec5..a644340ae 100644 --- a/tests/makefile_test_target.rs +++ b/tests/makefile_test_target.rs @@ -23,98 +23,17 @@ //! valid when the command a recipe runs changes. A guard test fails if a //! recipe starts setting `RUSTFLAGS` without joining the covered set. +#[path = "support/makefile.rs"] +mod makefile; + use anyhow::{Context, Result, ensure}; use camino::Utf8Path; -use cap_std::{ambient_authority, fs_utf8::Dir}; +use makefile::{read_repo_file, target_prerequisites, target_recipe}; use rstest::rstest; use std::collections::BTreeSet; use std::process::Command; use toml::Value; -/// Opens the repository root as a capability-scoped directory handle. -/// -/// Every read in this file is relative to that handle, so the tests cannot -/// reach outside the checkout. -fn repo_root() -> Result { - Dir::open_ambient_dir(env!("CARGO_MANIFEST_DIR"), ambient_authority()) - .context("open the repository root as a capability-scoped directory") -} - -fn read_repo_file(relative: &Utf8Path) -> Result { - repo_root()? - .read_to_string(relative) - .with_context(|| format!("{relative} should be readable")) -} - -/// Splits a Make rule line into its target and its prerequisites. -/// -/// Trailing `## ` help comments are discarded so `help` annotations do not leak -/// into the prerequisite list. -fn parse_rule(line: &str) -> Option<(&str, Vec<&str>)> { - if line.starts_with(['\t', ' ', '#', '.']) { - return None; - } - let (target, rest) = line.split_once(':')?; - if target.is_empty() || rest.starts_with('=') { - return None; - } - let prerequisites = rest - .split("##") - .next() - .unwrap_or_default() - .split_whitespace() - .collect(); - Some((target.trim(), prerequisites)) -} - -/// Returns the prerequisites declared for `target`. -fn target_prerequisites(contents: &str, target: &str) -> Option> { - contents.lines().find_map(|line| { - let (name, prerequisites) = parse_rule(line)?; - (name == target).then(|| prerequisites.into_iter().map(ToOwned::to_owned).collect()) - }) -} - -/// Returns the tab-indented recipe lines for `target`, joined by newlines. -fn target_recipe(contents: &str, target: &str) -> Option { - let mut lines = contents - .lines() - .skip_while(|line| parse_rule(line).is_none_or(|(name, _)| name != target)); - lines.next()?; - let recipe: Vec<&str> = lines - .take_while(|line| line.starts_with('\t') || line.trim().is_empty()) - .filter(|line| line.starts_with('\t')) - .collect(); - Some(recipe.join("\n")) -} - -#[test] -fn unit_parses_make_rules_and_ignores_help_comments() { - assert_eq!( - parse_rule("test: test-nextest doctest ## Run every Rust test"), - Some(("test", vec!["test-nextest", "doctest"])) - ); - assert_eq!( - parse_rule("doctest: ## Run doctests"), - Some(("doctest", vec![])) - ); - assert_eq!(parse_rule("BUILD_JOBS ?="), None); - assert_eq!(parse_rule("\tcargo nextest run"), None); - assert_eq!(parse_rule(".PHONY: test doctest"), None); -} - -#[test] -fn unit_extracts_recipe_lines_for_a_target() { - let makefile = "test: doctest ## composite\n\ndoctest: ## docs\n\tcargo test --doc\n\techo done\n\nother:\n\ttrue\n"; - - assert_eq!(target_recipe(makefile, "test").as_deref(), Some("")); - assert_eq!( - target_recipe(makefile, "doctest").as_deref(), - Some("\tcargo test --doc\n\techo done") - ); - assert_eq!(target_recipe(makefile, "missing"), None); -} - #[test] fn behavioural_make_test_composes_the_nextest_and_doctest_passes() -> Result<()> { let makefile = read_repo_file(Utf8Path::new("Makefile"))?; @@ -166,6 +85,194 @@ fn behavioural_make_test_composes_the_nextest_and_doctest_passes() -> Result<()> Ok(()) } +/// `test_support` is excluded from the root workspace, so a root-level cargo +/// invocation cannot reach its tests. Each pass must therefore target its +/// manifest explicitly; dropping one of those lines silently un-gates that +/// crate, which is invisible in a green test run. `dev-test` is covered too: +/// the guide calls it the accelerated counterpart of `test-nextest`, so it +/// carries the same obligation. +/// +/// Where such a line sets `RUSTFLAGS` — the `test-nextest` and `doctest` +/// passes, but not `dev-test`, which selects its toolchain instead — the value +/// is asserted by `RUSTFLAGS_CASES`, which expands each assignment rather than +/// matching recipe text. +#[rstest] +#[case::nextest("test-nextest", "nextest run")] +#[case::doctest("doctest", "--doc")] +#[case::dev_test("dev-test", "nextest run")] +fn behavioural_test_passes_also_target_test_support( + #[case] target: &str, + #[case] harness: &str, +) -> Result<()> { + let makefile = read_repo_file(Utf8Path::new("Makefile"))?; + let recipe = target_recipe(&makefile, target) + .with_context(|| format!("Makefile should declare a {target} target"))?; + + let scoped: Vec<&str> = recipe + .lines() + .filter(|line| line.contains(QUOTED_MANIFEST_FLAG)) + .collect(); + ensure!( + scoped.len() == 1, + concat!( + "{target} should invoke the harness once against ", + "$(TEST_SUPPORT_MANIFEST), found {count}: {recipe:?}", + ), + target = target, + count = scoped.len(), + recipe = recipe + ); + for line in &scoped { + ensure!( + line.contains(harness), + "{target}'s test_support pass should use {harness}, found {line:?}" + ); + ensure!( + line.contains("--all-features"), + "{target}'s test_support pass should enable all features, found {line:?}" + ); + // Checked per line rather than over the whole recipe: the root pass + // carries the same flag, so a recipe-wide `contains` stays satisfied + // even after the scoped pass loses it. `cargo test --doc` takes no + // `--all-targets`, so this applies to the nextest case alone. + if harness == "nextest run" { + ensure!( + line.contains("--all-targets"), + concat!( + "{target}'s test_support pass should cover every test ", + "target, found {line:?}", + ), + target = target, + line = line + ); + } + } + Ok(()) +} + +/// Every recipe reaching `test_support` must quote the manifest variable. +/// +/// `TEST_SUPPORT_MANIFEST` is overridable, so an unquoted expansion +/// word-splits on a path containing spaces and hands Cargo a truncated +/// manifest path. This reads the Makefile only — it neither runs Make nor +/// invokes Cargo — so it stays a fast static check. +#[test] +fn behavioural_test_support_passes_quote_the_manifest_variable() -> Result<()> { + let makefile = read_repo_file(Utf8Path::new("Makefile"))?; + let unquoted = "--manifest-path $(TEST_SUPPORT_MANIFEST)"; + + let offenders: Vec<&str> = makefile + .lines() + .filter(|line| line.contains(unquoted) && !line.contains(QUOTED_MANIFEST_FLAG)) + .map(str::trim) + .collect(); + ensure!( + offenders.is_empty(), + "every --manifest-path must quote the variable, found {offenders:#?}" + ); + + for target in ["test-nextest", "doctest", "dev-test", "lint-clippy"] { + let recipe = target_recipe(&makefile, target) + .with_context(|| format!("Makefile should declare a {target} target"))?; + let quoted = recipe + .lines() + .filter(|line| line.contains(QUOTED_MANIFEST_FLAG)) + .count(); + ensure!( + quoted == 1, + concat!( + "{target} should reach test_support on exactly one quoted ", + "--manifest-path line, found {quoted}: {recipe:?}", + ), + target = target, + quoted = quoted, + recipe = recipe + ); + } + Ok(()) +} + +/// Splits `fragment` with `sh` and returns the resulting arguments. +/// +/// The sibling `expand` helper refuses an expression containing `"`, which is +/// exactly the character under test here, so this splits rather than expands. +/// Only `sh` is involved: nothing runs Make, Cargo, or any other tool. +#[cfg(unix)] +fn shell_arguments(fragment: &str) -> Result> { + ensure!( + !fragment.contains('\'') && !fragment.contains('`') && !fragment.contains("$("), + "the splitting helper cannot safely embed {fragment:?}" + ); + let script = format!("set -- {fragment}\nfor arg in \"$@\"; do printf '%s\\n' \"$arg\"; done"); + let output = Command::new("sh") + .arg("-c") + .arg(&script) + .output() + .with_context(|| format!("split {fragment:?} with sh"))?; + ensure!( + output.status.success(), + "sh should split {fragment:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).context("split arguments should be UTF-8")?; + Ok(stdout.lines().map(ToOwned::to_owned).collect()) +} + +/// A quoted override survives word-splitting; an unquoted one does not. +/// +/// `TEST_SUPPORT_MANIFEST` is overridable, so a caller may point it at a path +/// containing spaces. Make expands the variable itself and hands the resulting +/// text to the shell, so the quotes in the recipe are what keep that path one +/// argument. `behavioural_test_support_passes_quote_the_manifest_variable` +/// pins that the Makefile really does use `QUOTED_MANIFEST_FLAG`; this checks +/// what that spelling buys. +/// +/// The unquoted case is asserted too, so the test fails if the shell stops +/// discriminating rather than silently passing for the wrong reason. +#[cfg(unix)] +#[test] +fn behavioural_a_quoted_manifest_override_stays_one_argument() -> Result<()> { + let spacey = "/tmp/netsuke contract/Cargo.toml"; + + let quoted = QUOTED_MANIFEST_FLAG.replace("$(TEST_SUPPORT_MANIFEST)", spacey); + let arguments = shell_arguments("ed)?; + ensure!( + arguments == ["--manifest-path".to_owned(), spacey.to_owned()], + "the quoted flag should split into two arguments, got {arguments:?}" + ); + + // The control. Without it a shell that stopped splitting at all would let + // the assertion above pass while proving nothing about the quotes. + let unquoted = quoted.replace('"', ""); + let split = shell_arguments(&unquoted)?; + ensure!( + split.len() > 2, + "an unquoted override should word-split, got {split:?}" + ); + Ok(()) +} + +/// The manifest path is a `?=` variable so a caller can point the second pass +/// at a relocated crate without editing the recipes. +#[test] +fn behavioural_test_support_manifest_is_overridable() -> Result<()> { + let makefile = read_repo_file(Utf8Path::new("Makefile"))?; + ensure!( + makefile + .lines() + .any(|line| line.trim() == "TEST_SUPPORT_MANIFEST ?= test_support/Cargo.toml"), + "the Makefile should default TEST_SUPPORT_MANIFEST overridably" + ); + Ok(()) +} + +/// The manifest flag every `test_support` pass must carry, quoted. +/// +/// The quotes are the contract, not incidental formatting: `TEST_SUPPORT_MANIFEST` +/// is overridable, so an unquoted expansion word-splits on a path containing +/// spaces and hands Cargo a truncated manifest path. +const QUOTED_MANIFEST_FLAG: &str = "--manifest-path \"$(TEST_SUPPORT_MANIFEST)\""; + /// The prefix introducing a quoted `RUSTFLAGS` assignment in a recipe. const RUSTFLAGS_PREFIX: &str = "RUSTFLAGS=\""; @@ -239,6 +346,17 @@ impl RustflagsCase { } } + /// Clippy's `test_support` pass. The marker must be the manifest flag: + /// `clippy` alone matches the root line, which `recipe_line` finds first. + const fn lint_clippy_test_support() -> Self { + Self { + target: "lint-clippy", + line_marker: QUOTED_MANIFEST_FLAG, + denies_warnings: true, + separator_only_when_set: true, + } + } + const fn lint_whitaker() -> Self { Self { target: "lint-whitaker", @@ -248,6 +366,40 @@ impl RustflagsCase { } } + // `test_support` is excluded from the root workspace, so `test-nextest`, + // `doctest`, and `lint-whitaker` each run a second time against its + // manifest. Those lines set `RUSTFLAGS` too and hold the same contract as + // their root counterparts. Their markers must select the scoped line + // rather than the root one, which `recipe_line` would otherwise find + // first: the root lines match `nextest run`, `--doc`, and `$(WHITAKER)` + // as well. + const fn test_nextest_test_support() -> Self { + Self { + target: "test-nextest", + line_marker: QUOTED_MANIFEST_FLAG, + denies_warnings: true, + separator_only_when_set: true, + } + } + + const fn doctest_test_support() -> Self { + Self { + target: "doctest", + line_marker: QUOTED_MANIFEST_FLAG, + denies_warnings: true, + separator_only_when_set: true, + } + } + + const fn lint_whitaker_test_support() -> Self { + Self { + target: "lint-whitaker", + line_marker: "cd test_support", + denies_warnings: true, + separator_only_when_set: true, + } + } + const fn typecheck() -> Self { Self { target: "typecheck", @@ -268,13 +420,17 @@ impl RustflagsCase { } /// Every `RUSTFLAGS`-setting recipe line under contract. -const RUSTFLAGS_CASES: [RustflagsCase; 8] = [ +const RUSTFLAGS_CASES: [RustflagsCase; 12] = [ RustflagsCase::test_nextest(), + RustflagsCase::test_nextest_test_support(), RustflagsCase::doctest(), + RustflagsCase::doctest_test_support(), RustflagsCase::binary_build(), RustflagsCase::lint_clippy_rustdoc(), RustflagsCase::lint_clippy(), + RustflagsCase::lint_clippy_test_support(), RustflagsCase::lint_whitaker(), + RustflagsCase::lint_whitaker_test_support(), RustflagsCase::typecheck(), RustflagsCase::kani_full(), ]; @@ -388,11 +544,15 @@ fn unit_extracts_the_rustflags_assignment_from_a_recipe_line() { #[cfg(unix)] #[rstest] #[case(RustflagsCase::test_nextest())] +#[case(RustflagsCase::test_nextest_test_support())] #[case(RustflagsCase::doctest())] +#[case(RustflagsCase::doctest_test_support())] #[case(RustflagsCase::binary_build())] #[case(RustflagsCase::lint_clippy_rustdoc())] #[case(RustflagsCase::lint_clippy())] +#[case(RustflagsCase::lint_clippy_test_support())] #[case(RustflagsCase::lint_whitaker())] +#[case(RustflagsCase::lint_whitaker_test_support())] #[case(RustflagsCase::typecheck())] #[case(RustflagsCase::kani_full())] fn behavioural_rustflags_recipes_preserve_inherited_flags( @@ -410,9 +570,13 @@ fn behavioural_rustflags_recipes_preserve_inherited_flags( ); ensure!( expanded.contains(&polonius), - "{} should re-state {polonius} because setting RUSTFLAGS overrides \ - .cargo/config.toml, expanded to {expanded:?}", - case.target + concat!( + "{} should re-state {polonius} because setting RUSTFLAGS ", + "overrides .cargo/config.toml, expanded to {expanded:?}", + ), + case.target, + polonius = polonius, + expanded = expanded ); ensure!( expanded.contains(DENY_WARNINGS) == case.denies_warnings, @@ -426,11 +590,15 @@ fn behavioural_rustflags_recipes_preserve_inherited_flags( #[cfg(unix)] #[rstest] #[case(RustflagsCase::test_nextest())] +#[case(RustflagsCase::test_nextest_test_support())] #[case(RustflagsCase::doctest())] +#[case(RustflagsCase::doctest_test_support())] #[case(RustflagsCase::binary_build())] #[case(RustflagsCase::lint_clippy_rustdoc())] #[case(RustflagsCase::lint_clippy())] +#[case(RustflagsCase::lint_clippy_test_support())] #[case(RustflagsCase::lint_whitaker())] +#[case(RustflagsCase::lint_whitaker_test_support())] #[case(RustflagsCase::typecheck())] #[case(RustflagsCase::kani_full())] fn behavioural_rustflags_recipes_are_well_formed_without_inherited_flags( @@ -444,9 +612,13 @@ fn behavioural_rustflags_recipes_are_well_formed_without_inherited_flags( ensure!( expanded.contains(&polonius), - "{} should re-state {polonius} even with no inherited RUSTFLAGS, \ - expanded to {expanded:?}", - case.target + concat!( + "{} should re-state {polonius} even with no inherited RUSTFLAGS, ", + "expanded to {expanded:?}", + ), + case.target, + polonius = polonius, + expanded = expanded ); ensure!( !expanded.contains(CALLER_RUSTFLAGS), @@ -461,9 +633,12 @@ fn behavioural_rustflags_recipes_are_well_formed_without_inherited_flags( if case.separator_only_when_set { ensure!( !expanded.starts_with(' '), - "{} should not emit a leading separator when RUSTFLAGS is unset, \ - expanded to {expanded:?}", - case.target + concat!( + "{} should not emit a leading separator when RUSTFLAGS is ", + "unset, expanded to {expanded:?}", + ), + case.target, + expanded = expanded ); } Ok(()) diff --git a/tests/polonius_toolchain_contract.rs b/tests/polonius_toolchain_contract.rs index 15967feec..b351ef579 100644 --- a/tests/polonius_toolchain_contract.rs +++ b/tests/polonius_toolchain_contract.rs @@ -8,9 +8,12 @@ //! the flag. These tests fail when any layer drops the pin or the flag, so //! a regression cannot reach CI as a confusing borrow-check error. +#[path = "support/makefile.rs"] +mod makefile; + use anyhow::{Context, Result, ensure}; use camino::Utf8Path; -use cap_std::{ambient_authority, fs_utf8::Dir}; +use makefile::{read_repo_file, target_recipe}; use rstest::rstest; use serde_yaml::Value as YamlValue; use toml::Value as TomlValue; @@ -18,18 +21,6 @@ use toml::Value as TomlValue; const POLONIUS_FLAG: &str = "-Zpolonius=next"; const POLONIUS_VAR: &str = "$(POLONIUS_FLAGS)"; -/// Opens the repository root as a capability-scoped directory handle. -fn repo_root() -> Result { - Dir::open_ambient_dir(env!("CARGO_MANIFEST_DIR"), ambient_authority()) - .context("open the repository root as a capability-scoped directory") -} - -fn read_repo_file(relative: &Utf8Path) -> Result { - repo_root()? - .read_to_string(relative) - .with_context(|| format!("{relative} should be readable")) -} - /// Returns the dated nightly channel pinned in `rust-toolchain.toml`. /// /// The workflow assertions compare against this value so a future pin move @@ -46,22 +37,6 @@ fn pinned_toolchain() -> Result { Ok(channel.to_owned()) } -/// Returns the tab-indented recipe lines for `target`, joined by newlines. -fn target_recipe(contents: &str, target: &str) -> Option { - let mut lines = contents.lines().skip_while(|line| { - line.starts_with(['\t', ' ', '#', '.']) - || line - .split_once(':') - .is_none_or(|(name, rest)| name.trim() != target || rest.starts_with('=')) - }); - lines.next()?; - let recipe: Vec<&str> = lines - .take_while(|line| line.starts_with('\t') || line.trim().is_empty()) - .filter(|line| line.starts_with('\t')) - .collect(); - Some(recipe.join("\n")) -} - /// Walks nested YAML mappings and returns the string at the key path. fn yaml_str<'a>(value: &'a YamlValue, keys: &[&str]) -> Option<&'a str> { let mut current = value; diff --git a/tests/std_filter_tests/support.rs b/tests/std_filter_tests/support.rs index 293696ed4..3dc95a55c 100644 --- a/tests/std_filter_tests/support.rs +++ b/tests/std_filter_tests/support.rs @@ -15,6 +15,13 @@ pub(crate) use test_support::{EnvVarGuard, env_lock::EnvLock}; pub(crate) type Workspace = (tempfile::TempDir, Utf8PathBuf); pub(crate) mod fallible { + //! Fallible variants of the `support` helpers, returning `anyhow::Result` + //! instead of panicking on setup failure. + //! + //! The parent module re-exports these functions so most call sites reach + //! them via `support::*`; keeping them namespaced here separates the + //! error-propagating implementations from the workspace types they + //! operate on. use super::{Workspace, stdlib}; use anyhow::{anyhow, Context, Result}; use camino::Utf8PathBuf; diff --git a/tests/support/makefile.rs b/tests/support/makefile.rs new file mode 100644 index 000000000..f4ad62162 --- /dev/null +++ b/tests/support/makefile.rs @@ -0,0 +1,255 @@ +//! Shared helpers for static Makefile contract tests. +//! +//! Scope, deliberately narrow: reading a file from the repository root through +//! a capability-scoped handle, and parsing a Make rule into its prerequisites +//! and recipe. Nothing here runs Make, runs Cargo, or writes anything. +//! +//! Reuse policy: include this module from a `tests/*.rs` binary that asserts on +//! Makefile *text*. Do not grow it into a general test-utility bag — fixture +//! construction, process invocation, and environment control belong in the +//! `test_support` crate, which is versioned, linted, and documented as such. +//! A helper earns a place here only when more than one contract test needs the +//! same reading or parsing behaviour. +//! +//! Integration tests under `tests/` compile as independent crates, so there is +//! no library to share through. The module lives in a subdirectory, which Cargo +//! does not auto-discover as a test target, and each consumer includes it with +//! `#[path = "support/makefile.rs"] mod makefile;`. +//! +//! Every helper is exercised by this module's own unit tests, which run once +//! per including crate. That is what keeps a consumer using only part of the +//! surface from tripping `dead_code`. + +use anyhow::{Context, Result}; +use camino::Utf8Path; +use cap_std::{ambient_authority, fs_utf8::Dir}; + +/// Opens the repository root as a capability-scoped directory handle. +/// +/// Every read goes through this handle, so a contract test cannot reach +/// outside the checkout. +/// +/// # Errors +/// +/// Returns an error when the manifest directory cannot be opened. +/// +/// # Examples +/// +/// ```no_run +/// let root = repo_root().expect("open the repository root"); +/// let makefile = root.read_to_string("Makefile").expect("read the Makefile"); +/// assert!(makefile.contains("test-nextest:")); +/// ``` +pub fn repo_root() -> Result { + Dir::open_ambient_dir(env!("CARGO_MANIFEST_DIR"), ambient_authority()) + .context("open the repository root as a capability-scoped directory") +} + +/// Reads `relative` from the repository root as a UTF-8 string. +/// +/// # Errors +/// +/// Returns an error naming `relative` when it cannot be read. +/// +/// # Examples +/// +/// ```no_run +/// use camino::Utf8Path; +/// +/// let makefile = read_repo_file(Utf8Path::new("Makefile")).expect("read the Makefile"); +/// assert!(makefile.contains("test-nextest:")); +/// ``` +pub fn read_repo_file(relative: &Utf8Path) -> Result { + repo_root()? + .read_to_string(relative) + .with_context(|| format!("{relative} should be readable")) +} + +/// Splits a Make rule line into its target and its prerequisites. +/// +/// Returns `None` for anything that is not a rule header: recipe and +/// continuation lines (leading tab or space), comments, directives such as +/// `.PHONY`, and variable assignments, which `rest.starts_with('=')` +/// distinguishes from a rule by catching the `:=` form. +/// +/// Trailing `## ` help comments are discarded so `help` annotations do not leak +/// into the prerequisite list. +/// +/// # Examples +/// +/// ``` +/// let (target, prerequisites) = +/// parse_rule("alpha: beta gamma ## build everything").expect("a rule header"); +/// assert_eq!(target, "alpha"); +/// assert_eq!(prerequisites, ["beta", "gamma"]); +/// +/// // A recipe line is not a rule header. +/// assert_eq!(parse_rule("\techo one"), None); +/// ``` +pub fn parse_rule(line: &str) -> Option<(&str, Vec<&str>)> { + if line.starts_with(['\t', ' ', '#', '.']) { + return None; + } + let (target, rest) = line.split_once(':')?; + if target.is_empty() || rest.starts_with('=') { + return None; + } + let prerequisites = rest + .split("##") + .next() + .unwrap_or_default() + .split_whitespace() + .collect(); + Some((target.trim(), prerequisites)) +} + +/// Returns the prerequisites declared for `target`. +/// +/// Yields `None` when `contents` declares no such target. +/// +/// # Examples +/// +/// ``` +/// let makefile = "alpha: beta gamma\n\techo one\n"; +/// assert_eq!( +/// target_prerequisites(makefile, "alpha"), +/// Some(vec!["beta".to_owned(), "gamma".to_owned()]) +/// ); +/// ``` +pub fn target_prerequisites(contents: &str, target: &str) -> Option> { + contents.lines().find_map(|line| { + let (name, prerequisites) = parse_rule(line)?; + (name == target).then(|| prerequisites.into_iter().map(ToOwned::to_owned).collect()) + }) +} + +/// Returns the tab-indented recipe lines for `target`, joined by newlines. +/// +/// A target with no recipe yields an empty string; an absent target yields +/// `None`. Blank lines inside a recipe are traversed but dropped, so a recipe +/// separated by a blank line is returned whole. +/// +/// # Examples +/// +/// ``` +/// let makefile = "alpha: beta\n\techo one\n\n\techo two\n\nbeta:\n"; +/// assert_eq!( +/// target_recipe(makefile, "alpha").as_deref(), +/// Some("\techo one\n\techo two") +/// ); +/// ``` +pub fn target_recipe(contents: &str, target: &str) -> Option { + let mut lines = contents + .lines() + .skip_while(|line| parse_rule(line).is_none_or(|(name, _)| name != target)); + lines.next()?; + let recipe: Vec<&str> = lines + .take_while(|line| line.starts_with('\t') || line.trim().is_empty()) + .filter(|line| line.starts_with('\t')) + .collect(); + Some(recipe.join("\n")) +} + +#[cfg(test)] +mod tests { + //! Edge cases the shared parser must support. + //! + //! These also keep every helper used from each including crate, so a + //! consumer needing only part of the surface does not trip `dead_code`. + + use super::{parse_rule, read_repo_file, repo_root, target_prerequisites, target_recipe}; + use camino::Utf8Path; + + const SAMPLE: &str = concat!( + "# a comment\n", + ".PHONY: alpha beta\n", + "BUILD_JOBS ?=\n", + "VAR := value\n", + "alpha: beta gamma ## help text\n", + "\techo one\n", + "\n", + "\techo two\n", + "\n", + "beta: ## no recipe\n", + "\n", + "gamma:\n", + "\techo three\n", + ); + + #[test] + fn parse_rule_accepts_a_rule_header_and_strips_help_comments() { + assert_eq!( + parse_rule("alpha: beta gamma ## help text"), + Some(("alpha", vec!["beta", "gamma"])) + ); + assert_eq!(parse_rule("gamma:"), Some(("gamma", vec![]))); + } + + #[test] + fn parse_rule_rejects_everything_that_is_not_a_rule_header() { + assert_eq!(parse_rule("\techo one"), None, "recipe line"); + assert_eq!(parse_rule(" continued"), None, "continuation line"); + assert_eq!(parse_rule("# a comment"), None, "comment"); + assert_eq!(parse_rule(".PHONY: alpha"), None, "directive"); + assert_eq!(parse_rule("VAR := value"), None, "assignment"); + assert_eq!(parse_rule("BUILD_JOBS ?="), None, "no colon"); + assert_eq!(parse_rule(": orphan"), None, "empty target"); + assert_eq!(parse_rule(""), None, "blank line"); + } + + #[test] + fn target_prerequisites_reports_declared_dependencies() { + assert_eq!( + target_prerequisites(SAMPLE, "alpha"), + Some(vec!["beta".to_owned(), "gamma".to_owned()]) + ); + assert_eq!(target_prerequisites(SAMPLE, "gamma"), Some(vec![])); + assert_eq!(target_prerequisites(SAMPLE, "missing"), None); + } + + #[test] + fn target_recipe_spans_blank_lines_and_stops_at_the_next_rule() { + assert_eq!( + target_recipe(SAMPLE, "alpha").as_deref(), + Some("\techo one\n\techo two"), + "a blank line inside a recipe should not truncate it" + ); + assert_eq!( + target_recipe(SAMPLE, "gamma").as_deref(), + Some("\techo three") + ); + } + + #[test] + fn target_recipe_distinguishes_an_empty_recipe_from_an_absent_target() { + assert_eq!( + target_recipe(SAMPLE, "beta").as_deref(), + Some(""), + "a target with no recipe yields an empty string" + ); + assert_eq!(target_recipe(SAMPLE, "missing"), None); + } + + #[test] + fn target_recipe_ignores_a_variable_whose_name_matches_the_target() { + let makefile = "VAR := not a rule\nVAR:\n\techo real\n"; + assert_eq!( + target_recipe(makefile, "VAR").as_deref(), + Some("\techo real") + ); + } + + #[test] + fn repo_root_reads_a_known_repository_file() -> anyhow::Result<()> { + repo_root()?; + let makefile = read_repo_file(Utf8Path::new("Makefile"))?; + // `ensure!` rather than `assert!`: the workspace denies + // `clippy::panic_in_result_fn`, so a Result-returning test reports a + // failure by returning it. + anyhow::ensure!( + makefile.contains("test-nextest:"), + "the Makefile should declare test-nextest" + ); + Ok(()) + } +} diff --git a/tests/whitaker_boundary_contract.rs b/tests/whitaker_boundary_contract.rs new file mode 100644 index 000000000..5b8921c4b --- /dev/null +++ b/tests/whitaker_boundary_contract.rs @@ -0,0 +1,344 @@ +//! Contract tests pinning the `no_std_fs_operations` exclusion boundary. +//! +//! The capability policy is enforced by configuration rather than by code, so +//! a regression here is silent: widening an entry from a module path to a whole +//! crate, or dropping the second Whitaker run, stops the lint reporting real +//! violations without failing anything. These tests pin the two invariants that +//! keep the boundary honest. +//! +//! First, `test_support` is excluded from the Cargo workspace, so a +//! workspace-root `cargo dylint` cannot reach it. `make lint-whitaker` +//! therefore runs the suite a second time from `test_support/`, against that +//! crate's own `dylint.toml`. Without the second invocation the crate is +//! unlinted while still appearing to be covered. +//! +//! Second, every exemption must name a bounded module. A bare crate name in +//! `excluded_paths`, or the application crate reappearing in `excluded_crates`, +//! would exempt far more than the ambient boundary it was added for. +//! +//! Third, neither manifest may pin the lint-library source. Installing the +//! libraries at Whitaker HEAD is a decision, not an oversight in the installer +//! pin, and it has been proposed as a defect more than once. A +//! `[workspace.metadata.dylint]` block would quietly reverse it, so the absence +//! is asserted here rather than left to review. +//! +//! These assertions are deterministic file checks. They do not invoke Whitaker: +//! the suite needs its own pinned toolchain and driver, which `make test` does +//! not require. `make lint-whitaker` remains the gate that actually runs it, +//! and `docs/developers-guide.md` records the manual negative probe for +//! confirming the exclusions have not widened. + +#[path = "support/makefile.rs"] +mod makefile; + +use anyhow::{Context, Result, ensure}; +use camino::Utf8Path; +use makefile::{read_repo_file, target_recipe}; +use rstest::rstest; +use toml::Value as TomlValue; + +/// The one module in `test_support` permitted to touch ambient `std::fs`. +const TEST_SUPPORT_BOUNDARY: &str = "test_support::fs"; + +/// PATH resolution for the `which` standard library function, whose candidate +/// directories come from the ambient `PATH`. +const WHICH_LOOKUP_BOUNDARY: &str = "netsuke::stdlib::which::lookup"; + +/// Durability sync for the runner's temporary Ninja file, scoped to the +/// submodule holding only that `sync_all`. +const RUNNER_SYNC_BOUNDARY: &str = "netsuke::runner::process::file_io::ambient_sync"; + +/// Manifest tables that can carry a `metadata.dylint` block, either of which +/// Dylint would honour to resolve lint libraries from a pinned git source. +const METADATA_TABLES: [&str; 2] = ["workspace", "package"]; + +/// Neither manifest may pin `whitaker_suite` to a tag or revision. +/// +/// Netsuke installs the lint libraries at Whitaker HEAD through +/// `whitaker-installer`, which stages them from the suite's default branch. +/// `WHITAKER_INSTALLER_VERSION` pins the installer binary — a separate artefact +/// that says nothing about which lints are staged. A `metadata.dylint` block +/// would take over library resolution and freeze lint behaviour at whatever +/// revision it names, which is the opposite of the intended policy. +/// +/// Keyed on the `dylint` entry rather than on `metadata`, because the root +/// manifest legitimately carries `package.metadata` for other tools. +#[rstest] +#[case::root("Cargo.toml")] +#[case::test_support("test_support/Cargo.toml")] +fn manifests_do_not_pin_the_lint_libraries(#[case] relative: &str) -> Result<()> { + let manifest: TomlValue = read_repo_file(Utf8Path::new(relative))? + .parse() + .with_context(|| format!("parse {relative}"))?; + + for table in METADATA_TABLES { + let pinned = manifest + .get(table) + .and_then(|section| section.get("metadata")) + .and_then(|metadata| metadata.get("dylint")); + ensure!( + pinned.is_none(), + concat!( + "{relative} declares [{table}.metadata.dylint], which would pin ", + "the lint libraries to a fixed source. This repository installs ", + "them at Whitaker HEAD on purpose; WHITAKER_INSTALLER_VERSION ", + "pins the installer, not the libraries. Read the quality-gates ", + "section of docs/developers-guide.md before changing this.", + ), + relative = relative, + table = table + ); + } + Ok(()) +} + +/// Returns the string entries of `key` under `[no_std_fs_operations]`. +fn exclusion_list(dylint_toml: &str, key: &str) -> Result> { + let config: TomlValue = dylint_toml.parse().context("parse dylint.toml")?; + let Some(entries) = config + .get("no_std_fs_operations") + .and_then(|lint| lint.get(key)) + else { + return Ok(Vec::new()); + }; + let array = entries + .as_array() + .with_context(|| format!("{key} should be an array"))?; + array + .iter() + .map(|entry| { + entry + .as_str() + .map(str::to_owned) + .with_context(|| format!("{key} entries should be strings, found {entry:?}")) + }) + .collect() +} + +#[test] +fn lint_whitaker_also_runs_inside_test_support() -> Result<()> { + let makefile = read_repo_file(Utf8Path::new("Makefile"))?; + let recipe = target_recipe(&makefile, "lint-whitaker") + .context("the Makefile should declare a lint-whitaker target")?; + + let invocations: Vec<&str> = recipe + .lines() + .filter(|line| line.contains("$(WHITAKER)")) + .collect(); + ensure!( + invocations.len() == 2, + concat!( + "lint-whitaker should invoke Whitaker twice — once at the ", + "repository root and once inside test_support, which the ", + "workspace excludes — found {count}: {recipe:?}", + ), + count = invocations.len(), + recipe = recipe + ); + + // Counted separately rather than with `any`, which two test_support runs + // would satisfy while leaving the whole application crate unlinted. + let (scoped, root): (Vec<&str>, Vec<&str>) = invocations + .iter() + .partition(|line| line.contains("cd test_support")); + ensure!( + scoped.len() == 1, + concat!( + "exactly one lint-whitaker invocation should run from ", + "test_support/, found {count}: {recipe:?}", + ), + count = scoped.len(), + recipe = recipe + ); + ensure!( + root.len() == 1, + concat!( + "exactly one lint-whitaker invocation should run from the ", + "repository root, found {count}: {recipe:?}", + ), + count = root.len(), + recipe = recipe + ); + Ok(()) +} + +#[test] +fn test_support_carries_its_own_scoped_lint_config() -> Result<()> { + let config = read_repo_file(Utf8Path::new("test_support/dylint.toml"))?; + + // The exact set, not mere membership. `test_support::fs` is the crate's + // only sanctioned ambient boundary, and the exemption list is meant to stay + // one entry long: the `dev_fast` modules that once looked like they needed + // their own entries were all expressible as wrappers returning plain data + // (see the rationale in `test_support/dylint.toml`). A second entry is + // therefore a decision to review, not a routine addition, so it should fail + // here and be justified rather than slip in unnoticed. + let paths = exclusion_list(&config, "excluded_paths")?; + ensure!( + paths == [TEST_SUPPORT_BOUNDARY], + concat!( + "test_support/dylint.toml should exempt exactly {boundary} and ", + "nothing else; adding an entry needs the same scrutiny that ", + "removing the dev_fast ones did. Found {paths:?}", + ), + boundary = TEST_SUPPORT_BOUNDARY, + paths = format!("{paths:?}") + ); + + let crates = exclusion_list(&config, "excluded_crates")?; + ensure!( + crates.is_empty(), + concat!( + "test_support should not exempt whole crates; the boundary is ", + "the {boundary} module, found {crates:?}", + ), + boundary = TEST_SUPPORT_BOUNDARY, + crates = format!("{crates:?}") + ); + Ok(()) +} + +#[rstest] +#[case::root("dylint.toml")] +#[case::test_support("test_support/dylint.toml")] +fn excluded_paths_name_bounded_modules(#[case] relative: &str) -> Result<()> { + let paths = exclusion_list(&read_repo_file(Utf8Path::new(relative))?, "excluded_paths")?; + for path in &paths { + ensure!( + path.contains("::"), + concat!( + "{relative}: {path:?} names a whole crate; excluded_paths ", + "entries must name a module so siblings stay under the ", + "capability policy", + ), + relative = relative, + path = path + ); + } + Ok(()) +} + +#[rstest] +#[case::application_crate("netsuke")] +#[case::test_support("test_support")] +fn crate_is_not_exempted_wholesale(#[case] crate_name: &str) -> Result<()> { + let crates = exclusion_list( + &read_repo_file(Utf8Path::new("dylint.toml"))?, + "excluded_crates", + )?; + ensure!( + !crates.iter().any(|entry| entry == crate_name), + concat!( + "{crate_name} should not appear in excluded_crates: its ambient ", + "access is scoped to named modules, and test_support is linted ", + "separately against test_support/dylint.toml", + ), + crate_name = crate_name + ); + Ok(()) +} + +/// Production boundaries that must stay exempt, each paired with the wider path +/// that must not appear because it would also exempt capability-based siblings. +/// +/// Membership rather than an exact set: `excluded_paths` legitimately grows as +/// new ambient boundaries are identified — `netsuke::cli::discovery::paths` was +/// added upstream while this branch was in review — and pinning the whole list +/// would turn every reviewed addition into a test failure without catching any +/// widening of the entries that matter. +#[rstest] +#[case::which_lookup(WHICH_LOOKUP_BOUNDARY, "netsuke::stdlib::which")] +#[case::runner_ambient_sync(RUNNER_SYNC_BOUNDARY, "netsuke::runner::process::file_io")] +fn production_boundaries_stay_narrowly_scoped( + #[case] required: &str, + #[case] widened: &str, +) -> Result<()> { + let paths = exclusion_list( + &read_repo_file(Utf8Path::new("dylint.toml"))?, + "excluded_paths", + )?; + ensure!( + paths.iter().any(|path| path == required), + "{required} should be exempt, found {paths:?}" + ); + ensure!( + !paths.iter().any(|path| path == widened), + concat!( + "{widened} would exempt more than the ambient boundary it was ", + "added for; keep the entry at {required}, found {paths:?}", + ), + widened = widened, + required = required, + paths = format!("{paths:?}") + ); + Ok(()) +} + +/// Does `entry` exempt `path` under Whitaker's segment-boundary matching? +/// +/// An entry covers itself and its descendants, but never a sibling that merely +/// shares a textual prefix. This mirrors the rule documented in +/// `docs/whitaker-users-guide.md` and is the reason every entry above is a +/// module path: it is what makes `netsuke::stdlib::which::lookup` safe to +/// exempt without also exempting `netsuke::stdlib::which::cache`. +fn entry_covers(entry: &str, path: &str) -> bool { + path == entry + || path + .strip_prefix(entry) + .is_some_and(|r| r.starts_with("::")) +} + +proptest::proptest! { + /// A descendant of an exempt module is covered, however deeply nested. + #[test] + fn exemptions_cover_their_descendants( + segments in proptest::collection::vec("[a-z][a-z0-9_]{0,7}", 1..4), + ) { + let entry = WHICH_LOOKUP_BOUNDARY; + let descendant = format!("{entry}::{}", segments.join("::")); + proptest::prop_assert!( + entry_covers(entry, &descendant), + "{entry} should cover {descendant}" + ); + } + + /// A sibling sharing a textual prefix is never covered. This is the case a + /// crate-wide exclusion would silently swallow. + #[test] + fn exemptions_never_cover_prefix_siblings( + suffix in "[a-z0-9_]{1,8}", + ) { + let entry = WHICH_LOOKUP_BOUNDARY; + let sibling = format!("{entry}{suffix}"); + proptest::prop_assert!( + !entry_covers(entry, &sibling), + "{entry} must not cover the sibling {sibling}" + ); + } + + /// Truncating an entry to a shorter module prefix widens it: the prefix + /// covers the original entry, which is why entries are kept at the + /// narrowest module that owns the ambient operation. + #[test] + fn shorter_prefixes_are_strictly_wider( + depth in 1usize..4, + ) { + let entry = RUNNER_SYNC_BOUNDARY; + let segments: Vec<&str> = entry.split("::").collect(); + let keep = segments.len().saturating_sub(depth).max(1); + let prefix = segments + .iter() + .take(keep) + .copied() + .collect::>() + .join("::"); + proptest::prop_assert!( + entry_covers(&prefix, entry), + "{prefix} would cover {entry}, so it is the wider exemption" + ); + proptest::prop_assert!( + !entry_covers(entry, &prefix), + "{entry} must not cover its own ancestor {prefix}" + ); + } +}