diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index e9cb8e55..cc098daf 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -744,6 +744,46 @@ User Input → CLI Parser → Mode Detection → Node Resolution - **Timeout handling**: Configurable per-connection and per-command - **Signal handling**: Clean shutdown on Ctrl+C with two-stage confirmation +### Test Environment-Variable Mutation Pattern (`EnvGuard`) + +Several test suites must temporarily set or remove process-wide environment +variables (e.g. `BACKENDAI_CLUSTER_HOSTS`, `HOME`, `SSH_AUTH_SOCK`). Under +Rust 2024 edition, `std::env::set_var` and `std::env::remove_var` are marked +`unsafe` because concurrent mutation of the environment is undefined behaviour +at the libc level on glibc, musl, and macOS. `EnvGuard` centralises all such +mutations in `src/test_helpers/env_guard.rs`. + +**Soundness contract**: every test that constructs an `EnvGuard` MUST be +annotated with `#[serial_test::serial]`. Every other test in the same crate +binary that reads or mutates the same variable MUST also carry `#[serial]` (or +a matching `#[serial(key)]` group). Note that `#[serial]` only serializes +against other `#[serial]` / `#[parallel]` tests — unannotated tests may still +run concurrently with serial ones and would race on environment reads. This is +not an `EnvGuard` limitation; it is an inherent constraint of the libc +environment-variable API. + +```rust +use serial_test::serial; +use crate::test_helpers::EnvGuard; + +#[test] +#[serial] +fn my_test() { + let _host = EnvGuard::set("BACKENDAI_CLUSTER_HOSTS", "node1,node2"); + // Variable is automatically restored when `_host` drops at end of scope. +} +``` + +**Integration tests** access the same struct via a `#[path]`-based re-export +in `tests/common/mod.rs`, which avoids making `EnvGuard` part of the public +`bssh` crate API while keeping a single source of truth. When adding a new +integration-test binary that needs `EnvGuard`, add `mod common;` at the top of +that file and use `common::EnvGuard`. + +Use `#[serial(key)]` (a named group) when two sets of tests touch different, +non-overlapping variables and can therefore run concurrently with each other +but not with themselves; omit the key (plain `#[serial]`) when in doubt. + ## Security Model ### Authentication diff --git a/src/cli/mode_detection_tests.rs b/src/cli/mode_detection_tests.rs index 339f6ead..d30e43e6 100644 --- a/src/cli/mode_detection_tests.rs +++ b/src/cli/mode_detection_tests.rs @@ -20,6 +20,7 @@ #[cfg(test)] mod tests { use crate::cli::pdsh::PDSH_COMPAT_ENV_VAR; + use crate::test_helpers::EnvGuard; use serial_test::serial; use std::env; @@ -27,40 +28,24 @@ mod tests { #[test] #[serial] fn test_env_var_detection_one() { - // Save and restore env var state - let original = env::var(PDSH_COMPAT_ENV_VAR).ok(); - - env::set_var(PDSH_COMPAT_ENV_VAR, "1"); + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, "1"); // Create a test for the env var checking logic let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); assert!(value.is_some()); let value = value.unwrap(); assert!(value == "1" || value.to_lowercase() == "true"); - - // Restore - match original { - Some(v) => env::set_var(PDSH_COMPAT_ENV_VAR, v), - None => env::remove_var(PDSH_COMPAT_ENV_VAR), - } } /// Test that environment variable detection works for "true" #[test] #[serial] fn test_env_var_detection_true() { - let original = env::var(PDSH_COMPAT_ENV_VAR).ok(); - - env::set_var(PDSH_COMPAT_ENV_VAR, "true"); + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, "true"); let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); assert!(value.is_some()); assert_eq!(value.unwrap().to_lowercase(), "true"); - - match original { - Some(v) => env::set_var(PDSH_COMPAT_ENV_VAR, v), - None => env::remove_var(PDSH_COMPAT_ENV_VAR), - } } /// Test that environment variable detection works for "TRUE" (case insensitive) @@ -84,47 +69,38 @@ mod tests { #[test] #[serial] fn test_env_var_not_set() { - let original = env::var(PDSH_COMPAT_ENV_VAR).ok(); - - env::remove_var(PDSH_COMPAT_ENV_VAR); + let _guard = EnvGuard::remove(PDSH_COMPAT_ENV_VAR); let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); assert!(value.is_none()); - - // Restore - if let Some(v) = original { - env::set_var(PDSH_COMPAT_ENV_VAR, v); - } } /// Test that invalid env var values are not treated as enabled #[test] #[serial] fn test_env_var_invalid_values() { - let original = env::var(PDSH_COMPAT_ENV_VAR).ok(); - // Test "0" - env::set_var(PDSH_COMPAT_ENV_VAR, "0"); - let value = env::var(PDSH_COMPAT_ENV_VAR).unwrap(); - let enabled = value == "1" || value.to_lowercase() == "true"; - assert!(!enabled); + { + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, "0"); + let value = env::var(PDSH_COMPAT_ENV_VAR).unwrap(); + let enabled = value == "1" || value.to_lowercase() == "true"; + assert!(!enabled); + } // Test "false" - env::set_var(PDSH_COMPAT_ENV_VAR, "false"); - let value = env::var(PDSH_COMPAT_ENV_VAR).unwrap(); - let enabled = value == "1" || value.to_lowercase() == "true"; - assert!(!enabled); + { + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, "false"); + let value = env::var(PDSH_COMPAT_ENV_VAR).unwrap(); + let enabled = value == "1" || value.to_lowercase() == "true"; + assert!(!enabled); + } // Test empty string - env::set_var(PDSH_COMPAT_ENV_VAR, ""); - let value = env::var(PDSH_COMPAT_ENV_VAR).unwrap(); - let enabled = value == "1" || value.to_lowercase() == "true"; - assert!(!enabled); - - // Restore - match original { - Some(v) => env::set_var(PDSH_COMPAT_ENV_VAR, v), - None => env::remove_var(PDSH_COMPAT_ENV_VAR), + { + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, ""); + let value = env::var(PDSH_COMPAT_ENV_VAR).unwrap(); + let enabled = value == "1" || value.to_lowercase() == "true"; + assert!(!enabled); } } diff --git a/src/cli/pdsh.rs b/src/cli/pdsh.rs index 7516214c..b41843da 100644 --- a/src/cli/pdsh.rs +++ b/src/cli/pdsh.rs @@ -141,12 +141,11 @@ pub const PDSH_COMPAT_ENV_VAR: &str = "BSSH_PDSH_COMPAT"; /// /// # Examples /// -/// ``` -/// use std::env; -/// -/// // When environment variable is set -/// env::set_var("BSSH_PDSH_COMPAT", "1"); -/// // is_pdsh_compat_mode() would return true +/// ```ignore +/// // When BSSH_PDSH_COMPAT=1 is set in the environment, +/// // is_pdsh_compat_mode() returns true. +/// // (Example is marked `ignore` because mutating environment variables in +/// // Rust 2024 requires `unsafe`; see `EnvGuard` in `src/test_helpers`.) /// ``` pub fn is_pdsh_compat_mode() -> bool { // Check environment variable first diff --git a/src/config/tests.rs b/src/config/tests.rs index 6e3e7e29..b50d5fc8 100644 --- a/src/config/tests.rs +++ b/src/config/tests.rs @@ -18,13 +18,14 @@ use std::path::{Path, PathBuf}; use super::types::{Config, InteractiveMode, NodeConfig}; use super::utils::{expand_env_vars, expand_tilde}; +use crate::test_helpers::EnvGuard; +use serial_test::serial; #[test] +#[serial] fn test_expand_env_vars() { - unsafe { - std::env::set_var("TEST_VAR", "test_value"); - std::env::set_var("TEST_USER", "testuser"); - } + let _test_var = EnvGuard::set("TEST_VAR", "test_value"); + let _test_user = EnvGuard::set("TEST_USER", "testuser"); // Test ${VAR} syntax assert_eq!(expand_env_vars("Hello ${TEST_VAR}!"), "Hello test_value!"); @@ -49,23 +50,13 @@ fn test_expand_env_vars() { } #[test] +#[serial] fn test_expand_tilde() { - // Save original HOME value - let original_home = std::env::var("HOME").ok(); - - // Set test HOME value - std::env::set_var("HOME", "/home/user"); + let _home = EnvGuard::set("HOME", "/home/user"); let path = Path::new("~/.ssh/config"); let expanded = expand_tilde(path); - // Restore original HOME value - if let Some(home) = original_home { - std::env::set_var("HOME", home); - } else { - std::env::remove_var("HOME"); - } - assert_eq!(expanded, PathBuf::from("/home/user/.ssh/config")); } @@ -194,14 +185,13 @@ clusters: } #[test] +#[serial] fn test_backendai_env_parsing() { - // Set up Backend.AI environment variables - unsafe { - std::env::set_var("BACKENDAI_CLUSTER_HOSTS", "sub1,main1"); - std::env::set_var("BACKENDAI_CLUSTER_HOST", "main1"); - std::env::set_var("BACKENDAI_CLUSTER_ROLE", "main"); - std::env::set_var("USER", "testuser"); - } + // Set up Backend.AI environment variables; guards restore prior values on drop. + let _hosts = EnvGuard::set("BACKENDAI_CLUSTER_HOSTS", "sub1,main1"); + let _host = EnvGuard::set("BACKENDAI_CLUSTER_HOST", "main1"); + let _role = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "main"); + let _user = EnvGuard::set("USER", "testuser"); let cluster = Config::from_backendai_env().unwrap(); @@ -216,10 +206,11 @@ fn test_backendai_env_parsing() { _ => panic!("Expected Simple node config"), } - // Test with sub role - should skip the first (main) node - unsafe { - std::env::set_var("BACKENDAI_CLUSTER_ROLE", "sub"); - } + // Test with sub role - should skip the first (main) node. + // The new guard shadows the outer `_role` binding; when it drops at the end + // of this test it'll restore to "main" (the value saved by `_role`), and + // then the outer `_role` drops and restores to the pre-test value. + let _role_sub = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "sub"); let cluster = Config::from_backendai_env().unwrap(); assert_eq!(cluster.nodes.len(), 1); @@ -229,13 +220,6 @@ fn test_backendai_env_parsing() { } _ => panic!("Expected Simple node config"), } - - // Clean up - unsafe { - std::env::remove_var("BACKENDAI_CLUSTER_HOSTS"); - std::env::remove_var("BACKENDAI_CLUSTER_HOST"); - std::env::remove_var("BACKENDAI_CLUSTER_ROLE"); - } } #[test] @@ -465,12 +449,11 @@ clusters: } #[test] +#[serial] fn test_jump_host_env_var_expansion() { - // Set up test environment variables - unsafe { - std::env::set_var("TEST_BASTION_HOST", "bastion.example.com"); - std::env::set_var("TEST_BASTION_PORT", "2222"); - } + // Set up test environment variables; guards restore prior values on drop. + let _host = EnvGuard::set("TEST_BASTION_HOST", "bastion.example.com"); + let _port = EnvGuard::set("TEST_BASTION_PORT", "2222"); let yaml = r#" defaults: @@ -504,12 +487,6 @@ clusters: config.get_cluster_jump_host(Some("staging")), Some("bastion.example.com".to_string()) ); - - // Clean up - unsafe { - std::env::remove_var("TEST_BASTION_HOST"); - std::env::remove_var("TEST_BASTION_PORT"); - } } #[test] diff --git a/src/executor/rank_detector.rs b/src/executor/rank_detector.rs index 974e37bb..6a7a3f5f 100644 --- a/src/executor/rank_detector.rs +++ b/src/executor/rank_detector.rs @@ -101,6 +101,7 @@ impl RankDetector { #[cfg(test)] mod tests { use super::*; + use crate::test_helpers::EnvGuard; use serial_test::serial; #[test] @@ -113,8 +114,8 @@ mod tests { #[serial] fn test_fallback_to_first_node() { // Clear Backend.AI env vars to test fallback behavior - env::remove_var("BACKENDAI_CLUSTER_ROLE"); - env::remove_var("BACKENDAI_CLUSTER_HOST"); + let _role = EnvGuard::remove("BACKENDAI_CLUSTER_ROLE"); + let _host = EnvGuard::remove("BACKENDAI_CLUSTER_HOST"); let nodes = vec![ Node::new("host1".to_string(), 22, "user".to_string()), @@ -129,36 +130,27 @@ mod tests { #[test] #[serial] fn test_backendai_role_detection() { - // Set environment variable - env::set_var("BACKENDAI_CLUSTER_ROLE", "main"); + // Set environment variable; guard restores on drop. + let _role = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "main"); assert!(RankDetector::is_backendai_main()); - - // Cleanup - env::remove_var("BACKENDAI_CLUSTER_ROLE"); } #[test] #[serial] fn test_backendai_role_case_insensitive() { - env::set_var("BACKENDAI_CLUSTER_ROLE", "MAIN"); + let _role_upper = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "MAIN"); assert!(RankDetector::is_backendai_main()); - env::set_var("BACKENDAI_CLUSTER_ROLE", "Main"); + let _role_mixed = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "Main"); assert!(RankDetector::is_backendai_main()); - - // Cleanup - env::remove_var("BACKENDAI_CLUSTER_ROLE"); } #[test] #[serial] fn test_backendai_role_non_main() { - env::set_var("BACKENDAI_CLUSTER_ROLE", "sub"); + let _role = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "sub"); assert!(!RankDetector::is_backendai_main()); - - // Cleanup - env::remove_var("BACKENDAI_CLUSTER_ROLE"); } #[test] @@ -170,16 +162,12 @@ mod tests { Node::new("host3".to_string(), 22, "user".to_string()), ]; - // Set Backend.AI environment - env::set_var("BACKENDAI_CLUSTER_ROLE", "main"); - env::set_var("BACKENDAI_CLUSTER_HOST", "host2"); + // Set Backend.AI environment; guards restore on drop. + let _role = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "main"); + let _host = EnvGuard::set("BACKENDAI_CLUSTER_HOST", "host2"); // Should find host2 at index 1 assert_eq!(RankDetector::identify_main_rank(&nodes), Some(1)); - - // Cleanup - env::remove_var("BACKENDAI_CLUSTER_ROLE"); - env::remove_var("BACKENDAI_CLUSTER_HOST"); } #[test] @@ -190,16 +178,12 @@ mod tests { Node::new("host2".to_string(), 22, "user".to_string()), ]; - // Set Backend.AI environment with non-existent host - env::set_var("BACKENDAI_CLUSTER_ROLE", "main"); - env::set_var("BACKENDAI_CLUSTER_HOST", "nonexistent"); + // Set Backend.AI environment with non-existent host; guards restore on drop. + let _role = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "main"); + let _host = EnvGuard::set("BACKENDAI_CLUSTER_HOST", "nonexistent"); // Should fallback to first node assert_eq!(RankDetector::identify_main_rank(&nodes), Some(0)); - - // Cleanup - env::remove_var("BACKENDAI_CLUSTER_ROLE"); - env::remove_var("BACKENDAI_CLUSTER_HOST"); } #[test] diff --git a/src/jump/chain/auth.rs b/src/jump/chain/auth.rs index 4cfbf97d..f7c4f227 100644 --- a/src/jump/chain/auth.rs +++ b/src/jump/chain/auth.rs @@ -449,6 +449,7 @@ pub(super) async fn authenticate_connection( #[cfg(test)] mod tests { use super::*; + use crate::test_helpers::EnvGuard; use std::env; use tempfile::TempDir; @@ -487,40 +488,27 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= #[tokio::test] #[serial_test::serial] async fn test_agent_available_false_when_no_socket() { - // Save and clear SSH_AUTH_SOCK - let original = env::var("SSH_AUTH_SOCK").ok(); - env::remove_var("SSH_AUTH_SOCK"); + let _sock = EnvGuard::remove("SSH_AUTH_SOCK"); // Verify SSH_AUTH_SOCK is not set assert!(env::var("SSH_AUTH_SOCK").is_err()); // The agent_available logic in determine_auth_method checks this - let agent_available = if env::var("SSH_AUTH_SOCK").is_ok() { - true // Would call agent_has_identities() in real code - } else { - false - }; + let agent_available = env::var("SSH_AUTH_SOCK").is_ok(); assert!( !agent_available, "agent_available should be false when SSH_AUTH_SOCK is not set" ); - - // Restore SSH_AUTH_SOCK - if let Some(val) = original { - env::set_var("SSH_AUTH_SOCK", val); - } } /// Test: When SSH_AUTH_SOCK points to invalid path, agent_has_identities returns false #[tokio::test] #[cfg(not(target_os = "windows"))] + #[serial_test::serial] async fn test_agent_has_identities_invalid_socket() { - // Save original value - let original = env::var("SSH_AUTH_SOCK").ok(); - - // Set to a non-existent path - env::set_var("SSH_AUTH_SOCK", "/tmp/nonexistent_ssh_agent_socket_12345"); + // Set to a non-existent path; guard restores prior value on drop. + let _sock = EnvGuard::set("SSH_AUTH_SOCK", "/tmp/nonexistent_ssh_agent_socket_12345"); // agent_has_identities should return false (connection will fail) let result = agent_has_identities().await; @@ -528,20 +516,14 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= !result, "agent_has_identities should return false for invalid socket" ); - - // Restore original value - match original { - Some(val) => env::set_var("SSH_AUTH_SOCK", val), - None => env::remove_var("SSH_AUTH_SOCK"), - } } /// Test: determine_auth_method falls back to key file when agent is unavailable #[tokio::test] + #[serial_test::serial] async fn test_determine_auth_method_fallback_to_key_file() { - // Save and clear SSH_AUTH_SOCK to ensure agent is "unavailable" - let original = env::var("SSH_AUTH_SOCK").ok(); - env::remove_var("SSH_AUTH_SOCK"); + // Clear SSH_AUTH_SOCK to ensure agent is "unavailable"; guard restores on drop. + let _sock = EnvGuard::remove("SSH_AUTH_SOCK"); let temp_dir = TempDir::new().expect("Failed to create temp dir"); let key_path = create_test_ssh_key(&temp_dir, "id_test"); @@ -573,11 +555,6 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= panic!("Unexpected auth method: {:?}", other); } } - - // Restore SSH_AUTH_SOCK - if let Some(val) = original { - env::set_var("SSH_AUTH_SOCK", val); - } } /// Test: determine_auth_method returns Agent when use_agent=true and agent is available @@ -633,10 +610,10 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= /// Test: determine_auth_method falls back to default keys when no key_path provided #[tokio::test] + #[serial_test::serial] async fn test_determine_auth_method_tries_default_keys() { - // Save and clear SSH_AUTH_SOCK - let original_sock = env::var("SSH_AUTH_SOCK").ok(); - env::remove_var("SSH_AUTH_SOCK"); + // Clear SSH_AUTH_SOCK; guard restores on drop. + let _sock = EnvGuard::remove("SSH_AUTH_SOCK"); // Create a temporary HOME directory with an SSH key let temp_home = TempDir::new().expect("Failed to create temp home"); @@ -653,9 +630,8 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= -----END OPENSSH PRIVATE KEY-----"#; std::fs::write(ssh_dir.join("id_ed25519"), key_content).expect("Failed to write key"); - // Save and set HOME - let original_home = env::var("HOME").ok(); - env::set_var("HOME", temp_home.path()); + // Set HOME; guard restores on drop. + let _home = EnvGuard::set("HOME", temp_home.path()); let jump_host = create_test_jump_host(); let auth_mutex = Mutex::new(()); @@ -688,14 +664,6 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= panic!("Expected PrivateKeyFile, got {:?}", other); } } - - // Restore environment - if let Some(val) = original_sock { - env::set_var("SSH_AUTH_SOCK", val); - } - if let Some(val) = original_home { - env::set_var("HOME", val); - } } /// Test: determine_auth_method fails when no authentication method is available @@ -703,13 +671,10 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= #[tokio::test] #[serial_test::serial] async fn test_determine_auth_method_fails_when_no_method_available() { - // Save original environment values - let original_sock = env::var("SSH_AUTH_SOCK").ok(); - let original_home = env::var("HOME").ok(); - - // Set SSH_AUTH_SOCK to an invalid path to ensure agent is "unavailable" - // Using remove_var alone isn't reliable in parallel test execution - env::set_var( + // Set SSH_AUTH_SOCK to an invalid path to ensure agent is "unavailable"; + // using remove_var alone isn't reliable in parallel test execution. + // Guards restore prior values on drop. + let _sock = EnvGuard::set( "SSH_AUTH_SOCK", "/nonexistent/path/to/agent/socket/test_12345", ); @@ -720,7 +685,7 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= std::fs::create_dir_all(&ssh_dir).expect("Failed to create .ssh dir"); // Don't create any keys - the .ssh dir is empty - env::set_var("HOME", temp_home.path()); + let _home = EnvGuard::set("HOME", temp_home.path()); let jump_host = create_test_jump_host(); let auth_mutex = Mutex::new(()); @@ -735,15 +700,6 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= ) .await; - // Restore environment BEFORE assertions to ensure cleanup happens - match original_sock { - Some(val) => env::set_var("SSH_AUTH_SOCK", val), - None => env::remove_var("SSH_AUTH_SOCK"), - } - if let Some(val) = original_home { - env::set_var("HOME", val); - } - // Now check the result // Note: Due to race conditions with parallel tests and environment variables, // the function may find keys from the real HOME directory before the env var @@ -814,10 +770,10 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= /// Test: Jump host's own ssh_key takes priority over cluster key_path #[tokio::test] + #[serial_test::serial] async fn test_jump_host_ssh_key_priority() { - // Save and clear SSH_AUTH_SOCK - let original_sock = env::var("SSH_AUTH_SOCK").ok(); - env::remove_var("SSH_AUTH_SOCK"); + // Clear SSH_AUTH_SOCK; guard restores on drop. + let _sock = EnvGuard::remove("SSH_AUTH_SOCK"); let temp_dir = TempDir::new().expect("Failed to create temp dir"); @@ -868,19 +824,14 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= panic!("Expected PrivateKeyFile, got {:?}", other); } } - - // Restore SSH_AUTH_SOCK - if let Some(val) = original_sock { - env::set_var("SSH_AUTH_SOCK", val); - } } /// Test: Falls back to cluster key when jump host has no ssh_key #[tokio::test] + #[serial_test::serial] async fn test_fallback_to_cluster_key() { - // Save and clear SSH_AUTH_SOCK - let original_sock = env::var("SSH_AUTH_SOCK").ok(); - env::remove_var("SSH_AUTH_SOCK"); + // Clear SSH_AUTH_SOCK; guard restores on drop. + let _sock = EnvGuard::remove("SSH_AUTH_SOCK"); let temp_dir = TempDir::new().expect("Failed to create temp dir"); let cluster_key_path = create_test_ssh_key(&temp_dir, "cluster_key"); @@ -919,10 +870,5 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= panic!("Expected PrivateKeyFile, got {:?}", other); } } - - // Restore SSH_AUTH_SOCK - if let Some(val) = original_sock { - env::set_var("SSH_AUTH_SOCK", val); - } } } diff --git a/src/jump/parser/tests.rs b/src/jump/parser/tests.rs index 62a87df0..14f36c2e 100644 --- a/src/jump/parser/tests.rs +++ b/src/jump/parser/tests.rs @@ -14,6 +14,7 @@ use super::host_parser::parse_single_jump_host; use super::*; +use crate::test_helpers::EnvGuard; #[test] fn test_parse_single_jump_host_hostname_only() { @@ -174,8 +175,7 @@ fn test_jump_host_effective_values() { #[test] #[serial_test::serial] fn test_max_jump_hosts_limit_exactly_10() { - // Clear any environment variable first - std::env::remove_var("BSSH_MAX_JUMP_HOSTS"); + let _guard = EnvGuard::remove("BSSH_MAX_JUMP_HOSTS"); // Exactly 10 jump hosts should be allowed let spec = (0..10) @@ -190,8 +190,7 @@ fn test_max_jump_hosts_limit_exactly_10() { #[test] #[serial_test::serial] fn test_max_jump_hosts_limit_11_rejected() { - // Clear any environment variable first - std::env::remove_var("BSSH_MAX_JUMP_HOSTS"); + let _guard = EnvGuard::remove("BSSH_MAX_JUMP_HOSTS"); // 11 jump hosts should be rejected let spec = (0..11) @@ -240,7 +239,7 @@ fn test_max_jump_hosts_limit_excessive() { #[serial_test::serial] fn test_get_max_jump_hosts_default() { // Without environment variable, should return default (10) - std::env::remove_var("BSSH_MAX_JUMP_HOSTS"); + let _guard = EnvGuard::remove("BSSH_MAX_JUMP_HOSTS"); let max = get_max_jump_hosts(); assert_eq!(max, 10, "Default should be 10"); } @@ -249,68 +248,46 @@ fn test_get_max_jump_hosts_default() { #[serial_test::serial] fn test_get_max_jump_hosts_custom_value() { // Set environment variable to custom value - unsafe { - std::env::set_var("BSSH_MAX_JUMP_HOSTS", "15"); - } + let _guard = EnvGuard::set("BSSH_MAX_JUMP_HOSTS", "15"); let max = get_max_jump_hosts(); assert_eq!(max, 15, "Should use custom value from environment"); - - // Cleanup - std::env::remove_var("BSSH_MAX_JUMP_HOSTS"); } #[test] #[serial_test::serial] fn test_get_max_jump_hosts_capped_at_absolute_max() { // Set environment variable beyond absolute maximum (30) - unsafe { - std::env::set_var("BSSH_MAX_JUMP_HOSTS", "50"); - } + let _guard = EnvGuard::set("BSSH_MAX_JUMP_HOSTS", "50"); let max = get_max_jump_hosts(); assert_eq!( max, 30, "Should be capped at absolute maximum of 30 for security" ); - - // Cleanup - std::env::remove_var("BSSH_MAX_JUMP_HOSTS"); } #[test] #[serial_test::serial] fn test_get_max_jump_hosts_zero_falls_back() { // Zero is invalid, should fall back to default - unsafe { - std::env::set_var("BSSH_MAX_JUMP_HOSTS", "0"); - } + let _guard = EnvGuard::set("BSSH_MAX_JUMP_HOSTS", "0"); let max = get_max_jump_hosts(); assert_eq!(max, 10, "Zero should fall back to default (10)"); - - // Cleanup - std::env::remove_var("BSSH_MAX_JUMP_HOSTS"); } #[test] #[serial_test::serial] fn test_get_max_jump_hosts_invalid_value() { // Invalid value should fall back to default - unsafe { - std::env::set_var("BSSH_MAX_JUMP_HOSTS", "invalid"); - } + let _guard = EnvGuard::set("BSSH_MAX_JUMP_HOSTS", "invalid"); let max = get_max_jump_hosts(); assert_eq!(max, 10, "Invalid value should fall back to default (10)"); - - // Cleanup - std::env::remove_var("BSSH_MAX_JUMP_HOSTS"); } #[test] #[serial_test::serial] fn test_max_jump_hosts_respects_environment() { // Set custom limit via environment variable - unsafe { - std::env::set_var("BSSH_MAX_JUMP_HOSTS", "15"); - } + let _guard = EnvGuard::set("BSSH_MAX_JUMP_HOSTS", "15"); // Create spec with 15 hosts (should succeed) let spec_15 = (0..15) @@ -334,7 +311,4 @@ fn test_max_jump_hosts_respects_environment() { result.is_err(), "Should reject 16 hosts when BSSH_MAX_JUMP_HOSTS=15" ); - - // Cleanup - std::env::remove_var("BSSH_MAX_JUMP_HOSTS"); } diff --git a/src/lib.rs b/src/lib.rs index d72e6f85..ae49660a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,6 +29,9 @@ pub mod ssh; pub mod ui; pub mod utils; +#[cfg(test)] +pub(crate) mod test_helpers; + pub use cli::Cli; pub use config::Config; pub use executor::ParallelExecutor; diff --git a/src/security/sudo.rs b/src/security/sudo.rs index 162b0268..a19b63f8 100644 --- a/src/security/sudo.rs +++ b/src/security/sudo.rs @@ -237,6 +237,7 @@ pub fn get_sudo_password(warn_env: bool) -> Result { #[cfg(test)] mod tests { use super::*; + use crate::test_helpers::EnvGuard; use serial_test::serial; #[test] @@ -325,12 +326,9 @@ mod tests { #[test] #[serial] fn test_get_sudo_password_from_env_empty() { - // Ensure variable is not set from other tests - std::env::remove_var("BSSH_SUDO_PASSWORD"); - // Set environment variable to empty string - std::env::set_var("BSSH_SUDO_PASSWORD", ""); + // Set environment variable to empty string; guard restores prior value on drop. + let _guard = EnvGuard::set("BSSH_SUDO_PASSWORD", ""); let result = get_sudo_password_from_env(); - std::env::remove_var("BSSH_SUDO_PASSWORD"); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("empty")); @@ -339,12 +337,9 @@ mod tests { #[test] #[serial] fn test_get_sudo_password_from_env_valid() { - // Ensure variable is not set from other tests - std::env::remove_var("BSSH_SUDO_PASSWORD"); - // Set environment variable to valid password - std::env::set_var("BSSH_SUDO_PASSWORD", "test_password"); + // Set environment variable to valid password; guard restores prior value on drop. + let _guard = EnvGuard::set("BSSH_SUDO_PASSWORD", "test_password"); let result = get_sudo_password_from_env(); - std::env::remove_var("BSSH_SUDO_PASSWORD"); assert!(result.is_ok()); let password = result.unwrap(); @@ -355,7 +350,7 @@ mod tests { #[test] #[serial] fn test_get_sudo_password_from_env_not_set() { - std::env::remove_var("BSSH_SUDO_PASSWORD"); + let _guard = EnvGuard::remove("BSSH_SUDO_PASSWORD"); let result = get_sudo_password_from_env(); assert!(result.is_ok()); diff --git a/src/server/config/loader.rs b/src/server/config/loader.rs index 03421a4f..7481d0fb 100644 --- a/src/server/config/loader.rs +++ b/src/server/config/loader.rs @@ -404,6 +404,7 @@ fn validate_config(config: &ServerFileConfig) -> Result<()> { #[cfg(test)] mod tests { use super::*; + use crate::test_helpers::EnvGuard; use std::io::Write; use tempfile::NamedTempFile; @@ -446,62 +447,45 @@ auth: #[test] #[serial_test::serial] fn test_env_override_port() { - // Clear any existing env vars - std::env::remove_var("BSSH_PORT"); - - std::env::set_var("BSSH_PORT", "3333"); + let _port = EnvGuard::set("BSSH_PORT", "3333"); let config = apply_env_overrides(ServerFileConfig::default()).unwrap(); assert_eq!(config.server.port, 3333); - std::env::remove_var("BSSH_PORT"); } #[test] #[serial_test::serial] fn test_env_override_bind_address() { - // Clear any existing env vars - std::env::remove_var("BSSH_PORT"); - - std::env::set_var("BSSH_BIND_ADDRESS", "192.168.1.1"); + let _port = EnvGuard::remove("BSSH_PORT"); + let _bind = EnvGuard::set("BSSH_BIND_ADDRESS", "192.168.1.1"); let config = apply_env_overrides(ServerFileConfig::default()).unwrap(); assert_eq!(config.server.bind_address, "192.168.1.1"); - std::env::remove_var("BSSH_BIND_ADDRESS"); } #[test] #[serial_test::serial] fn test_env_override_host_keys() { - // Clear any existing env vars - std::env::remove_var("BSSH_PORT"); - - std::env::set_var("BSSH_HOST_KEY", "/key1,/key2,/key3"); + let _port = EnvGuard::remove("BSSH_PORT"); + let _host_key = EnvGuard::set("BSSH_HOST_KEY", "/key1,/key2,/key3"); let config = apply_env_overrides(ServerFileConfig::default()).unwrap(); assert_eq!(config.server.host_keys.len(), 3); assert_eq!(config.server.host_keys[0], PathBuf::from("/key1")); - std::env::remove_var("BSSH_HOST_KEY"); } #[test] #[serial_test::serial] fn test_env_override_auth_methods() { - // Clear any existing env vars - std::env::remove_var("BSSH_PORT"); - - std::env::set_var("BSSH_AUTH_METHODS", "publickey,password"); + let _port = EnvGuard::remove("BSSH_PORT"); + let _methods = EnvGuard::set("BSSH_AUTH_METHODS", "publickey,password"); let config = apply_env_overrides(ServerFileConfig::default()).unwrap(); assert_eq!(config.auth.methods.len(), 2); - std::env::remove_var("BSSH_AUTH_METHODS"); } #[test] #[serial_test::serial] fn test_env_override_invalid_port() { - // Clear any existing env vars first - std::env::remove_var("BSSH_PORT"); - - std::env::set_var("BSSH_PORT", "invalid"); + let _port = EnvGuard::set("BSSH_PORT", "invalid"); let result = apply_env_overrides(ServerFileConfig::default()); assert!(result.is_err()); - std::env::remove_var("BSSH_PORT"); } #[test] diff --git a/src/ssh/auth.rs b/src/ssh/auth.rs index d2777aff..16f23c9c 100644 --- a/src/ssh/auth.rs +++ b/src/ssh/auth.rs @@ -737,6 +737,8 @@ impl AuthContext { #[cfg(test)] mod tests { use super::*; + use crate::test_helpers::EnvGuard; + use serial_test::serial; use tempfile::TempDir; #[tokio::test] @@ -856,9 +858,10 @@ mod tests { #[cfg(not(target_os = "windows"))] #[tokio::test] + #[serial] async fn test_agent_auth_with_invalid_socket() { - // Set SSH_AUTH_SOCK to non-existent path - std::env::set_var("SSH_AUTH_SOCK", "/tmp/nonexistent-ssh-agent.sock"); + // Set SSH_AUTH_SOCK to non-existent path; guard restores prior value on drop. + let _sock = EnvGuard::set("SSH_AUTH_SOCK", "/tmp/nonexistent-ssh-agent.sock"); let ctx = AuthContext::new("user".to_string(), "host".to_string()) .unwrap() @@ -867,9 +870,6 @@ mod tests { // Should return None since socket doesn't exist let auth = ctx.agent_auth().unwrap(); assert!(auth.is_none()); - - // Clean up - std::env::remove_var("SSH_AUTH_SOCK"); } #[tokio::test] @@ -886,20 +886,17 @@ mod tests { } #[tokio::test] + #[serial] async fn test_password_fallback_in_non_interactive() { - // Save original environment variables - let original_home = std::env::var("HOME").ok(); - let original_ssh_auth_sock = std::env::var("SSH_AUTH_SOCK").ok(); - // Create a fake home directory WITHOUT default keys (to trigger fallback) let temp_dir = TempDir::new().unwrap(); let ssh_dir = temp_dir.path().join(".ssh"); std::fs::create_dir_all(&ssh_dir).unwrap(); // Intentionally NOT creating any key files - // Set test environment - std::env::set_var("HOME", temp_dir.path().to_str().unwrap()); - std::env::remove_var("SSH_AUTH_SOCK"); + // Set test environment; guards restore prior values on drop. + let _home = EnvGuard::set("HOME", temp_dir.path().to_str().unwrap()); + let _sock = EnvGuard::remove("SSH_AUTH_SOCK"); let ctx = AuthContext::new("user".to_string(), "host".to_string()).unwrap(); @@ -910,15 +907,5 @@ mod tests { // Error message should mention authentication failure let error_msg = result.unwrap_err().to_string(); assert!(error_msg.contains("authentication")); - - // Restore original environment variables - if let Some(home) = original_home { - std::env::set_var("HOME", home); - } else { - std::env::remove_var("HOME"); - } - if let Some(sock) = original_ssh_auth_sock { - std::env::set_var("SSH_AUTH_SOCK", sock); - } } } diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index b5abb5a1..d4675a5c 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -274,6 +274,7 @@ impl SshClient { #[cfg(test)] mod tests { use super::*; + use crate::test_helpers::EnvGuard; use serial_test::serial; use tempfile::TempDir; @@ -310,10 +311,6 @@ mod tests { async fn test_determine_auth_method_with_agent() { use std::os::unix::net::UnixListener; - // Save original environment - let original_ssh_auth_sock = std::env::var("SSH_AUTH_SOCK").ok(); - let original_home = std::env::var("HOME").ok(); - // Create a temporary directory for the socket and SSH keys let temp_dir = TempDir::new().unwrap(); let socket_path = temp_dir.path().join("ssh-agent.sock"); @@ -329,8 +326,9 @@ mod tests { "-----BEGIN PRIVATE KEY-----\nfake key content\n-----END PRIVATE KEY-----"; std::fs::write(ssh_dir.join("id_rsa"), key_content).unwrap(); - std::env::set_var("SSH_AUTH_SOCK", socket_path.to_str().unwrap()); - std::env::set_var("HOME", temp_dir.path()); + // Guards restore prior values on drop. + let _sock = EnvGuard::set("SSH_AUTH_SOCK", socket_path.to_str().unwrap()); + let _home = EnvGuard::set("HOME", temp_dir.path()); let client = SshClient::new("test.com".to_string(), 22, "user".to_string()); let auth = client @@ -344,16 +342,6 @@ mod tests { .await .unwrap(); - // Restore original environment - if let Some(sock) = original_ssh_auth_sock { - std::env::set_var("SSH_AUTH_SOCK", sock); - } else { - std::env::remove_var("SSH_AUTH_SOCK"); - } - if let Some(home) = original_home { - std::env::set_var("HOME", home); - } - // With the agent identity check, if the agent has no identities (our fake socket), // it will fall back to key file authentication. Accept either outcome. match auth { @@ -373,10 +361,6 @@ mod tests { async fn test_determine_auth_method_with_agent() { use std::os::unix::net::UnixListener; - // Save original environment - let original_ssh_auth_sock = std::env::var("SSH_AUTH_SOCK").ok(); - let original_home = std::env::var("HOME").ok(); - // Create a temporary directory for the socket and SSH keys let temp_dir = TempDir::new().unwrap(); let socket_path = temp_dir.path().join("ssh-agent.sock"); @@ -392,8 +376,9 @@ mod tests { "-----BEGIN PRIVATE KEY-----\nfake key content\n-----END PRIVATE KEY-----"; std::fs::write(ssh_dir.join("id_rsa"), key_content).unwrap(); - std::env::set_var("SSH_AUTH_SOCK", socket_path.to_str().unwrap()); - std::env::set_var("HOME", temp_dir.path()); + // Guards restore prior values on drop. + let _sock = EnvGuard::set("SSH_AUTH_SOCK", socket_path.to_str().unwrap()); + let _home = EnvGuard::set("HOME", temp_dir.path()); let client = SshClient::new("test.com".to_string(), 22, "user".to_string()); let auth = client @@ -401,16 +386,6 @@ mod tests { .await .unwrap(); - // Restore original environment - if let Some(sock) = original_ssh_auth_sock { - std::env::set_var("SSH_AUTH_SOCK", sock); - } else { - std::env::remove_var("SSH_AUTH_SOCK"); - } - if let Some(home) = original_home { - std::env::set_var("HOME", home); - } - // With the agent identity check, if the agent has no identities (our fake socket), // it will fall back to key file authentication. Accept either outcome. match auth { @@ -436,10 +411,6 @@ mod tests { #[tokio::test] #[serial] async fn test_determine_auth_method_fallback_to_default() { - // Save original environment variables - let original_home = std::env::var("HOME").ok(); - let original_ssh_auth_sock = std::env::var("SSH_AUTH_SOCK").ok(); - // Create a fake home directory with default key let temp_dir = TempDir::new().unwrap(); let ssh_dir = temp_dir.path().join(".ssh"); @@ -447,9 +418,9 @@ mod tests { let default_key = ssh_dir.join("id_rsa"); std::fs::write(&default_key, "fake key").unwrap(); - // Set test environment - std::env::set_var("HOME", temp_dir.path().to_str().unwrap()); - std::env::remove_var("SSH_AUTH_SOCK"); + // Guards restore prior values on drop. + let _home = EnvGuard::set("HOME", temp_dir.path().to_str().unwrap()); + let _sock = EnvGuard::remove("SSH_AUTH_SOCK"); let client = SshClient::new("test.com".to_string(), 22, "user".to_string()); let auth = client @@ -463,16 +434,6 @@ mod tests { .await .unwrap(); - // Restore original environment variables - if let Some(home) = original_home { - std::env::set_var("HOME", home); - } else { - std::env::remove_var("HOME"); - } - if let Some(sock) = original_ssh_auth_sock { - std::env::set_var("SSH_AUTH_SOCK", sock); - } - match auth { AuthMethod::PrivateKeyFile { key_file_path, .. } => { // Path should be canonicalized now diff --git a/src/ssh/ssh_config/integration_tests/env_cache_integration_test.rs b/src/ssh/ssh_config/integration_tests/env_cache_integration_test.rs index e1b0aaeb..3a5e6333 100644 --- a/src/ssh/ssh_config/integration_tests/env_cache_integration_test.rs +++ b/src/ssh/ssh_config/integration_tests/env_cache_integration_test.rs @@ -21,6 +21,8 @@ use std::time::{Duration, Instant}; #[cfg(test)] mod tests { use super::*; + use crate::test_helpers::EnvGuard; + use serial_test::serial; #[test] fn test_path_expansion_uses_cache() { @@ -222,6 +224,7 @@ mod tests { } #[test] + #[serial] fn test_cache_ttl_behavior() { // Create cache with very short TTL let config = EnvCacheConfig { @@ -234,7 +237,7 @@ mod tests { // Use a custom environment variable for testing to avoid conflicts with other tests // that might modify HOME let test_var = "BSSH_TEST_CACHE_VAR"; - std::env::set_var(test_var, "test_value_12345"); + let _test_var_guard = EnvGuard::set(test_var, "test_value_12345"); // Add test variable to safe list for this test // Since we can't modify the safe list at runtime, we'll use USER which is safe @@ -271,7 +274,6 @@ mod tests { let final_stats = cache.stats(); assert!(final_stats.ttl_evictions > 0, "Should have TTL evictions"); - // Clean up test variable - std::env::remove_var(test_var); + // `_test_var_guard` dropped here -> test variable automatically removed. } } diff --git a/src/test_helpers/env_guard.rs b/src/test_helpers/env_guard.rs new file mode 100644 index 00000000..6f96d77e --- /dev/null +++ b/src/test_helpers/env_guard.rs @@ -0,0 +1,254 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! RAII wrapper for safe environment variable mutation in tests. +//! +//! All `unsafe` calls to `std::env::set_var` / `std::env::remove_var` are +//! encapsulated here. Combined with `#[serial_test::serial]`, this gives tests +//! a safe, cleanup-guaranteed way to manipulate process-wide environment +//! variables without scattering `unsafe {}` blocks across the codebase. +//! +//! ## Soundness contract +//! +//! `EnvGuard` consolidates the `unsafe` env mutation that `std::env::set_var` +//! and `std::env::remove_var` require under Rust 2024. The `unsafe` blocks +//! inside this module are sound only when callers uphold the following +//! contract: every test that constructs an `EnvGuard` MUST be annotated +//! with `#[serial_test::serial]`, AND every other test in the same crate +//! that reads or mutates the same environment variable MUST also be +//! `#[serial]` (or share a matching `#[serial(key)]` group). Non-serial +//! tests racing against `EnvGuard` mutations are undefined behaviour at the +//! libc level on glibc/musl/macOS. +//! +//! Note: `#[serial]` only serializes against other `#[serial]` and +//! `#[parallel]` tests. Tests with neither attribute may run concurrently +//! with serial tests and are not covered by the serial ordering guarantee. +//! +//! # Usage +//! +//! ```ignore +//! use serial_test::serial; +//! use crate::test_helpers::EnvGuard; +//! +//! #[test] +//! #[serial] +//! fn example() { +//! let _home = EnvGuard::set("HOME", "/tmp/test"); +//! // ... test body; HOME is restored when `_home` drops. +//! } +//! ``` +//! +//! # Drop order +//! +//! When a test holds multiple guards, they drop in LIFO order: +//! +//! ```ignore +//! let _home = EnvGuard::set("HOME", "/tmp/test"); +//! let _user = EnvGuard::set("USER", "testuser"); +//! // Drops in reverse: USER first, then HOME. +//! ``` +//! +//! # Encoding +//! +//! Values are stored as `OsString` to preserve non-UTF-8 data on platforms +//! where environment variables can contain arbitrary bytes. Test values used +//! in this crate are all ASCII, so the `var_os` + `set_var` cycle is lossless. + +#![cfg(test)] + +use std::ffi::{OsStr, OsString}; + +/// RAII guard that sets or removes an environment variable on construction +/// and restores the previous value (or unset state) on drop. +/// +/// Tests that use `EnvGuard` **must** also be annotated with +/// `#[serial_test::serial]` to prevent races with other tests that read or +/// mutate the same variable. +#[must_use = "EnvGuard must be bound to a local; dropping it immediately restores the variable"] +pub struct EnvGuard { + key: OsString, + original: Option, +} + +// `#[allow(dead_code)]` is applied per-method so integration tests that only +// use one constructor don't generate "unused" warnings for the other. The +// file is shared between the unit test module (via the `test_helpers` module +// tree) and integration tests (via `#[path]` include in `tests/common/mod.rs`). +impl EnvGuard { + /// Set an environment variable, saving its prior value for restoration. + #[allow(dead_code)] + pub fn set(key: impl Into, value: impl AsRef) -> Self { + let key = key.into(); + let original = std::env::var_os(&key); + // SAFETY: `#[serial]`-annotated tests that construct `EnvGuard` do not + // run concurrently with each other, so cross-serial races on the env + // block are eliminated. Callers MUST ensure that any test which + // observes or mutates the same variable is also annotated with + // `#[serial]` (or with a matching `#[serial(key)]`); non-serial tests + // reading these variables can still race with `EnvGuard`'s mutations. + // See the module-level soundness contract for full requirements. + unsafe { + std::env::set_var(&key, value); + } + Self { key, original } + } + + /// Remove an environment variable, saving its prior value for restoration. + #[allow(dead_code)] + pub fn remove(key: impl Into) -> Self { + let key = key.into(); + let original = std::env::var_os(&key); + // SAFETY: same rationale as `EnvGuard::set`; see the full comment + // there and the module-level soundness contract. + unsafe { + std::env::remove_var(&key); + } + Self { key, original } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: same rationale as `EnvGuard::set`; see the full comment + // there and the module-level soundness contract. + unsafe { + match self.original.take() { + Some(v) => std::env::set_var(&self.key, v), + None => std::env::remove_var(&self.key), + } + } + } +} + +// The file is already gated with `#![cfg(test)]`, so no additional +// `#[cfg(test)]` attribute is needed on this inner module. +mod tests { + use super::EnvGuard; + use serial_test::serial; + + // Use a unique prefix so these tests cannot clash with any other test + // that happens to read or write a same-named variable in parallel. + const KEY_SET: &str = "BSSH_ENVGUARD_TEST_VAR_SET"; + const KEY_REMOVE: &str = "BSSH_ENVGUARD_TEST_VAR_REMOVE"; + const KEY_CHAIN: &str = "BSSH_ENVGUARD_TEST_VAR_CHAIN"; + const KEY_UNSET: &str = "BSSH_ENVGUARD_TEST_VAR_UNSET"; + const KEY_OVERWRITE: &str = "BSSH_ENVGUARD_TEST_VAR_OVERWRITE"; + + /// `EnvGuard::set` must restore the prior value (or absence) when dropped. + #[test] + #[serial] + fn set_restores_prior_value_on_drop() { + // Ensure the variable is absent before we start. + unsafe { std::env::remove_var(KEY_SET) }; + + { + let _guard = EnvGuard::set(KEY_SET, "temporary"); + assert_eq!(std::env::var(KEY_SET).ok().as_deref(), Some("temporary")); + } + // After the guard drops the variable must be absent again. + assert!( + std::env::var(KEY_SET).is_err(), + "variable should be unset after guard drop" + ); + } + + /// `EnvGuard::remove` must restore the prior value when dropped. + #[test] + #[serial] + fn remove_restores_prior_value_on_drop() { + unsafe { std::env::set_var(KEY_REMOVE, "original") }; + + { + let _guard = EnvGuard::remove(KEY_REMOVE); + assert!( + std::env::var(KEY_REMOVE).is_err(), + "variable should be absent while guard is live" + ); + } + // After drop the original value must be back. + assert_eq!( + std::env::var(KEY_REMOVE).ok().as_deref(), + Some("original"), + "variable should be restored after guard drop" + ); + // Clean up so later test runs start clean. + unsafe { std::env::remove_var(KEY_REMOVE) }; + } + + /// Multiple guards dropped in LIFO order must each restore their own prior state. + #[test] + #[serial] + fn chained_set_guards_restore_in_lifo_order() { + unsafe { std::env::remove_var(KEY_CHAIN) }; + + { + let _g1 = EnvGuard::set(KEY_CHAIN, "first"); + assert_eq!(std::env::var(KEY_CHAIN).ok().as_deref(), Some("first")); + + { + let _g2 = EnvGuard::set(KEY_CHAIN, "second"); + assert_eq!(std::env::var(KEY_CHAIN).ok().as_deref(), Some("second")); + } + // _g2 dropped: KEY_CHAIN should be back to "first" (what _g1 saved). + assert_eq!( + std::env::var(KEY_CHAIN).ok().as_deref(), + Some("first"), + "inner guard should restore to value set by outer guard" + ); + } + // _g1 dropped: KEY_CHAIN should be absent (what existed before _g1). + assert!( + std::env::var(KEY_CHAIN).is_err(), + "outer guard should restore to absent state" + ); + } + + /// `EnvGuard::remove` on a variable that is already unset must be a no-op + /// and leave the variable unset after dropping. + #[test] + #[serial] + fn remove_on_already_unset_variable_is_noop() { + unsafe { std::env::remove_var(KEY_UNSET) }; + + { + let _guard = EnvGuard::remove(KEY_UNSET); + assert!(std::env::var(KEY_UNSET).is_err()); + } + // Still absent after drop — no spurious value introduced. + assert!( + std::env::var(KEY_UNSET).is_err(), + "removing an already-absent variable should leave it absent" + ); + } + + /// `EnvGuard::set` when the variable already has a value must restore + /// that pre-existing value, not leave the variable absent. + #[test] + #[serial] + fn set_over_existing_value_restores_original() { + unsafe { std::env::set_var(KEY_OVERWRITE, "before") }; + + { + let _guard = EnvGuard::set(KEY_OVERWRITE, "during"); + assert_eq!(std::env::var(KEY_OVERWRITE).ok().as_deref(), Some("during")); + } + assert_eq!( + std::env::var(KEY_OVERWRITE).ok().as_deref(), + Some("before"), + "guard should restore the pre-existing value, not remove the variable" + ); + // Clean up. + unsafe { std::env::remove_var(KEY_OVERWRITE) }; + } +} diff --git a/src/test_helpers/mod.rs b/src/test_helpers/mod.rs new file mode 100644 index 00000000..df637e19 --- /dev/null +++ b/src/test_helpers/mod.rs @@ -0,0 +1,24 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Shared test utilities. +//! +//! This module is compiled only under `#[cfg(test)]` (gated at the `lib.rs` +//! declaration) and is not linked into release binaries. Integration tests +//! under `tests/` access this code via `tests/common/mod.rs`, which includes +//! `env_guard.rs` directly with `#[path]` so both unit and integration tests +//! share one implementation. + +mod env_guard; +pub use env_guard::EnvGuard; diff --git a/tests/backendai_env_test.rs b/tests/backendai_env_test.rs index 24be7ff1..5308a80b 100644 --- a/tests/backendai_env_test.rs +++ b/tests/backendai_env_test.rs @@ -12,29 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. -use bssh::config::Config; -use once_cell::sync::Lazy; -use std::env; -use tokio::sync::Mutex; +mod common; -// Global mutex to serialize tests that modify environment variables -static ENV_MUTEX: Lazy> = Lazy::new(|| Mutex::new(())); +use bssh::config::Config; +use common::EnvGuard; +use serial_test::serial; #[tokio::test] +#[serial] async fn test_backendai_env_auto_detection() { - let _guard = ENV_MUTEX.lock().await; - - // Save original env vars - let orig_hosts = env::var("BACKENDAI_CLUSTER_HOSTS").ok(); - let orig_host = env::var("BACKENDAI_CLUSTER_HOST").ok(); - let orig_role = env::var("BACKENDAI_CLUSTER_ROLE").ok(); - - // Set Backend.AI environment variables - unsafe { - env::set_var("BACKENDAI_CLUSTER_HOSTS", "node1.ai,node2.ai,node3.ai"); - env::set_var("BACKENDAI_CLUSTER_HOST", "node1.ai"); - env::set_var("BACKENDAI_CLUSTER_ROLE", "main"); - } + // Guards restore prior values on drop. + let _hosts = EnvGuard::set("BACKENDAI_CLUSTER_HOSTS", "node1.ai,node2.ai,node3.ai"); + let _host = EnvGuard::set("BACKENDAI_CLUSTER_HOST", "node1.ai"); + let _role = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "main"); // Create a temporary directory for the test let temp_dir = tempfile::tempdir().unwrap(); @@ -79,45 +69,16 @@ async fn test_backendai_env_auto_detection() { Some("/home/config/ssh/id_cluster".to_string()), "get_ssh_key should return Backend.AI cluster key path" ); - - // Restore original env vars - unsafe { - if let Some(val) = orig_hosts { - env::set_var("BACKENDAI_CLUSTER_HOSTS", val); - } else { - env::remove_var("BACKENDAI_CLUSTER_HOSTS"); - } - - if let Some(val) = orig_host { - env::set_var("BACKENDAI_CLUSTER_HOST", val); - } else { - env::remove_var("BACKENDAI_CLUSTER_HOST"); - } - - if let Some(val) = orig_role { - env::set_var("BACKENDAI_CLUSTER_ROLE", val); - } else { - env::remove_var("BACKENDAI_CLUSTER_ROLE"); - } - } } #[tokio::test] +#[serial] async fn test_backendai_env_with_single_host() { - let _guard = ENV_MUTEX.lock().await; - - // Save original env vars - let orig_hosts = env::var("BACKENDAI_CLUSTER_HOSTS").ok(); - let orig_host = env::var("BACKENDAI_CLUSTER_HOST").ok(); - let orig_role = env::var("BACKENDAI_CLUSTER_ROLE").ok(); - - // Set Backend.AI environment variables with single host - unsafe { - env::set_var("BACKENDAI_CLUSTER_HOSTS", "single-node.ai"); - env::set_var("BACKENDAI_CLUSTER_HOST", "single-node.ai"); - // Explicitly remove ROLE to avoid contamination from previous tests - env::remove_var("BACKENDAI_CLUSTER_ROLE"); - } + // Guards restore prior values on drop. + let _hosts = EnvGuard::set("BACKENDAI_CLUSTER_HOSTS", "single-node.ai"); + let _host = EnvGuard::set("BACKENDAI_CLUSTER_HOST", "single-node.ai"); + // Explicitly remove ROLE to avoid contamination from previous tests + let _role = EnvGuard::remove("BACKENDAI_CLUSTER_ROLE"); // Create a temporary directory for the test let temp_dir = tempfile::tempdir().unwrap(); @@ -137,42 +98,15 @@ async fn test_backendai_env_with_single_host() { assert_eq!(nodes.len(), 1); assert_eq!(nodes[0].host, "single-node.ai"); assert_eq!(nodes[0].port, 2200); - - // Restore - unsafe { - if let Some(val) = orig_hosts { - env::set_var("BACKENDAI_CLUSTER_HOSTS", val); - } else { - env::remove_var("BACKENDAI_CLUSTER_HOSTS"); - } - - if let Some(val) = orig_host { - env::set_var("BACKENDAI_CLUSTER_HOST", val); - } else { - env::remove_var("BACKENDAI_CLUSTER_HOST"); - } - - if let Some(val) = orig_role { - env::set_var("BACKENDAI_CLUSTER_ROLE", val); - } else { - env::remove_var("BACKENDAI_CLUSTER_ROLE"); - } - } } #[tokio::test] +#[serial] async fn test_no_backendai_env() { - let _guard = ENV_MUTEX.lock().await; - - // Save and clear Backend.AI env vars - let orig_hosts = env::var("BACKENDAI_CLUSTER_HOSTS").ok(); - let orig_host = env::var("BACKENDAI_CLUSTER_HOST").ok(); - - unsafe { - env::remove_var("BACKENDAI_CLUSTER_HOSTS"); - env::remove_var("BACKENDAI_CLUSTER_HOST"); - env::remove_var("BACKENDAI_CLUSTER_ROLE"); - } + // Clear all Backend.AI env vars; guards restore prior values on drop. + let _hosts = EnvGuard::remove("BACKENDAI_CLUSTER_HOSTS"); + let _host = EnvGuard::remove("BACKENDAI_CLUSTER_HOST"); + let _role = EnvGuard::remove("BACKENDAI_CLUSTER_ROLE"); // Load config without Backend.AI env let config = Config::load_with_priority(&std::path::PathBuf::from("nonexistent.yaml")) @@ -181,14 +115,4 @@ async fn test_no_backendai_env() { // Verify no backendai cluster was created assert!(!config.clusters.contains_key("backendai")); - - // Restore if needed - unsafe { - if let Some(val) = orig_hosts { - env::set_var("BACKENDAI_CLUSTER_HOSTS", val); - } - if let Some(val) = orig_host { - env::set_var("BACKENDAI_CLUSTER_HOST", val); - } - } } diff --git a/tests/common/mod.rs b/tests/common/mod.rs new file mode 100644 index 00000000..be43ad09 --- /dev/null +++ b/tests/common/mod.rs @@ -0,0 +1,26 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Shared helpers for integration tests. +//! +//! The integration test binaries under `tests/` cannot see the crate-private +//! `test_helpers` module directly, so we include the `env_guard.rs` +//! implementation via `#[path]`. This ensures unit and integration tests share +//! one source of truth without exposing `EnvGuard` as public API of `bssh`. + +#[path = "../../src/test_helpers/env_guard.rs"] +mod env_guard_impl; + +#[allow(unused_imports)] +pub use env_guard_impl::EnvGuard; diff --git a/tests/exit_code_integration_test.rs b/tests/exit_code_integration_test.rs index 9a8181ec..f7fbda08 100644 --- a/tests/exit_code_integration_test.rs +++ b/tests/exit_code_integration_test.rs @@ -17,9 +17,12 @@ //! These tests verify the end-to-end behavior of exit code handling, //! including rank detection and strategy application. +mod common; + use bssh::executor::{ExecutionResult, ExitCodeStrategy, RankDetector}; use bssh::node::Node; use bssh::ssh::client::CommandResult; +use common::EnvGuard; use serial_test::serial; /// Helper to create a success result @@ -137,9 +140,9 @@ fn test_hybrid_strategy_main_fails() { #[test] #[serial] fn test_backendai_main_rank_detection() { - // Set Backend.AI environment - std::env::set_var("BACKENDAI_CLUSTER_ROLE", "main"); - std::env::set_var("BACKENDAI_CLUSTER_HOST", "host2"); + // Set Backend.AI environment; guards restore prior values on drop. + let _role = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "main"); + let _host = EnvGuard::set("BACKENDAI_CLUSTER_HOST", "host2"); let nodes = vec![ Node::new("host1".to_string(), 22, "user".to_string()), @@ -153,10 +156,6 @@ fn test_backendai_main_rank_detection() { Some(1), "Should detect host2 as main rank via Backend.AI env" ); - - // Cleanup - std::env::remove_var("BACKENDAI_CLUSTER_ROLE"); - std::env::remove_var("BACKENDAI_CLUSTER_HOST"); } #[test] @@ -273,9 +272,9 @@ fn test_main_rank_marking_in_results() { #[test] #[serial] fn test_main_rank_marking_with_backendai_env() { - // Test that Backend.AI environment affects rank marking - std::env::set_var("BACKENDAI_CLUSTER_ROLE", "main"); - std::env::set_var("BACKENDAI_CLUSTER_HOST", "host3"); + // Test that Backend.AI environment affects rank marking; guards restore on drop. + let _role = EnvGuard::set("BACKENDAI_CLUSTER_ROLE", "main"); + let _host = EnvGuard::set("BACKENDAI_CLUSTER_HOST", "host3"); let nodes = vec![ Node::new("host1".to_string(), 22, "user".to_string()), @@ -315,10 +314,6 @@ fn test_main_rank_marking_with_backendai_env() { results[2].is_main_rank, "host3 should be marked as main rank" ); - - // Cleanup - std::env::remove_var("BACKENDAI_CLUSTER_ROLE"); - std::env::remove_var("BACKENDAI_CLUSTER_HOST"); } #[test] diff --git a/tests/glob_pattern_test.rs b/tests/glob_pattern_test.rs index f983fdf1..d6668e53 100644 --- a/tests/glob_pattern_test.rs +++ b/tests/glob_pattern_test.rs @@ -18,17 +18,17 @@ use tempfile::TempDir; /// Helper function to resolve glob patterns (mimics the main.rs implementation) fn resolve_source_files(source: &Path) -> anyhow::Result> { - if let Some(pattern_str) = source.to_str() { - if pattern_str.contains('*') || pattern_str.contains('?') || pattern_str.contains('[') { - // It's a glob pattern - let matches: Vec = glob::glob(pattern_str)?.filter_map(Result::ok).collect(); - - if matches.is_empty() { - anyhow::bail!("No files matched the pattern: {pattern_str}"); - } - - return Ok(matches); + if let Some(pattern_str) = source.to_str() + && (pattern_str.contains('*') || pattern_str.contains('?') || pattern_str.contains('[')) + { + // It's a glob pattern + let matches: Vec = glob::glob(pattern_str)?.filter_map(Result::ok).collect(); + + if matches.is_empty() { + anyhow::bail!("No files matched the pattern: {pattern_str}"); } + + return Ok(matches); } // Not a glob pattern, return as-is diff --git a/tests/interactive_test.rs b/tests/interactive_test.rs index e621b72a..77080441 100644 --- a/tests/interactive_test.rs +++ b/tests/interactive_test.rs @@ -20,6 +20,8 @@ use bssh::ssh::known_hosts::StrictHostKeyChecking; use bssh::ssh::tokio_client::SshConnectionConfig; use std::path::PathBuf; +mod common; + #[tokio::test] async fn test_interactive_command_creation() { let cmd = InteractiveCommand { @@ -196,7 +198,9 @@ mod terminal_tests { #[cfg(test)] mod auth_method_tests { + use crate::common::EnvGuard; use bssh::ssh::tokio_client::AuthMethod; + use serial_test::serial; use std::env; #[test] @@ -220,23 +224,9 @@ mod auth_method_tests { } #[test] + #[serial] fn test_ssh_agent_detection() { - // Save original value - let original = env::var("SSH_AUTH_SOCK").ok(); - - // Test with SSH_AUTH_SOCK set - unsafe { - env::set_var("SSH_AUTH_SOCK", "/tmp/ssh-agent.sock"); - } + let _guard = EnvGuard::set("SSH_AUTH_SOCK", "/tmp/ssh-agent.sock"); assert!(env::var("SSH_AUTH_SOCK").is_ok()); - - // Restore original value - unsafe { - if let Some(val) = original { - env::set_var("SSH_AUTH_SOCK", val); - } else { - env::remove_var("SSH_AUTH_SOCK"); - } - } } } diff --git a/tests/jump_host_config_test.rs b/tests/jump_host_config_test.rs index ee5169f6..19189728 100644 --- a/tests/jump_host_config_test.rs +++ b/tests/jump_host_config_test.rs @@ -21,8 +21,12 @@ //! Tests for issue #167 verify per-jump-host SSH key configuration. //! Tests for issue #170 verify SSH config Host alias reference support. +mod common; + use bssh::config::{Config, JumpHostConfig}; use bssh::ssh::ssh_config::SshConfig; +use common::EnvGuard; +use serial_test::serial; /// Test that global default jump_host is applied to all clusters #[test] @@ -165,12 +169,11 @@ clusters: /// Test environment variable expansion in jump_host #[test] +#[serial] fn test_config_jump_host_env_expansion() { - // Set environment variables - unsafe { - std::env::set_var("BSSH_TEST_BASTION", "env-bastion.example.com"); - std::env::set_var("BSSH_TEST_PORT", "2222"); - } + // Set environment variables; guards restore prior values on drop. + let _bastion = EnvGuard::set("BSSH_TEST_BASTION", "env-bastion.example.com"); + let _port = EnvGuard::set("BSSH_TEST_PORT", "2222"); let yaml = r#" defaults: @@ -204,12 +207,6 @@ clusters: config.get_jump_host("production", 1), Some("env-bastion.example.com:2222".to_string()) ); - - // Clean up - unsafe { - std::env::remove_var("BSSH_TEST_BASTION"); - std::env::remove_var("BSSH_TEST_PORT"); - } } /// Test jump_host with user@host:port format @@ -513,9 +510,10 @@ clusters: /// Test environment variable expansion in jump_host ssh_key field #[test] +#[serial] fn test_jump_host_ssh_key_env_expansion() { - std::env::set_var("TEST_JUMP_HOST", "env-bastion.example.com"); - std::env::set_var("TEST_JUMP_KEY", "/keys/env_key"); + let _host = EnvGuard::set("TEST_JUMP_HOST", "env-bastion.example.com"); + let _key = EnvGuard::set("TEST_JUMP_KEY", "/keys/env_key"); let yaml = r#" clusters: @@ -534,9 +532,6 @@ clusters: assert_eq!(conn_str, "env-bastion.example.com"); assert_eq!(ssh_key.as_deref(), Some("/keys/env_key")); - - std::env::remove_var("TEST_JUMP_HOST"); - std::env::remove_var("TEST_JUMP_KEY"); } /// Test backward compatibility: both simple and structured formats work diff --git a/tests/pdsh_compat_test.rs b/tests/pdsh_compat_test.rs index 8ae730a0..6ed807d9 100644 --- a/tests/pdsh_compat_test.rs +++ b/tests/pdsh_compat_test.rs @@ -17,39 +17,13 @@ //! These tests verify that bssh correctly handles pdsh-style arguments //! and behaves as expected in pdsh compatibility mode. +mod common; + use bssh::cli::{PDSH_COMPAT_ENV_VAR, PdshCli, has_pdsh_compat_flag, remove_pdsh_compat_flag}; +use common::EnvGuard; use serial_test::serial; use std::env; -/// Helper to run a test with env var protection -fn with_env_var(key: &str, value: &str, f: F) -> T -where - F: FnOnce() -> T, -{ - let original = env::var(key).ok(); - env::set_var(key, value); - let result = f(); - match original { - Some(v) => env::set_var(key, v), - None => env::remove_var(key), - } - result -} - -/// Helper to run a test with env var removed -fn without_env_var(key: &str, f: F) -> T -where - F: FnOnce() -> T, -{ - let original = env::var(key).ok(); - env::remove_var(key); - let result = f(); - if let Some(v) = original { - env::set_var(key, v); - } - result -} - // ============================================================================= // CLI Flag Detection Tests // ============================================================================= @@ -119,56 +93,44 @@ fn test_remove_pdsh_compat_flag_no_flag_present() { #[test] #[serial] fn test_env_var_detection_with_one() { - without_env_var(PDSH_COMPAT_ENV_VAR, || { - with_env_var(PDSH_COMPAT_ENV_VAR, "1", || { - // We can't call is_pdsh_compat_mode directly because it also checks argv[0] - // Instead, verify the env var logic works - let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); - assert!(value.is_some()); - let v = value.unwrap(); - assert!(v == "1" || v.to_lowercase() == "true"); - }); - }); + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, "1"); + // We can't call is_pdsh_compat_mode directly because it also checks argv[0] + // Instead, verify the env var logic works + let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); + assert!(value.is_some()); + let v = value.unwrap(); + assert!(v == "1" || v.to_lowercase() == "true"); } #[test] #[serial] fn test_env_var_detection_with_true() { - without_env_var(PDSH_COMPAT_ENV_VAR, || { - with_env_var(PDSH_COMPAT_ENV_VAR, "true", || { - let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); - assert!(value.is_some()); - assert_eq!(value.unwrap().to_lowercase(), "true"); - }); - }); + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, "true"); + let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); + assert!(value.is_some()); + assert_eq!(value.unwrap().to_lowercase(), "true"); } #[test] #[serial] fn test_env_var_detection_disabled_with_zero() { - without_env_var(PDSH_COMPAT_ENV_VAR, || { - with_env_var(PDSH_COMPAT_ENV_VAR, "0", || { - let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); - assert!(value.is_some()); - let v = value.unwrap(); - // "0" should NOT be treated as enabled - assert!(!(v == "1" || v.to_lowercase() == "true")); - }); - }); + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, "0"); + let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); + assert!(value.is_some()); + let v = value.unwrap(); + // "0" should NOT be treated as enabled + assert!(!(v == "1" || v.to_lowercase() == "true")); } #[test] #[serial] fn test_env_var_detection_disabled_with_false() { - without_env_var(PDSH_COMPAT_ENV_VAR, || { - with_env_var(PDSH_COMPAT_ENV_VAR, "false", || { - let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); - assert!(value.is_some()); - let v = value.unwrap(); - // "false" should NOT be treated as enabled - assert!(!(v == "1" || v.to_lowercase() == "true")); - }); - }); + let _guard = EnvGuard::set(PDSH_COMPAT_ENV_VAR, "false"); + let value = env::var(PDSH_COMPAT_ENV_VAR).ok(); + assert!(value.is_some()); + let v = value.unwrap(); + // "false" should NOT be treated as enabled + assert!(!(v == "1" || v.to_lowercase() == "true")); } // ============================================================================= diff --git a/tests/pty_stress_test.rs b/tests/pty_stress_test.rs index 1c61f277..012cafb8 100644 --- a/tests/pty_stress_test.rs +++ b/tests/pty_stress_test.rs @@ -225,16 +225,12 @@ async fn test_concurrent_message_producers() { if let PtyMessage::LocalInput(data) = msg { let content = String::from_utf8_lossy(&data); // Extract producer ID from message - if let Some(start) = content.find("Producer ") { - if let Some(end) = content[start + 9..].find(" ") { - if let Ok(producer_id) = - content[start + 9..start + 9 + end].parse::() - { - if producer_id < producers { - producer_counts[producer_id] += 1; - } - } - } + if let Some(start) = content.find("Producer ") + && let Some(end) = content[start + 9..].find(" ") + && let Ok(producer_id) = content[start + 9..start + 9 + end].parse::() + && producer_id < producers + { + producer_counts[producer_id] += 1; } total_received += 1; }