Stabilize test, improve locators.

simi · simi · commit fd60aac4e892 · 2025-11-09T15:15:50.000+01:00
diff --git a/crates/rb-cli/src/config/locator.rs b/crates/rb-cli/src/config/locator.rs
@@ -1,6 +1,20 @@
 use log::debug;
 use std::path::PathBuf;
 
+/// Trait for reading environment variables - allows mocking in tests
+pub trait EnvReader {
+    fn var(&self, key: &str) -> Result<String, std::env::VarError>;
+}
+
+/// Production implementation using std::env
+pub struct StdEnvReader;
+
+impl EnvReader for StdEnvReader {
+    fn var(&self, key: &str) -> Result<String, std::env::VarError> {
+        std::env::var(key)
+    }
+}
+
 /// Locate the configuration file following XDG Base Directory specification
 ///
 /// Supports both rb.kdl and rb.toml (preferring .kdl)
@@ -13,6 +27,14 @@ use std::path::PathBuf;
 /// 5. %APPDATA%/rb/rb.kdl or rb.toml (Windows)
 /// 6. ~/.rb.kdl or ~/.rb.toml (cross-platform fallback)
 pub fn locate_config_file(override_path: Option<PathBuf>) -> Option<PathBuf> {
+    locate_config_file_with_env(override_path, &StdEnvReader)
+}
+
+/// Internal function that accepts an environment reader for testing
+fn locate_config_file_with_env(
+    override_path: Option<PathBuf>,
+    env: &dyn EnvReader,
+) -> Option<PathBuf> {
     debug!("Searching for configuration file...");
 
     // 1. Check for explicit override first
@@ -25,7 +47,7 @@ pub fn locate_config_file(override_path: Option<PathBuf>) -> Option<PathBuf> {
     }
 
     // 2. Check RB_CONFIG environment variable
-    if let Ok(rb_config) = std::env::var("RB_CONFIG") {
+    if let Ok(rb_config) = env.var("RB_CONFIG") {
         let config_path = PathBuf::from(rb_config);
         debug!("  Checking RB_CONFIG env var: {}", config_path.display());
         if config_path.exists() {
@@ -35,7 +57,7 @@ pub fn locate_config_file(override_path: Option<PathBuf>) -> Option<PathBuf> {
     }
 
     // 3. Try XDG_CONFIG_HOME (Unix/Linux)
-    if let Ok(xdg_config) = std::env::var("XDG_CONFIG_HOME") {
+    if let Ok(xdg_config) = env.var("XDG_CONFIG_HOME") {
         let base_path = PathBuf::from(xdg_config).join("rb");
         // Try .kdl first, then .toml
         for ext in &["rb.kdl", "rb.toml"] {
@@ -98,6 +120,34 @@ pub fn locate_config_file(override_path: Option<PathBuf>) -> Option<PathBuf> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::collections::HashMap;
+
+    /// Mock environment reader for testing without global state mutation
+    struct MockEnvReader {
+        vars: HashMap<String, String>,
+    }
+
+    impl MockEnvReader {
+        fn new() -> Self {
+            Self {
+                vars: HashMap::new(),
+            }
+        }
+
+        fn with_var(mut self, key: impl Into<String>, value: impl Into<String>) -> Self {
+            self.vars.insert(key.into(), value.into());
+            self
+        }
+    }
+
+    impl EnvReader for MockEnvReader {
+        fn var(&self, key: &str) -> Result<String, std::env::VarError> {
+            self.vars
+                .get(key)
+                .cloned()
+                .ok_or(std::env::VarError::NotPresent)
+        }
+    }
 
     #[test]
     fn test_locate_config_file_returns_option() {
@@ -127,24 +177,44 @@ mod tests {
     fn test_locate_config_file_with_env_var() {
         use std::fs;
         let temp_dir = std::env::temp_dir();
-        let config_path = temp_dir.join("test_rb_env.toml");
+        let config_path = temp_dir.join("test_rb_env_mock.toml");
 
         // Create a temporary config file
         fs::write(&config_path, "# test config").expect("Failed to write test config");
 
-        // Set environment variable (unsafe but required for testing)
-        unsafe {
-            std::env::set_var("RB_CONFIG", &config_path);
-        }
+        // Use mock environment - no global state mutation!
+        let mock_env =
+            MockEnvReader::new().with_var("RB_CONFIG", config_path.to_string_lossy().to_string());
 
         // Should return the env var path
-        let result = locate_config_file(None);
+        let result = locate_config_file_with_env(None, &mock_env);
         assert_eq!(result, Some(config_path.clone()));
 
         // Cleanup
-        unsafe {
-            std::env::remove_var("RB_CONFIG");
-        }
         let _ = fs::remove_file(&config_path);
     }
+
+    #[test]
+    fn test_locate_config_file_with_xdg_config_home() {
+        use std::fs;
+        let temp_dir = std::env::temp_dir();
+        let xdg_base = temp_dir.join("test_xdg_config");
+        let rb_dir = xdg_base.join("rb");
+        let config_path = rb_dir.join("rb.toml");
+
+        // Create directory structure
+        fs::create_dir_all(&rb_dir).expect("Failed to create test directory");
+        fs::write(&config_path, "# test config").expect("Failed to write test config");
+
+        // Use mock environment
+        let mock_env = MockEnvReader::new()
+            .with_var("XDG_CONFIG_HOME", xdg_base.to_string_lossy().to_string());
+
+        // Should return the XDG config path
+        let result = locate_config_file_with_env(None, &mock_env);
+        assert_eq!(result, Some(config_path.clone()));
+
+        // Cleanup
+        let _ = fs::remove_dir_all(&xdg_base);
+    }
 }
diff --git a/crates/rb-cli/src/lib.rs b/crates/rb-cli/src/lib.rs
@@ -242,7 +242,21 @@ mod tests {
                 .expect("Failed to set permissions");
         }
 
-        let result = create_ruby_context(Some(sandbox.root().to_path_buf()), None);
+        // Create gem directories so gem runtime is inferred
+        let gem_base = sandbox.gem_base_dir();
+        let gem_dir = gem_base.join("3.2.5");
+        std::fs::create_dir_all(&gem_dir).expect("Failed to create gem dir");
+
+        // Use the internal method that accepts current_dir to avoid global state
+        use rb_core::butler::ButlerRuntime;
+        let result = ButlerRuntime::discover_and_compose_with_current_dir(
+            sandbox.root().to_path_buf(),
+            None,
+            None,
+            false,
+            sandbox.root().to_path_buf(), // Current dir = sandbox root
+        )
+        .expect("Failed to create ButlerRuntime");
 
         // Should successfully create a ButlerRuntime
         let current_path = std::env::var("PATH").ok();
diff --git a/crates/rb-core/src/bundler/mod.rs b/crates/rb-core/src/bundler/mod.rs
@@ -1,5 +1,6 @@
 use crate::butler::Command;
 use crate::butler::runtime_provider::RuntimeProvider;
+use crate::ruby::RubyVersionExt;
 use log::debug;
 use semver::Version;
 use std::path::{Path, PathBuf};
@@ -40,14 +41,12 @@ impl BundlerRuntime {
         self.app_config_dir().join("vendor").join("bundler")
     }
 
-    /// Returns the ruby-specific vendor directory (.rb/vendor/bundler/ruby/X.Y.Z)
-    /// This requires a Ruby version to be detected
+    /// Returns the ruby-specific vendor directory (.rb/vendor/bundler/ruby/X.Y.0)
+    /// Uses Ruby ABI version (major.minor.0) for compatibility grouping
     pub fn ruby_vendor_dir(&self, ruby_version: &Version) -> PathBuf {
-        let version_str = format!(
-            "{}.{}.{}",
-            ruby_version.major, ruby_version.minor, ruby_version.patch
-        );
-        self.vendor_dir().join("ruby").join(version_str)
+        self.vendor_dir()
+            .join("ruby")
+            .join(ruby_version.ruby_abi_version())
     }
 
     /// Detect Ruby version from .ruby-version file or Gemfile ruby declaration
@@ -58,7 +57,7 @@ impl BundlerRuntime {
     }
 
     /// Returns the bin directory where bundler-installed executables live
-    /// Path: .rb/vendor/bundler/ruby/X.Y.Z/bin
+    /// Path: .rb/vendor/bundler/ruby/X.Y.0/bin
     pub fn bin_dir(&self) -> PathBuf {
         let bin_dir = self.ruby_vendor_dir(&self.ruby_version).join("bin");
         debug!("Bundler bin directory: {}", bin_dir.display());
@@ -418,8 +417,8 @@ mod tests {
     #[test]
     fn bin_dir_is_vendor_bin() {
         let br = bundler_rt("/home/user/project");
-        // bin_dir should include Ruby version: .rb/vendor/bundler/ruby/3.3.7/bin
-        let expected = Path::new("/home/user/project/.rb/vendor/bundler/ruby/3.3.7/bin");
+        // bin_dir should include Ruby minor version: .rb/vendor/bundler/ruby/3.3.0/bin
+        let expected = Path::new("/home/user/project/.rb/vendor/bundler/ruby/3.3.0/bin");
         assert_eq!(br.bin_dir(), expected);
     }
 
@@ -432,10 +431,10 @@ mod tests {
         // Should be configured since we created vendor structure
         assert!(br.is_configured());
 
-        // bin_dir should include Ruby version path
-        let expected_bin = br.vendor_dir().join("ruby").join("3.3.7").join("bin");
-        // gem_dir should be the Ruby-specific vendor directory
-        let expected_gem = br.vendor_dir().join("ruby").join("3.3.7");
+        // bin_dir should include Ruby minor version path (X.Y.0)
+        let expected_bin = br.vendor_dir().join("ruby").join("3.3.0").join("bin");
+        // gem_dir should be the Ruby-minor-specific vendor directory
+        let expected_gem = br.vendor_dir().join("ruby").join("3.3.0");
 
         assert_eq!(
             <BundlerRuntime as RuntimeProvider>::bin_dir(&br),
diff --git a/crates/rb-core/src/butler/mod.rs b/crates/rb-core/src/butler/mod.rs
@@ -129,6 +129,29 @@ impl ButlerRuntime {
             ButlerError::General(format!("Unable to determine current directory: {}", e))
         })?;
 
+        Self::discover_and_compose_with_current_dir(
+            rubies_dir,
+            requested_ruby_version,
+            gem_base_dir,
+            skip_bundler,
+            current_dir,
+        )
+    }
+
+    /// Internal method: Perform comprehensive environment discovery with explicit current directory
+    ///
+    /// This method accepts the current directory as a parameter instead of reading it from
+    /// the environment, which makes it suitable for testing without global state mutation.
+    ///
+    /// Note: This method is primarily intended for testing but is made public to allow
+    /// flexible usage patterns where the current directory needs to be explicitly controlled.
+    pub fn discover_and_compose_with_current_dir(
+        rubies_dir: PathBuf,
+        requested_ruby_version: Option<String>,
+        gem_base_dir: Option<PathBuf>,
+        skip_bundler: bool,
+        current_dir: PathBuf,
+    ) -> Result<Self, ButlerError> {
         debug!("Starting comprehensive environment discovery");
         debug!("Rubies directory: {}", rubies_dir.display());
         debug!("Current directory: {}", current_dir.display());
@@ -197,18 +220,18 @@ impl ButlerRuntime {
             bundler_root.map(|root| BundlerRuntime::new(root, selected_ruby.version.clone()));
 
         // Step 6: Create gem runtime (using custom base directory if provided)
-        // IMPORTANT: When in bundler context, user gems are NOT available for isolation
-        let gem_runtime = if bundler_runtime.is_some() {
+        // IMPORTANT: Custom gem base (-G flag) takes precedence over bundler isolation
+        let gem_runtime = if let Some(ref custom_gem_base) = gem_base_dir {
             debug!(
-                "Bundler context detected - user gems will NOT be available (bundler isolation)"
-            );
-            None
-        } else if let Some(ref custom_gem_base) = gem_base_dir {
-            debug!(
-                "Using custom gem base directory: {}",
+                "Using custom gem base directory (overrides bundler isolation): {}",
                 custom_gem_base.display()
             );
             Some(selected_ruby.gem_runtime_for_base(custom_gem_base))
+        } else if bundler_runtime.is_some() {
+            debug!(
+                "Bundler context detected - user gems will NOT be available (bundler isolation)"
+            );
+            None
         } else {
             match selected_ruby.infer_gem_runtime() {
                 Ok(gem_runtime) => {
diff --git a/crates/rb-core/src/ruby/mod.rs b/crates/rb-core/src/ruby/mod.rs
@@ -6,9 +6,12 @@ use std::env::consts::EXE_SUFFIX;
 use std::path::{Path, PathBuf};
 
 pub mod version_detector;
+pub mod version_ext;
+
 pub use version_detector::{
     CompositeDetector, GemfileDetector, RubyVersionDetector, RubyVersionFileDetector,
 };
+pub use version_ext::RubyVersionExt;
 
 /// Errors that can occur during Ruby discovery
 #[derive(Debug, Clone)]
diff --git a/crates/rb-core/src/ruby/version_detector/mod.rs b/crates/rb-core/src/ruby/version_detector/mod.rs
@@ -26,7 +26,7 @@
 //! # Usage
 //!
 //! For standard Ruby projects:
-//! ```rust,ignore
+//! ```text
 //! use rb_core::ruby::CompositeDetector;
 //!
 //! let detector = CompositeDetector::standard();
@@ -36,7 +36,7 @@
 //! ```
 //!
 //! For bundler-managed projects:
-//! ```rust,ignore
+//! ```text
 //! let detector = CompositeDetector::bundler();
 //! let version = detector.detect(bundler_root);
 //! ```
@@ -46,7 +46,7 @@
 //! To add support for new version sources (e.g., `.tool-versions` for asdf):
 //!
 //! 1. Implement the `RubyVersionDetector` trait:
-//!    ```rust,ignore
+//!    ```text
 //!    pub struct ToolVersionsDetector;
 //!    impl RubyVersionDetector for ToolVersionsDetector {
 //!        fn detect(&self, context: &Path) -> Option<Version> {
@@ -57,7 +57,7 @@
 //!    ```
 //!
 //! 2. Add to the detector chain:
-//!    ```rust,ignore
+//!    ```text
 //!    CompositeDetector {
 //!        detectors: vec![
 //!            Box::new(RubyVersionFileDetector),
@@ -66,8 +66,6 @@
 //!        ]
 //!    }
 //!    ```
-//!
-//! See `tool_versions.rs` for a complete example implementation.
 
 use log::debug;
 use semver::Version;
diff --git a/crates/rb-core/src/ruby/version_ext.rs b/crates/rb-core/src/ruby/version_ext.rs
diff --git a/crates/rb-core/tests/bundler_integration_tests.rs b/crates/rb-core/tests/bundler_integration_tests.rs
diff --git a/crates/rb-core/tests/butler_integration_tests.rs b/crates/rb-core/tests/butler_integration_tests.rs
