diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 9ed87131b..319ebcc3b 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1691,6 +1691,45 @@ lock is still held; `try_lock` from the owning thread returns `WouldBlock`, so "blocked" means the bundle still holds it. Reverting the field order turns that assertion red deterministically. +### `StubEnv` strictness + +`test_support::locale_stubs::StubEnv` is the environment-variable test double +used by locale-resolution tests. It answers only the keys a test declares, and +**panics**, naming the key, on any other read. The permissive alternative — +returning `None` for anything unrecognized — hides exactly the regression a +test double should catch: if the code under test starts reading a differently +named variable, through a rename, a typo, or a new precedence rung, a +permissive stub answers `None` and the test still passes, asserting nothing +about the new read. Recognize the panic message, `"which the test did not +declare"`, when a test starts failing after a rename; it means the test's +declarations need updating, not that the stub is broken. + +Three distinct states are representable for a key: **declared with a value** +(`with_var`), **declared but unset** (`allowing`, which reports `None`), and +**undeclared** (any other key, which panics). The middle case matters because +an unset variable is a legitimate scenario to exercise, and it must be +distinguishable from a variable the test never expected to be read at all. +`StubEnv::with_locale` and `StubEnv::without_locale` are the common +constructors for `NETSUKE_LOCALE`; `strict()` starts from nothing declared. + +Declaring the same key twice is well-defined: the most recent declaration +wins, in either order. `allowing` after `with_var` clears the value; `with_var` +after `allowing` restores one. Were `allowing` merely to append to the +permitted-keys list rather than clearing the stored value, it would read as +declaring the key unset while still answering with the earlier value. + +`Default` is deliberately **not** implemented for `StubEnv`. On a strict stub, +"default" would have to mean "deny every read", so `StubEnv::default()` would +compile and then panic at run time for the common "no locale set" case; +requiring `StubEnv::without_locale()` instead makes that intent explicit at +compile time. This refusal is itself a tested contract: +`tests/locale_stub_ui_tests.rs` compiles a fixture calling +`StubEnv::default()` directly with `rustc` and asserts the compile fails with +`E0599` naming the missing `default` item, guarding against the constraint +regressing to a doc-comment promise. `tests/locale_stub_strictness_tests.rs` +covers the panic, the trichotomy, and the last-declaration-wins rule with +both example-based and property tests. + ### Manifest `env()` reader The `env()` Jinja helper reads through an injected [`EnvReader`], a shared diff --git a/test_support/src/locale_stubs.rs b/test_support/src/locale_stubs.rs index 5b2a169f7..e9c604f40 100644 --- a/test_support/src/locale_stubs.rs +++ b/test_support/src/locale_stubs.rs @@ -4,29 +4,151 @@ //! deterministic environment and system locales. use netsuke::locale_resolution::{self, LocaleEnvProvider, SystemLocale}; +use std::collections::HashMap; /// Stub environment provider for locale resolution. -#[derive(Debug, Default, Clone)] +/// +/// Answers only the variables it was given, and **panics** on any other key. +/// +/// The permissive alternative — returning `None` for anything unrecognized — +/// hides exactly the change a test double should catch. Were the code under +/// test altered to read a differently-named variable, through a rename, a typo, +/// or a new precedence rung, a permissive stub would quietly answer `None` and +/// the test would still pass while asserting nothing about the new read. The +/// panic converts that silent pass into a failure naming the unexpected key. +/// +/// `Default` is deliberately **not** implemented. On a strict stub it would +/// mean "deny every read", so `StubEnv::default()` would compile and then +/// panic at run time for the common "no locale set" case. Requiring +/// [`StubEnv::without_locale`] makes that intent explicit at compile time. +#[derive(Debug, Clone)] pub struct StubEnv { - /// Optional locale value to return for `NETSUKE_LOCALE`. - pub locale: Option, + values: HashMap, + allowed: Vec, } impl StubEnv { - /// Create a stub environment with the provided locale. - pub fn with_locale(locale: impl Into) -> Self { + /// Create a stub declaring nothing; every read panics until one is added. + /// + /// # Examples + /// + /// ```rust,should_panic + /// use netsuke::locale_resolution::LocaleEnvProvider; + /// use test_support::locale_stubs::StubEnv; + /// + /// // Nothing is declared, so any read is a programming error. + /// StubEnv::strict().var("ANYTHING"); + /// ``` + #[must_use] + pub fn strict() -> Self { Self { - locale: Some(locale.into()), + values: HashMap::new(), + allowed: Vec::new(), + } + } + + /// Create a stub answering `NETSUKE_LOCALE` with `locale`. + /// + /// # Examples + /// + /// ```rust + /// use netsuke::locale_resolution::LocaleEnvProvider; + /// use test_support::locale_stubs::StubEnv; + /// + /// let env = StubEnv::with_locale("es-ES"); + /// assert_eq!(env.var("NETSUKE_LOCALE").as_deref(), Some("es-ES")); + /// ``` + #[must_use] + pub fn with_locale(locale: impl Into) -> Self { + Self::strict().with_var(locale_resolution::NETSUKE_LOCALE_ENV, locale) + } + + /// Create a stub in which `NETSUKE_LOCALE` is unset but may be read. + /// + /// Distinct from a stub that never expected the read at all: an unset + /// variable is a legitimate case to exercise. + /// + /// # Examples + /// + /// ```rust + /// use netsuke::locale_resolution::LocaleEnvProvider; + /// use test_support::locale_stubs::StubEnv; + /// + /// // Declared, so the read is permitted; unset, so it reports `None`. + /// assert_eq!(StubEnv::without_locale().var("NETSUKE_LOCALE"), None); + /// ``` + #[must_use] + pub fn without_locale() -> Self { + Self::strict().allowing(locale_resolution::NETSUKE_LOCALE_ENV) + } + + /// Answer `key` with `value`. + /// + /// The most recent declaration for a key wins, so this overrides an earlier + /// [`StubEnv::allowing`] for the same key. + /// + /// # Examples + /// + /// ```rust + /// use netsuke::locale_resolution::LocaleEnvProvider; + /// use test_support::locale_stubs::StubEnv; + /// + /// let env = StubEnv::strict().allowing("X").with_var("X", "set"); + /// assert_eq!(env.var("X").as_deref(), Some("set")); + /// ``` + #[must_use] + pub fn with_var(mut self, key: impl Into, value: impl Into) -> Self { + let name = key.into(); + if !self.allowed.iter().any(|allowed| allowed == &name) { + self.allowed.push(name.clone()); } + self.values.insert(name, value.into()); + self + } + + /// Permit `key` to be read, reporting it as unset. + /// + /// Needed because an unset variable is a legitimate case to test, and must + /// be distinguishable from a variable the test never expected to be read. + /// + /// The most recent declaration for a key wins, so this clears a value set + /// by an earlier [`StubEnv::with_var`]. Were it merely to append to the + /// permitted list, the builder would read as declaring the key unset while + /// still answering with the old value. + /// + /// # Examples + /// + /// ```rust + /// use netsuke::locale_resolution::LocaleEnvProvider; + /// use test_support::locale_stubs::StubEnv; + /// + /// let env = StubEnv::strict().with_var("X", "set").allowing("X"); + /// assert_eq!(env.var("X"), None); + /// ``` + #[must_use] + pub fn allowing(mut self, key: impl Into) -> Self { + let name = key.into(); + self.values.remove(&name); + if !self.allowed.iter().any(|allowed| allowed == &name) { + self.allowed.push(name); + } + self } } impl LocaleEnvProvider for StubEnv { fn var(&self, key: &str) -> Option { - if key == locale_resolution::NETSUKE_LOCALE_ENV { - return self.locale.clone(); - } - None + assert!( + self.allowed.iter().any(|allowed| allowed == key), + concat!( + "StubEnv was asked for {:?}, which the test did not declare. ", + "Declare it with `.with_var(..)` or `.allowing(..)` if the read ", + "is intended; otherwise the code under test is reading a ", + "variable the test does not know about." + ), + key + ); + self.values.get(key).cloned() } } diff --git a/tests/bdd/steps/cli.rs b/tests/bdd/steps/cli.rs index 450738d9e..344541427 100644 --- a/tests/bdd/steps/cli.rs +++ b/tests/bdd/steps/cli.rs @@ -34,9 +34,10 @@ use test_support::locale_stubs::{StubEnv, StubSystemLocale}; /// Tests that do not explicitly set up configuration or environment variables /// may be affected by ambient host configuration. pub(super) fn apply_cli(world: &TestWorld, args: &CliArgs) { - let env = StubEnv { - locale: world.locale_env.get(), - }; + let env = world + .locale_env + .get() + .map_or_else(StubEnv::without_locale, StubEnv::with_locale); let system = StubSystemLocale { locale: world.locale_system.get(), }; diff --git a/tests/bdd/steps/locale_resolution.rs b/tests/bdd/steps/locale_resolution.rs index d8ddee5c8..822895345 100644 --- a/tests/bdd/steps/locale_resolution.rs +++ b/tests/bdd/steps/locale_resolution.rs @@ -76,9 +76,10 @@ fn set_cli_override(world: &TestWorld, locale: &str) { #[when("the startup locale is resolved for {args:string}")] fn resolve_startup_locale(world: &TestWorld, args: &str) { - let env = StubEnv { - locale: world.locale_env.get(), - }; + let env = world + .locale_env + .get() + .map_or_else(StubEnv::without_locale, StubEnv::with_locale); let system = StubSystemLocale { locale: world.locale_system.get(), }; diff --git a/tests/locale_resolution_tests.rs b/tests/locale_resolution_tests.rs index 9247f18bc..3d0120b18 100644 --- a/tests/locale_resolution_tests.rs +++ b/tests/locale_resolution_tests.rs @@ -59,7 +59,7 @@ fn resolve_startup_locale_uses_env_then_system() -> Result<()> { "expected env locale to win, got {resolved:?}" ); - let env_fallback = StubEnv::default(); + let env_fallback = StubEnv::without_locale(); let resolved_fallback = resolve_startup_locale(&args, &env_fallback, &system); ensure!( resolved_fallback.as_deref() == Some("es-ES"), diff --git a/tests/locale_stub_strictness_tests.proptest-regressions b/tests/locale_stub_strictness_tests.proptest-regressions new file mode 100644 index 000000000..1a9a7f35c --- /dev/null +++ b/tests/locale_stub_strictness_tests.proptest-regressions @@ -0,0 +1,9 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +# Recorded while mutation-testing the property (allowing's value clear was +# disabled to prove the test detects it), not from a defect in StubEnv. +cc 04881a9256003f8215b73836a16209c1713861db70e59925b4d1ca3ea1cebec3 # shrinks to declarations = [Set("A", "a"), Allow("A")] diff --git a/tests/locale_stub_strictness_tests.rs b/tests/locale_stub_strictness_tests.rs new file mode 100644 index 000000000..7b6f0b39b --- /dev/null +++ b/tests/locale_stub_strictness_tests.rs @@ -0,0 +1,161 @@ +//! Tests for `StubEnv`'s strictness about undeclared reads. +//! +//! Without these, relaxing or deleting the assertion would leave every other +//! test passing while restoring exactly the permissive behaviour the stub was +//! made strict to remove. + +use netsuke::locale_resolution::LocaleEnvProvider; +use test_support::locale_stubs::StubEnv; + +#[test] +#[should_panic(expected = "which the test did not declare")] +fn undeclared_read_panics() { + StubEnv::strict().var("SOME_UNDECLARED_VARIABLE"); +} + +#[test] +#[should_panic(expected = "SOME_OTHER_VARIABLE")] +fn the_panic_names_the_offending_key() { + StubEnv::with_locale("es-ES").var("SOME_OTHER_VARIABLE"); +} + +#[test] +fn declared_reads_do_not_panic() { + assert_eq!( + StubEnv::with_locale("es-ES") + .var("NETSUKE_LOCALE") + .as_deref(), + Some("es-ES") + ); + assert_eq!(StubEnv::without_locale().var("NETSUKE_LOCALE"), None); +} + +/// The most recent declaration for a key wins, in either order. +/// +/// Were `allowing` merely to append to the permitted list, the second case +/// would read as declaring the key unset while still answering `Some("set")`. +#[test] +fn the_last_declaration_for_a_key_wins() { + let value_then_unset = StubEnv::strict().with_var("X", "set").allowing("X"); + assert_eq!(value_then_unset.var("X"), None, "allowing should clear"); + + let unset_then_value = StubEnv::strict().allowing("X").with_var("X", "set"); + assert_eq!( + unset_then_value.var("X").as_deref(), + Some("set"), + "with_var should override" + ); +} + +/// Declaring a key twice must not make the stub answer differently. +#[test] +fn repeated_declaration_is_idempotent() { + let env = StubEnv::strict() + .with_var("X", "first") + .with_var("X", "second"); + assert_eq!(env.var("X").as_deref(), Some("second")); +} + +mod properties { + //! Property coverage for the builder's declaration semantics. + //! + //! The fixed cases above check single interleavings of `with_var` and + //! `allowing`; this states the invariant they are instances of — the last + //! declaration for a key wins — over arbitrary declaration sequences. + + use super::{LocaleEnvProvider, StubEnv}; + use proptest::collection::vec; + use proptest::prelude::*; + use std::cell::Cell; + use std::collections::HashMap; + use std::panic::{AssertUnwindSafe, catch_unwind}; + use std::sync::Once; + + #[derive(Debug, Clone)] + enum Declaration { + Allow(String), + Set(String, String), + } + + /// Three keys only, so generated sequences redeclare the same key often + /// enough for ordering to matter; a wide key space would almost never + /// produce the collisions the invariant is about. + fn declaration() -> impl Strategy { + prop_oneof![ + "[ABC]".prop_map(Declaration::Allow), + ("[ABC]", "[a-z]{1,4}").prop_map(|(key, value)| Declaration::Set(key, value)), + ] + } + + thread_local! { + static SILENCED: Cell = const { Cell::new(false) }; + } + + /// Install the gated hook exactly once, wrapping whatever hook was + /// current when the first probe ran. + fn install_gated_hook() { + let prior = std::panic::take_hook(); + let gated = move |info: &std::panic::PanicHookInfo<'_>| { + if SILENCED.with(Cell::get) { + return; + } + prior(info); + }; + std::panic::set_hook(Box::new(gated)); + } + + /// Run `probe` with the default panic hook silenced for this thread only. + /// + /// Each undeclared read otherwise prints its full panic message, and 256 + /// cases times three keys of that buries any genuine failure output. The + /// hook is process-wide, so instead of swapping it around each probe — + /// which under the threaded in-process coverage runner could eat another + /// test's panic or race the restore — a wrapper is installed once and + /// consults a thread-local flag, leaving every other thread's panics on + /// the prior hook. + fn silenced(probe: impl FnOnce() -> T) -> T { + static INSTALL: Once = Once::new(); + INSTALL.call_once(install_gated_hook); + + SILENCED.with(|flag| flag.set(true)); + let result = probe(); + SILENCED.with(|flag| flag.set(false)); + result + } + + proptest! { + /// Every key answers per its last declaration; undeclared keys panic. + /// + /// The model is a plain last-write-wins map, independent of the + /// stub's split `values`/`allowed` representation, so a bookkeeping + /// slip between the two collections fails here rather than agreeing + /// with itself. + #[test] + fn the_last_declaration_wins_over_any_sequence( + declarations in vec(declaration(), 0..8) + ) { + let mut model: HashMap> = HashMap::new(); + let mut stub = StubEnv::strict(); + for declaration in &declarations { + match declaration { + Declaration::Allow(key) => { + model.insert(key.clone(), None); + stub = stub.allowing(key.clone()); + } + Declaration::Set(key, value) => { + model.insert(key.clone(), Some(value.clone())); + stub = stub.with_var(key.clone(), value.clone()); + } + } + } + for key in ["A", "B", "C"] { + if let Some(expected) = model.get(key) { + prop_assert_eq!(stub.var(key), expected.clone()); + } else { + let read = silenced(|| catch_unwind(AssertUnwindSafe(|| stub.var(key)))); + prop_assert!(read.is_err(), "undeclared {} should panic", key); + } + } + } + } +} diff --git a/tests/locale_stub_ui_tests.rs b/tests/locale_stub_ui_tests.rs new file mode 100644 index 000000000..fc9a9eb9f --- /dev/null +++ b/tests/locale_stub_ui_tests.rs @@ -0,0 +1,190 @@ +//! Compile-time tests for `StubEnv`'s builder-only construction. +//! +//! `StubEnv` deliberately does not implement `Default`: on a strict stub it +//! would mean "deny every read", so `StubEnv::default()` would compile and +//! then panic at run time for the common "no locale set" case. These tests +//! keep that a compile-time contract rather than a doc-comment promise. +//! +//! Trybuild cannot drive them: it removes ambient `RUSTFLAGS` and overrides +//! workspace `build.rustflags` outright (`env_remove("RUSTFLAGS")` plus +//! `--config=build.rustflags=…` in its cargo invocations), so it would +//! rebuild the `netsuke` dependency without `-Zpolonius=next` and reject the +//! crate's `POLONIUS(...)` sites (see docs/polonius.md). Instead the +//! `test_support` rlib is built by Cargo — which does inherit the ambient +//! flags — and the fixtures are compiled directly with the workspace `rustc` +//! against that rlib. + +use rstest::{fixture, rstest}; +use std::{ + io, + path::{Path, PathBuf}, + process::{Command, Output}, +}; + +/// One `test_support` build shared by both tests. +/// +/// Built once: the two tests run in parallel, so independent builds would +/// contend on Cargo's target-directory lock and repeat completed work. +#[fixture] +#[once] +fn test_support_rlib() -> TestSupportRlib { + #[expect( + clippy::expect_used, + reason = "a once fixture cannot return Result; a build failure must abort the suite here" + )] + let rlib = TestSupportRlib::build().expect("test_support should build"); + rlib +} + +#[rstest] +fn stub_env_default_does_not_compile(test_support_rlib: &TestSupportRlib) -> io::Result<()> { + let output = test_support_rlib.compile("tests/ui/stub_env_default_compile_fail.rs")?; + + if output.status.success() { + return Err(io::Error::other("StubEnv::default() should not compile")); + } + let stderr = stderr(&output); + if !stderr.contains("E0599") || !stderr.contains("`default`") { + return Err(io::Error::other(format!( + concat!( + "the rejection should be the missing `default` item, ", + "not a harness fault:\n{}", + ), + stderr + ))); + } + Ok(()) +} + +/// The builder constructors compile under the same harness. +/// +/// This is the control for the compile-fail case: it fails if the `--extern` +/// or `-L dependency` wiring breaks, which would otherwise make the rejection +/// above pass for the wrong reason. +#[rstest] +fn stub_env_builders_compile_under_the_same_harness( + test_support_rlib: &TestSupportRlib, +) -> io::Result<()> { + let output = test_support_rlib.compile("tests/ui/stub_env_strict_compile_pass.rs")?; + + if !output.status.success() { + return Err(io::Error::other(format!( + "the control fixture should compile; the harness wiring is broken:\n{}", + stderr(&output), + ))); + } + Ok(()) +} + +/// The `test_support` rlib and the deps directory holding its dependencies. +struct TestSupportRlib { + rlib: PathBuf, + deps_dir: PathBuf, +} + +impl TestSupportRlib { + /// Build `test_support` with Cargo and locate the resulting rlib. + /// + /// Cargo inherits the ambient `RUSTFLAGS`, so the rlib is borrow-checked + /// with the same Polonius flags as the rest of the suite — the property + /// trybuild could not preserve. + fn build() -> io::Result { + let output = Command::new(cargo()) + .arg("build") + .arg("--manifest-path") + .arg(manifest_dir().join("test_support/Cargo.toml")) + .arg("--message-format=json") + .output()?; + if !output.status.success() { + return Err(io::Error::other(format!( + "building test_support failed:\n{}", + stderr(&output), + ))); + } + + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let rlib = stdout + .lines() + .filter_map(test_support_rlib_in_message) + .next_back() + .ok_or_else(|| io::Error::other("cargo reported no test_support rlib artefact"))?; + // Cargo uplifts the top-level package's rlib out of `deps/` into the + // profile directory, so the rlib's own parent is not where the + // dependency rlibs live. + let parent = rlib + .parent() + .ok_or_else(|| io::Error::other("the rlib path should have a parent"))?; + let deps_dir = if parent.file_name() == Some(std::ffi::OsStr::new("deps")) { + parent.to_path_buf() + } else { + parent.join("deps") + }; + Ok(Self { rlib, deps_dir }) + } + + /// Type-check `source` against the rlib without linking a binary. + /// + /// `--emit=metadata` is enough to surface the missing-item error while + /// sparing the harness a full link of `test_support`'s dependency tree. + fn compile(&self, source: &str) -> io::Result { + let output_dir = tempfile::tempdir()?; + Command::new(rustc()) + .arg("--edition=2024") + .arg("--crate-type=bin") + .arg("--emit=metadata") + .arg(manifest_dir().join(source)) + .arg("--extern") + .arg(format!("test_support={}", self.rlib.display())) + .arg("-L") + .arg(format!("dependency={}", self.deps_dir.display())) + .arg("-o") + .arg(output_dir.path().join("stub-env-ui.rmeta")) + .output() + } +} + +/// Extract the `test_support` rlib path from one Cargo JSON message, if any. +fn test_support_rlib_in_message(line: &str) -> Option { + let message: serde_json::Value = serde_json::from_str(line).ok()?; + if message.get("reason")? != "compiler-artifact" + || message.get("target")?.get("name")? != "test_support" + { + return None; + } + message + .get("filenames")? + .as_array()? + .iter() + .filter_map(|filename| filename.as_str()) + .filter(|filename| { + Path::new(filename) + .extension() + .is_some_and(|extension| extension.eq_ignore_ascii_case("rlib")) + }) + .map(PathBuf::from) + .next_back() +} + +fn manifest_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +#[expect( + clippy::disallowed_methods, + reason = "locating build tools Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] +fn cargo() -> PathBuf { + std::env::var_os("CARGO").map_or_else(|| Path::new("cargo").to_path_buf(), PathBuf::from) +} + +#[expect( + clippy::disallowed_methods, + reason = "locating build tools Cargo reports through the environment; there is no seam to inject and no process state to isolate" +)] +fn rustc() -> PathBuf { + std::env::var_os("RUSTC").map_or_else(|| Path::new("rustc").to_path_buf(), PathBuf::from) +} + +fn stderr(output: &Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} diff --git a/tests/ui/stub_env_default_compile_fail.rs b/tests/ui/stub_env_default_compile_fail.rs new file mode 100644 index 000000000..bb1a99473 --- /dev/null +++ b/tests/ui/stub_env_default_compile_fail.rs @@ -0,0 +1,9 @@ +//! `StubEnv::default()` must not compile. +//! +//! `Default` on a strict stub would mean "deny every read", so the common +//! "no locale set" case would compile and then panic at run time; the builder +//! constructors keep that intent explicit at compile time. + +fn main() { + let _ = test_support::locale_stubs::StubEnv::default(); +} diff --git a/tests/ui/stub_env_strict_compile_pass.rs b/tests/ui/stub_env_strict_compile_pass.rs new file mode 100644 index 000000000..f3aa53332 --- /dev/null +++ b/tests/ui/stub_env_strict_compile_pass.rs @@ -0,0 +1,9 @@ +//! Control fixture proving the harness links `test_support` correctly. +//! +//! Were the `--extern` wiring broken, the compile-fail fixture would be +//! rejected for the wrong reason and its test would pass vacuously; this +//! fixture fails instead, naming the harness as the fault. + +fn main() { + let _ = test_support::locale_stubs::StubEnv::strict(); +}