From 0a57367fee221a45254113437e200edcf8d83f3a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 10 Apr 2026 08:49:31 -0400 Subject: [PATCH] refactor(config): use lookup injection for active_settings_path env test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The active_settings_path_honors_fabro_config_env test was using an EnvGuard that called std::env::set_var/remove_var — unsafe shared mutable state in a parallel test binary. Extract active_settings_path_with_lookup that takes an env-lookup closure (same pattern as resolve_auth_mode_with_lookup in jwt_auth.rs). Rewrite the test to inject the env value via the closure. Delete the EnvGuard struct — no remaining callers. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-config/src/user.rs | 46 ++++++++++------------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/lib/crates/fabro-config/src/user.rs b/lib/crates/fabro-config/src/user.rs index 415878877..4cce634c4 100644 --- a/lib/crates/fabro-config/src/user.rs +++ b/lib/crates/fabro-config/src/user.rs @@ -33,8 +33,15 @@ pub fn legacy_default_storage_root() -> PathBuf { } pub fn active_settings_path(path: Option<&Path>) -> PathBuf { + active_settings_path_with_lookup(path, |name| std::env::var_os(name)) +} + +fn active_settings_path_with_lookup( + path: Option<&Path>, + lookup: impl Fn(&str) -> Option, +) -> PathBuf { path.map(Path::to_path_buf) - .or_else(|| std::env::var_os(FABRO_CONFIG_ENV).map(PathBuf::from)) + .or_else(|| lookup(FABRO_CONFIG_ENV).map(PathBuf::from)) .unwrap_or_else(default_settings_path) } @@ -110,37 +117,12 @@ fn load_v2_layer_from_path(path: &Path) -> anyhow::Result { #[cfg(test)] mod tests { use super::{ - FABRO_CONFIG_ENV, LEGACY_OLD_USER_CONFIG_FILENAME, LEGACY_SERVER_CONFIG_FILENAME, - LEGACY_USER_CONFIG_FILENAME, SETTINGS_CONFIG_FILENAME, active_settings_path, + LEGACY_OLD_USER_CONFIG_FILENAME, LEGACY_SERVER_CONFIG_FILENAME, + LEGACY_USER_CONFIG_FILENAME, SETTINGS_CONFIG_FILENAME, active_settings_path_with_lookup, default_settings_path, default_socket_path, legacy_old_user_config_path, legacy_server_config_path, legacy_user_config_path, should_warn_about_legacy_user_config, }; - struct EnvGuard { - key: &'static str, - original: Option, - } - - impl EnvGuard { - fn set(key: &'static str, value: Option<&std::path::Path>) -> Self { - let original = std::env::var_os(key); - match value { - Some(value) => std::env::set_var(key, value), - None => std::env::remove_var(key), - } - Self { key, original } - } - } - - impl Drop for EnvGuard { - fn drop(&mut self) { - match &self.original { - Some(value) => std::env::set_var(self.key, value), - None => std::env::remove_var(self.key), - } - } - } - #[test] fn should_warn_about_legacy_user_config_once_per_path() { let dir = tempfile::tempdir().unwrap(); @@ -193,8 +175,10 @@ mod tests { fn active_settings_path_honors_fabro_config_env() { let dir = tempfile::tempdir().unwrap(); let custom_path = dir.path().join("custom-settings.toml"); - let _guard = EnvGuard::set(FABRO_CONFIG_ENV, Some(&custom_path)); - - assert_eq!(active_settings_path(None), custom_path); + let custom_os = custom_path.clone().into_os_string(); + assert_eq!( + active_settings_path_with_lookup(None, |_| Some(custom_os.clone())), + custom_path, + ); } }