From 5df0b36af1b7726f6d380f3b53dd44c76764e93c Mon Sep 17 00:00:00 2001 From: fengmk2 Date: Tue, 31 Mar 2026 03:08:47 +0000 Subject: [PATCH] fix(test): add #[serial] to env-var tests to prevent concurrent access races (#1223) Tests that manipulate process env vars (VP_SHIM_TOOL, VITE_PLUS_SHIM_TOOL, PATH) without #[serial] can race with other tests running in parallel, causing flaky CI failures. Ensure all such tests are serialized and clear all related env vars before running. Also document env var testing guidelines in CLAUDE.md. --- CLAUDE.md | 6 +++ Cargo.lock | 1 + crates/vite_global_cli/src/shim/mod.rs | 58 +++++++++++--------------- crates/vite_shared/Cargo.toml | 3 ++ crates/vite_shared/src/home.rs | 1 + crates/vite_shared/src/path_env.rs | 2 +- 6 files changed, 37 insertions(+), 34 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 50a2c4ad0f..3c17fe0e26 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -106,6 +106,12 @@ All user-facing output must go through shared output modules instead of raw prin - Run `cargo test` to execute all tests - You never need to run `pnpm install` in the test fixtures dir, vite-plus should able to load and parse the workspace without `pnpm install`. +### Environment Variables in Tests + +- **Prefer `EnvConfig::test_scope()`**: For tests needing custom config values (VP_HOME, npm registry, etc.), use thread-local `EnvConfig::test_scope()` or `EnvConfig::test_guard()` from `vite_shared` — no `unsafe`, no `#[serial]`, full parallelism +- **`#[serial]` required for `std::env::set_var`/`remove_var`**: Any test that directly modifies process env vars (PATH, VP_SHIM_TOOL, etc.) MUST have `#[serial_test::serial]` to prevent concurrent access races +- **Clean up ALL related env vars**: When clearing env vars before a test, clear ALL vars that the function under test reads — not just the one being tested + ## Build - Run `pnpm bootstrap-cli` from the project root to build all packages and install the global CLI diff --git a/Cargo.lock b/Cargo.lock index 3cbddd5ac0..65f6dd67a7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7419,6 +7419,7 @@ dependencies = [ "rustls", "serde", "serde_json", + "serial_test", "supports-color 3.0.2", "tracing-subscriber", "vite_path", diff --git a/crates/vite_global_cli/src/shim/mod.rs b/crates/vite_global_cli/src/shim/mod.rs index 90e12466b5..e93d730847 100644 --- a/crates/vite_global_cli/src/shim/mod.rs +++ b/crates/vite_global_cli/src/shim/mod.rs @@ -217,10 +217,18 @@ mod tests { assert!(!is_potential_package_binary("another-fake-tool")); } + /// Clear both shim env vars to isolate tests. + /// SAFETY: caller must be `#[serial]` since this mutates process-global state. + unsafe fn clear_shim_env_vars() { + unsafe { + std::env::remove_var(SHIM_TOOL_ENV_VAR); + std::env::remove_var(LEGACY_SHIM_TOOL_ENV_VAR); + } + } + #[test] #[serial] fn test_detect_shim_tool_from_env_var() { - // SAFETY: We're in a test at startup, no other threads unsafe { std::env::set_var(SHIM_TOOL_ENV_VAR, "node"); std::env::remove_var(LEGACY_SHIM_TOOL_ENV_VAR); @@ -236,7 +244,6 @@ mod tests { fn test_detect_shim_tool_from_legacy_env_var() { // When only VITE_PLUS_SHIM_TOOL is set (older trampoline), it should // fall back to reading the legacy env var. - // SAFETY: We're in a test at startup, no other threads unsafe { std::env::remove_var(SHIM_TOOL_ENV_VAR); std::env::set_var(LEGACY_SHIM_TOOL_ENV_VAR, "npm"); @@ -247,43 +254,28 @@ mod tests { assert!(std::env::var(LEGACY_SHIM_TOOL_ENV_VAR).is_err()); } + /// Tests that argv0-based tool detection works for a given tool name, + /// including full path and .exe extension variants. + fn assert_detect_shim_tool_from_argv0(tool: &str) { + unsafe { clear_shim_env_vars() }; + + assert_eq!(detect_shim_tool(tool), Some(tool.to_string())); + assert_eq!( + detect_shim_tool(&vite_str::format!("/home/user/.vite-plus/bin/{tool}")), + Some(tool.to_string()), + ); + assert_eq!(detect_shim_tool(&vite_str::format!("{tool}.exe")), Some(tool.to_string()),); + } + #[test] #[serial] fn test_detect_shim_tool_vpx() { - // vpx should be detected via the argv0 check, before the env var check - // and before is_shim_tool (which would incorrectly match it as a package binary) - // SAFETY: We're in a test - unsafe { - std::env::remove_var(SHIM_TOOL_ENV_VAR); - } - let result = detect_shim_tool("vpx"); - assert_eq!(result, Some("vpx".to_string())); - - // Also works with full path - let result = detect_shim_tool("/home/user/.vite-plus/bin/vpx"); - assert_eq!(result, Some("vpx".to_string())); - - // Also works with .exe extension (Windows) - let result = detect_shim_tool("vpx.exe"); - assert_eq!(result, Some("vpx".to_string())); + assert_detect_shim_tool_from_argv0("vpx"); } #[test] + #[serial] fn test_detect_shim_tool_vpr() { - // vpr should be detected via the argv0 check, same pattern as vpx - // SAFETY: We're in a test - unsafe { - std::env::remove_var(SHIM_TOOL_ENV_VAR); - } - let result = detect_shim_tool("vpr"); - assert_eq!(result, Some("vpr".to_string())); - - // Also works with full path - let result = detect_shim_tool("/home/user/.vite-plus/bin/vpr"); - assert_eq!(result, Some("vpr".to_string())); - - // Also works with .exe extension (Windows) - let result = detect_shim_tool("vpr.exe"); - assert_eq!(result, Some("vpr".to_string())); + assert_detect_shim_tool_from_argv0("vpr"); } } diff --git a/crates/vite_shared/Cargo.toml b/crates/vite_shared/Cargo.toml index 4614724285..ff814d571b 100644 --- a/crates/vite_shared/Cargo.toml +++ b/crates/vite_shared/Cargo.toml @@ -22,5 +22,8 @@ which = { workspace = true } [target.'cfg(not(target_os = "windows"))'.dependencies] rustls = { workspace = true } +[dev-dependencies] +serial_test = { workspace = true } + [lints] workspace = true diff --git a/crates/vite_shared/src/home.rs b/crates/vite_shared/src/home.rs index 6b705180ad..65070b607c 100644 --- a/crates/vite_shared/src/home.rs +++ b/crates/vite_shared/src/home.rs @@ -64,6 +64,7 @@ mod tests { } #[test] + #[serial_test::serial] fn test_get_vite_plus_without_home() { use std::path::PathBuf; diff --git a/crates/vite_shared/src/path_env.rs b/crates/vite_shared/src/path_env.rs index 3cd905b778..f57d25db51 100644 --- a/crates/vite_shared/src/path_env.rs +++ b/crates/vite_shared/src/path_env.rs @@ -142,7 +142,7 @@ mod tests { } #[test] - #[ignore] + #[serial_test::serial] fn test_format_path_prepended_always_prepends() { // Even if the directory exists somewhere in PATH, it should be prepended let test_dir = "/test/node/bin";