From af818e430a315df85511801ce1ff6e4fa24eb3bf Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 29 Jul 2025 18:09:36 +0100 Subject: [PATCH 1/4] Extract EnvVarGuard for tests --- crates/comenqd/src/config.rs | 46 +++++++----------------------------- tests/cucumber.rs | 1 + tests/steps/config_steps.rs | 46 +++--------------------------------- tests/support/env_guard.rs | 43 +++++++++++++++++++++++++++++++++ tests/support/mod.rs | 3 +++ 5 files changed, 59 insertions(+), 80 deletions(-) create mode 100644 tests/support/env_guard.rs create mode 100644 tests/support/mod.rs diff --git a/crates/comenqd/src/config.rs b/crates/comenqd/src/config.rs index 451775c..8ff9b63 100644 --- a/crates/comenqd/src/config.rs +++ b/crates/comenqd/src/config.rs @@ -118,46 +118,18 @@ mod tests { use std::fs; use tempfile::tempdir; - struct EnvVarGuard { - key: String, - original: Option, + mod env_guard { + include!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/../../tests/support/env_guard.rs" + )); } - impl EnvVarGuard { - fn set(key: &str, val: &str) -> Self { - let original = std::env::var(key).ok(); - set_env_var(key, val); - Self { - key: key.to_string(), - original, - } - } - } - - impl Drop for EnvVarGuard { - fn drop(&mut self) { - match &self.original { - Some(v) => set_env_var(&self.key, v), - None => remove_env_var(&self.key), - } - } + pub mod support { + pub use super::env_guard::{EnvVarGuard, remove_env_var, set_env_var}; } - fn remove_env(key: &str) { - remove_env_var(key); - } - - /// Safely set an environment variable for tests. - fn set_env_var(key: &str, val: &str) { - // Safety: tests using `serial_test::serial` run single-threaded. - unsafe { std::env::set_var(key, val) }; - } - - /// Safely remove an environment variable for tests. - fn remove_env_var(key: &str) { - // Safety: tests using `serial_test::serial` run single-threaded. - unsafe { std::env::remove_var(key) }; - } + use support::{EnvVarGuard, remove_env_var}; #[rstest] #[serial_test::serial] @@ -169,7 +141,7 @@ mod tests { "github_token='abc'\nsocket_path='/tmp/s.sock'\nqueue_path='/tmp/q'", ) .unwrap(); - remove_env("COMENQD_SOCKET_PATH"); + remove_env_var("COMENQD_SOCKET_PATH"); let cfg = Config::from_file(&path).unwrap(); assert_eq!(cfg.github_token, "abc"); assert_eq!(cfg.socket_path, PathBuf::from("/tmp/s.sock")); diff --git a/tests/cucumber.rs b/tests/cucumber.rs index de5e190..a2ef699 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -1,4 +1,5 @@ mod steps; +mod support; use cucumber::World as _; use steps::{CliWorld, ClientWorld, CommentWorld, ConfigWorld, ListenerWorld, WorkerWorld}; diff --git a/tests/steps/config_steps.rs b/tests/steps/config_steps.rs index 51a10d6..a03fbab 100644 --- a/tests/steps/config_steps.rs +++ b/tests/steps/config_steps.rs @@ -5,49 +5,9 @@ use std::fs; use std::path::PathBuf; use tempfile::TempDir; +use crate::support::env_guard::{EnvVarGuard, remove_env_var}; use comenqd::config::Config; -/// RAII guard for temporarily setting an environment variable. -#[derive(Debug)] -struct EnvVarGuard { - key: String, - original: Option, -} - -impl EnvVarGuard { - fn set(key: &str, value: &str) -> Self { - let original = std::env::var(key).ok(); - set_env_var_safe(key, value); - Self { - key: key.to_string(), - original, - } - } -} - -impl Drop for EnvVarGuard { - fn drop(&mut self) { - match &self.original { - Some(val) => set_env_var_safe(&self.key, val), - None => remove_env_var_safe(&self.key), - } - } -} - -fn remove_env(key: &str) { - remove_env_var_safe(key); -} - -fn set_env_var_safe(key: &str, value: &str) { - // Safety: each scenario runs under serial_test, so no concurrent access. - unsafe { std::env::set_var(key, value) }; -} - -fn remove_env_var_safe(key: &str) { - // Safety: each scenario runs under serial_test, so no concurrent access. - unsafe { std::env::remove_var(key) }; -} - #[derive(Debug, Default, World)] pub struct ConfigWorld { dir: Option, @@ -68,7 +28,7 @@ fn config_file_with_token(world: &mut ConfigWorld, token: String) { fs::write(&path, format!("github_token='{token}'")).expect("write file"); world.dir = Some(dir); world.path = Some(path); - remove_env("COMENQD_SOCKET_PATH"); + remove_env_var("COMENQD_SOCKET_PATH"); } #[expect(clippy::expect_used, reason = "test setup uses expect")] @@ -107,7 +67,7 @@ fn config_without_socket(world: &mut ConfigWorld, token: String) { .expect("write file"); world.dir = Some(dir); world.path = Some(path); - remove_env("COMENQD_SOCKET_PATH"); + remove_env_var("COMENQD_SOCKET_PATH"); } #[given("a missing configuration file")] diff --git a/tests/support/env_guard.rs b/tests/support/env_guard.rs new file mode 100644 index 0000000..70ad976 --- /dev/null +++ b/tests/support/env_guard.rs @@ -0,0 +1,43 @@ +//! Test helpers for managing environment variables. +//! +//! `EnvVarGuard` temporarily sets an environment variable and restores the +//! previous value on drop. + +#[derive(Debug)] +pub struct EnvVarGuard { + key: String, + original: Option, +} + +impl EnvVarGuard { + /// Set an environment variable for the lifetime of the returned guard. + pub fn set(key: &str, value: &str) -> Self { + let original = std::env::var(key).ok(); + set_env_var(key, value); + Self { + key: key.to_string(), + original, + } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + match &self.original { + Some(v) => set_env_var(&self.key, v), + None => remove_env_var(&self.key), + } + } +} + +/// Safely set an environment variable for tests. +pub fn set_env_var(key: &str, value: &str) { + // Safety: tests execute serially so no concurrent access occurs. + unsafe { std::env::set_var(key, value) }; +} + +/// Safely remove an environment variable for tests. +pub fn remove_env_var(key: &str) { + // Safety: tests execute serially so no concurrent access occurs. + unsafe { std::env::remove_var(key) }; +} diff --git a/tests/support/mod.rs b/tests/support/mod.rs new file mode 100644 index 0000000..0ab1eed --- /dev/null +++ b/tests/support/mod.rs @@ -0,0 +1,3 @@ +//! Support utilities shared by tests. + +pub mod env_guard; From 572153e26f973c54252fd356b3fd200ce6303393 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 29 Jul 2025 18:39:16 +0100 Subject: [PATCH 2/4] Add tests for env var utilities --- tests/support/env_guard.rs | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/support/env_guard.rs b/tests/support/env_guard.rs index 70ad976..df784b0 100644 --- a/tests/support/env_guard.rs +++ b/tests/support/env_guard.rs @@ -41,3 +41,63 @@ pub fn remove_env_var(key: &str) { // Safety: tests execute serially so no concurrent access occurs. unsafe { std::env::remove_var(key) }; } + +#[cfg(test)] +mod tests { + #[test] + #[serial_test::serial] + fn set_env_var_sets_variable() { + let key = "ENV_GUARD_SET"; + super::remove_env_var(key); + assert!(std::env::var(key).is_err()); + + super::set_env_var(key, "value"); + assert_eq!(std::env::var(key).unwrap(), "value"); + + super::remove_env_var(key); + } + + #[test] + #[serial_test::serial] + fn remove_env_var_removes_variable() { + let key = "ENV_GUARD_REMOVE"; + std::env::set_var(key, "to_remove"); + assert_eq!(std::env::var(key).unwrap(), "to_remove"); + + super::remove_env_var(key); + assert!(std::env::var(key).is_err()); + } + + #[test] + #[serial_test::serial] + fn remove_env_var_when_unset_is_noop() { + let key = "ENV_GUARD_REMOVE_UNSET"; + super::remove_env_var(key); + assert!(std::env::var(key).is_err()); + } + + #[test] + #[serial_test::serial] + fn nested_env_var_guard_restores_previous_value() { + let key = "ENV_GUARD_TEST_NESTED"; + super::remove_env_var(key); + + std::env::set_var(key, "initial"); + assert_eq!(std::env::var(key).unwrap(), "initial"); + + let guard1 = super::EnvVarGuard::set(key, "first"); + assert_eq!(std::env::var(key).unwrap(), "first"); + + { + let _guard2 = super::EnvVarGuard::set(key, "second"); + assert_eq!(std::env::var(key).unwrap(), "second"); + } + + assert_eq!(std::env::var(key).unwrap(), "first"); + + drop(guard1); + assert_eq!(std::env::var(key).unwrap(), "initial"); + + super::remove_env_var(key); + } +} From a490cab3c0219d28a2d07438f27ffdb132e4afe5 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 31 Jul 2025 16:30:31 +0100 Subject: [PATCH 3/4] Clarify unsafe env helpers --- tests/support/env_guard.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/support/env_guard.rs b/tests/support/env_guard.rs index df784b0..8afc885 100644 --- a/tests/support/env_guard.rs +++ b/tests/support/env_guard.rs @@ -30,15 +30,18 @@ impl Drop for EnvVarGuard { } } -/// Safely set an environment variable for tests. +/// Set an environment variable for tests. +/// +/// The nightly compiler marks `std::env::set_var` as `unsafe`. +/// Tests run serially so using it is acceptable here. pub fn set_env_var(key: &str, value: &str) { - // Safety: tests execute serially so no concurrent access occurs. unsafe { std::env::set_var(key, value) }; } -/// Safely remove an environment variable for tests. +/// Remove an environment variable for tests. +/// +/// `std::env::remove_var` is also `unsafe` on nightly. pub fn remove_env_var(key: &str) { - // Safety: tests execute serially so no concurrent access occurs. unsafe { std::env::remove_var(key) }; } From 80c8d5afd33950cd599f8c78435009bf43de5cbf Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 31 Jul 2025 17:50:09 +0100 Subject: [PATCH 4/4] Use helpers for env var tests --- tests/support/env_guard.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/support/env_guard.rs b/tests/support/env_guard.rs index 8afc885..2007712 100644 --- a/tests/support/env_guard.rs +++ b/tests/support/env_guard.rs @@ -64,7 +64,7 @@ mod tests { #[serial_test::serial] fn remove_env_var_removes_variable() { let key = "ENV_GUARD_REMOVE"; - std::env::set_var(key, "to_remove"); + super::set_env_var(key, "to_remove"); assert_eq!(std::env::var(key).unwrap(), "to_remove"); super::remove_env_var(key); @@ -85,7 +85,7 @@ mod tests { let key = "ENV_GUARD_TEST_NESTED"; super::remove_env_var(key); - std::env::set_var(key, "initial"); + super::set_env_var(key, "initial"); assert_eq!(std::env::var(key).unwrap(), "initial"); let guard1 = super::EnvVarGuard::set(key, "first");