Make the locale test stub strict about unexpected variable reads (#489)

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<String>,
+    values: HashMap<String, String>,
+    allowed: Vec<String>,
 }
 
 impl StubEnv {
-    /// Create a stub environment with the provided locale.
-    pub fn with_locale(locale: impl Into<String>) -> 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<String>) -> 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<String>, value: impl Into<String>) -> 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<String>) -> 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<String> {
-        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()
     }
 }
 

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(),
     };

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(),
     };

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"),

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")]

tests/locale_stub_strictness_tests.rs

@@ -0,0 +1,131 @@
+//! 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::collections::HashMap;
+    use std::panic::{AssertUnwindSafe, catch_unwind};
+
+    #[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<Value = Declaration> {
+        prop_oneof![
+            "[ABC]".prop_map(Declaration::Allow),
+            ("[ABC]", "[a-z]{1,4}").prop_map(|(key, value)| Declaration::Set(key, value)),
+        ]
+    }
+
+    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<String, Option<String>> = 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 {
+                    // Silence the default panic hook around the probe: 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 it
+                    // is restored immediately rather than left installed.
+                    let prior = std::panic::take_hook();
+                    std::panic::set_hook(Box::new(|_| {}));
+                    let read = catch_unwind(AssertUnwindSafe(|| stub.var(key)));
+                    std::panic::set_hook(prior);
+                    prop_assert!(read.is_err(), "undeclared {} should panic", key);
+                }
+            }
+        }
+    }
+}