From f0ecc9b5ae43e2cee6b52c1fddeb632639f65880 Mon Sep 17 00:00:00 2001 From: Frank O'Hara Date: Sun, 9 Aug 2026 07:32:13 -0600 Subject: [PATCH] fix(types): stop the user-config tests writing to the real home directory `test_user_config_save_and_load` called `UserConfig::save()`, which writes `~/.lash/config.toml` on the developer's machine. It restored afterwards with `UserConfig::default().save()`, which was wrong twice over: on the happy path it replaced whatever the user actually had with defaults, and if the test panicked or the process was killed the restore never ran at all, leaving `color_scheme = "Test Theme"` behind. No theme resolves that name, so every later `lash` invocation on the machine failed with `Color scheme 'Test Theme' not found`. A mutation-testing run surfaced this on 2026-08-09 by killing the suite mid-execution hundreds of times, but any interrupted `cargo test` does the same. `load_from(path)` and `save_to(path)` now take the config path explicitly; `load()` and `save()` are thin wrappers that pass the home-directory path. The tests point at a `TempDir` and never resolve `$HOME`. Also covered: saving into a directory that does not exist yet, and rejecting invalid TOML on load. `test_user_config_load_nonexistent` no longer carries its "assumes ~/.lash/config.toml doesn't exist or is valid" caveat, since it now reads a path it created. Audited the rest of the workspace for the same pattern. lash-cli's `Config::user_config_path` resolves a different location (`dirs::config_dir`) and its test only inspects the path. lash-tui's `apply_selected_theme` calls `save()` for real, which is correct, and no test exercises it. --- crates/lash-types/src/config.rs | 93 +++++++++++++++++++++++---------- 1 file changed, 66 insertions(+), 27 deletions(-) diff --git a/crates/lash-types/src/config.rs b/crates/lash-types/src/config.rs index 1411145..ded009d 100644 --- a/crates/lash-types/src/config.rs +++ b/crates/lash-types/src/config.rs @@ -332,7 +332,18 @@ impl UserConfig { /// /// Returns error if config file exists but is invalid pub fn load() -> Result { - let config_path = Self::user_config_path()?; + Self::load_from(Self::user_config_path()?) + } + + /// Load user configuration from an explicit path + /// + /// If the file doesn't exist, returns default configuration. + /// + /// # Errors + /// + /// Returns error if config file exists but is invalid + pub fn load_from(config_path: impl Into) -> Result { + let config_path = config_path.into(); if !config_path.exists() { return Ok(Self::default()); @@ -366,17 +377,30 @@ impl UserConfig { /// /// Returns error if config cannot be written pub fn save(&self) -> Result<()> { - let config_dir = Self::user_config_dir()?; - let config_path = Self::user_config_path()?; + self.save_to(Self::user_config_path()?) + } + + /// Save user configuration to an explicit path + /// + /// Creates the parent directory if it doesn't exist. Uses atomic writes to + /// prevent corruption. + /// + /// # Errors + /// + /// Returns error if config cannot be written + pub fn save_to(&self, config_path: impl Into) -> Result<()> { + let config_path = config_path.into(); // Create directory if it doesn't exist - if !config_dir.exists() { - fs::create_dir_all(&config_dir).map_err(|e| LashError::IO { - code: codes::E_IO_WRITE_ERROR, - message: format!("Failed to create user config directory: {e}"), - path: Some(config_dir.clone()), - io_error: Some(e.to_string()), - })?; + if let Some(config_dir) = config_path.parent() { + if !config_dir.exists() { + fs::create_dir_all(config_dir).map_err(|e| LashError::IO { + code: codes::E_IO_WRITE_ERROR, + message: format!("Failed to create user config directory: {e}"), + path: Some(config_dir.to_path_buf()), + io_error: Some(e.to_string()), + })?; + } } // Serialize to TOML @@ -718,33 +742,48 @@ mod tests { #[test] fn test_user_config_load_nonexistent() { - // This test assumes ~/.lash/config.toml doesn't exist or is valid - // If it exists, it should parse successfully - let result = UserConfig::load(); - assert!(result.is_ok()); + let dir = TempDir::new().unwrap(); + let missing = dir.path().join("config.toml"); + + let loaded = UserConfig::load_from(&missing).unwrap(); + + assert_eq!(loaded, UserConfig::default()); + assert!(!missing.exists(), "load must not create the config file"); } #[test] fn test_user_config_save_and_load() { - // Create a custom config + let dir = TempDir::new().unwrap(); + let path = dir.path().join("config.toml"); + let config = UserConfig { color_scheme: "Test Theme".to_string(), tree_view: TreeViewConfig::default(), }; + config.save_to(&path).unwrap(); - // Save it - let save_result = config.save(); - assert!( - save_result.is_ok(), - "Failed to save config: {:?}", - save_result - ); + let loaded = UserConfig::load_from(&path).unwrap(); - // Load it back - let loaded = UserConfig::load().unwrap(); - assert_eq!(loaded.color_scheme, "Test Theme"); + assert_eq!(loaded, config); + } + + #[test] + fn test_user_config_save_creates_missing_parent_directory() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("nested").join(".lash").join("config.toml"); + + UserConfig::default().save_to(&path).unwrap(); + + assert!(path.exists()); + assert_eq!(UserConfig::load_from(&path).unwrap(), UserConfig::default()); + } + + #[test] + fn test_user_config_load_rejects_invalid_toml() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("config.toml"); + fs::write(&path, "color_scheme = ").unwrap(); - // Restore default - UserConfig::default().save().unwrap(); + assert!(UserConfig::load_from(&path).is_err()); } }