Skip to content

Commit 3eb2ec1

Browse files
mikolalysenkoclaude
mikolalysenko
and
claude
authored
Build: share one test env guard in core (#1889) (#1891)
* Start fix for #1889 Assisted-by: Claude Code:claude-opus-5-5 * Share one env guard across core tests Tests that change process env each wrote their own restore-on-drop guard: 11 EnvGuard copies plus the with_env / with_home_env closures and api::client's OrgResolutionEnvGuard. The copies drifted. Most saved the previous value as a String, so a non-UTF-8 value was lost, and the two closure helpers never restored env when the body panicked, which leaked into the next #[serial] test. Add crate::test_env with EnvGuard::{set, unset, many} and with_env. It saves OsString values, restores newest-first and is panic-safe. Every core unit test now uses it. The three integration tests that had their own copy compile the same file in via #[path]. Tests only; no production code changes. Assisted-by: Claude Code:claude-opus-5-5 * Address review: env guard panic safety - EnvGuard::many records each saved value before changing it, so a panic partway through still restores the vars already changed. - Move the guard's self-tests into test_env_tests.rs, so the integration tests that #[path]-include test_env.rs don't run them again. - Put SOCKET_PROXY_URL under the guard in the org-resolution proxy test instead of restoring it by hand after the asserts. - Pass paths straight to EnvGuard::set instead of to_str().unwrap(). Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f54766b commit 3eb2ec1

16 files changed

Lines changed: 200 additions & 362 deletions

‎crates/socket-patch-core/src/api/client.rs‎

Lines changed: 18 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -3271,9 +3271,9 @@ mod tests {
32713271
#[serial_test::serial]
32723272
async fn binary_url_uses_the_resolved_proxy_when_org_resolution_fails() {
32733273
const HASH: &str = "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789";
3274-
let _env = OrgResolutionEnvGuard::isolate();
3275-
let saved_proxy = std::env::var("SOCKET_PROXY_URL").ok();
3276-
std::env::set_var("SOCKET_PROXY_URL", "http://env-proxy.test:9999/");
3274+
let _env = isolate_org_resolution_env();
3275+
let _proxy =
3276+
crate::test_env::EnvGuard::set("SOCKET_PROXY_URL", "http://env-proxy.test:9999/");
32773277

32783278
let (client, use_public_proxy) = get_api_client_with_overrides(ApiClientEnvOverrides {
32793279
api_url: Some("http://127.0.0.1:1".into()),
@@ -3287,44 +3287,23 @@ mod tests {
32873287
std::env::remove_var("SOCKET_PROXY_URL");
32883288
let (url, use_auth) = client.binary_url("blob", HASH);
32893289

3290-
match saved_proxy {
3291-
Some(v) => std::env::set_var("SOCKET_PROXY_URL", v),
3292-
None => std::env::remove_var("SOCKET_PROXY_URL"),
3293-
}
3294-
32953290
assert!(use_public_proxy);
32963291
assert!(client.api_token().is_none(), "a proxy client has no bearer");
32973292
assert_eq!(url, format!("http://env-proxy.test:9999/patch/blob/{HASH}"));
32983293
assert!(!use_auth, "the bearer must never target the proxy");
32993294
}
33003295

3301-
/// Guard that snapshots the env vars that can short-circuit org
3302-
/// auto-resolution, forces the "no ambient slug / offline flag /
3303-
/// socket-cli config" state, and restores the snapshot on drop (panic
3304-
/// included). Keeps the resolution tests below honest on a dev machine
3305-
/// with `socket login` state or ambient SOCKET_* vars.
3306-
struct OrgResolutionEnvGuard {
3307-
saved: Vec<(&'static str, Option<String>)>,
3308-
}
3309-
impl OrgResolutionEnvGuard {
3310-
fn isolate() -> Self {
3311-
let names = ["SOCKET_ORG_SLUG", "SOCKET_OFFLINE", "SOCKET_NO_CONFIG"];
3312-
let saved = names.iter().map(|&n| (n, std::env::var(n).ok())).collect();
3313-
std::env::remove_var("SOCKET_ORG_SLUG");
3314-
std::env::remove_var("SOCKET_OFFLINE");
3315-
std::env::set_var("SOCKET_NO_CONFIG", "1");
3316-
Self { saved }
3317-
}
3318-
}
3319-
impl Drop for OrgResolutionEnvGuard {
3320-
fn drop(&mut self) {
3321-
for (name, value) in self.saved.drain(..) {
3322-
match value {
3323-
Some(v) => std::env::set_var(name, v),
3324-
None => std::env::remove_var(name),
3325-
}
3326-
}
3327-
}
3296+
/// Forces the "no ambient slug / offline flag / socket-cli config"
3297+
/// state for the env vars that can short-circuit org auto-resolution;
3298+
/// the returned guard restores them on drop (panic included). Keeps
3299+
/// the resolution tests below honest on a dev machine with `socket
3300+
/// login` state or ambient SOCKET_* vars.
3301+
fn isolate_org_resolution_env() -> crate::test_env::EnvGuard {
3302+
crate::test_env::EnvGuard::many([
3303+
("SOCKET_ORG_SLUG", None),
3304+
("SOCKET_OFFLINE", None),
3305+
("SOCKET_NO_CONFIG", Some("1")),
3306+
])
33283307
}
33293308

33303309
/// The network half of `resolve_org_slug`: `GET /v0/organizations`
@@ -3338,7 +3317,7 @@ mod tests {
33383317
use wiremock::matchers::{method, path};
33393318
use wiremock::{Mock, MockServer, ResponseTemplate};
33403319

3341-
let _env = OrgResolutionEnvGuard::isolate();
3320+
let _env = isolate_org_resolution_env();
33423321
let server = MockServer::start().await;
33433322
Mock::given(method("GET"))
33443323
.and(path("/v0/organizations"))
@@ -3381,7 +3360,7 @@ mod tests {
33813360
use wiremock::matchers::{method, path};
33823361
use wiremock::{Mock, MockServer, ResponseTemplate};
33833362

3384-
let _env = OrgResolutionEnvGuard::isolate();
3363+
let _env = isolate_org_resolution_env();
33853364
let server = MockServer::start().await;
33863365
Mock::given(method("GET"))
33873366
.and(path("/v0/organizations"))
@@ -3423,7 +3402,7 @@ mod tests {
34233402

34243403
const HASH: &str = "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789";
34253404
const UUID: &str = "11111111-2222-4333-8444-555555555555";
3426-
let _env = OrgResolutionEnvGuard::isolate();
3405+
let _env = isolate_org_resolution_env();
34273406
let token = format!("sktsec_{}_api", "x".repeat(44));
34283407
let proxy = "http://proxy.example.test";
34293408

@@ -4185,7 +4164,7 @@ mod tests {
41854164
#[tokio::test]
41864165
#[serial_test::serial]
41874166
async fn vendor_package_url_after_failed_org_resolution_targets_proxy_host() {
4188-
let _env = OrgResolutionEnvGuard::isolate();
4167+
let _env = isolate_org_resolution_env();
41894168
let (client, _) = get_api_client_with_overrides(ApiClientEnvOverrides {
41904169
api_url: Some("http://127.0.0.1:1".into()),
41914170
api_token: Some("sktsec_x_api".into()),

‎crates/socket-patch-core/src/crawlers/deno_crawler.rs‎

Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,7 @@ mod tests {
692692
let deno_home = tempfile::tempdir().unwrap();
693693
let jsr = deno_home.path().join("npm").join("jsr.io");
694694
stage(&jsr, "@std", "path", "0.220.0").await;
695-
let _g = EnvGuard::set("DENO_DIR", deno_home.path().to_str().unwrap());
695+
let _g = EnvGuard::set("DENO_DIR", deno_home.path());
696696

697697
let crawler = DenoCrawler;
698698
let opts = CrawlerOptions {
@@ -719,7 +719,7 @@ mod tests {
719719
stage(&jsr, "@std", "path", "0.220.0").await;
720720
stage(&jsr, "@std", "fs", "1.0.0").await;
721721
stage(&jsr, "@other", "x", "2.0.0").await;
722-
let _g = EnvGuard::set("DENO_DIR", deno_home.path().to_str().unwrap());
722+
let _g = EnvGuard::set("DENO_DIR", deno_home.path());
723723

724724
let opts = CrawlerOptions {
725725
cwd: project.path().to_path_buf(),
@@ -810,7 +810,7 @@ mod tests {
810810
let jsr = deno_home.path().join("npm").join("jsr.io");
811811
stage(&jsr, "@std", "path", "0.220.0").await;
812812
stage(&jsr, "@std", "fs", "1.0.0").await;
813-
let _g = EnvGuard::set("DENO_DIR", deno_home.path().to_str().unwrap());
813+
let _g = EnvGuard::set("DENO_DIR", deno_home.path());
814814
let opts = CrawlerOptions {
815815
cwd: project.path().to_path_buf(),
816816
global: false,
@@ -943,31 +943,7 @@ mod tests {
943943

944944
// ── deno_dir / cache-root resolution ───────────────────────────
945945

946-
/// Save and restore an env var around a closure body.
947-
struct EnvGuard {
948-
key: &'static str,
949-
prev: Option<String>,
950-
}
951-
impl EnvGuard {
952-
fn set(key: &'static str, value: &str) -> Self {
953-
let prev = std::env::var(key).ok();
954-
std::env::set_var(key, value);
955-
Self { key, prev }
956-
}
957-
fn unset(key: &'static str) -> Self {
958-
let prev = std::env::var(key).ok();
959-
std::env::remove_var(key);
960-
Self { key, prev }
961-
}
962-
}
963-
impl Drop for EnvGuard {
964-
fn drop(&mut self) {
965-
match &self.prev {
966-
Some(v) => std::env::set_var(self.key, v),
967-
None => std::env::remove_var(self.key),
968-
}
969-
}
970-
}
946+
use crate::test_env::EnvGuard;
971947

972948
/// #444: only Deno's `bin/.<tool>/node_modules` whose config leaves
973949
/// `nodeModulesDir` unset (or `none`/`false`), with no `package.json`,

‎crates/socket-patch-core/src/crawlers/go_crawler.rs‎

Lines changed: 7 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,7 +1216,7 @@ mod tests {
12161216
mkdir(&cache);
12171217
write(&project.join("go.mod"), "module example.com/app\n");
12181218
let env = (
1219-
EnvGuard::set("GOMODCACHE", cache.to_str().unwrap()),
1219+
EnvGuard::set("GOMODCACHE", &cache),
12201220
EnvGuard::unset("GOWORK"),
12211221
);
12221222
Fixture {
@@ -1412,7 +1412,7 @@ mod tests {
14121412
// GOWORK naming a file elsewhere (absolute on every platform:
14131413
// `/x` has no drive on Windows, so go rejects it as relative).
14141414
let elsewhere = f.project.parent().unwrap().join("elsewhere/go.work");
1415-
let _gowork = EnvGuard::set("GOWORK", &elsewhere.to_string_lossy());
1415+
let _gowork = EnvGuard::set("GOWORK", &elsewhere);
14161416
assert_eq!(f.crawl().await, walked);
14171417
}
14181418

@@ -1498,31 +1498,7 @@ mod tests {
14981498

14991499
// ---- get_gomodcache env tests ----
15001500

1501-
/// Save and restore an env var around a test body.
1502-
struct EnvGuard {
1503-
key: &'static str,
1504-
prev: Option<String>,
1505-
}
1506-
impl EnvGuard {
1507-
fn set(key: &'static str, value: &str) -> Self {
1508-
let prev = std::env::var(key).ok();
1509-
std::env::set_var(key, value);
1510-
Self { key, prev }
1511-
}
1512-
fn unset(key: &'static str) -> Self {
1513-
let prev = std::env::var(key).ok();
1514-
std::env::remove_var(key);
1515-
Self { key, prev }
1516-
}
1517-
}
1518-
impl Drop for EnvGuard {
1519-
fn drop(&mut self) {
1520-
match &self.prev {
1521-
Some(v) => std::env::set_var(self.key, v),
1522-
None => std::env::remove_var(self.key),
1523-
}
1524-
}
1525-
}
1501+
use crate::test_env::EnvGuard;
15261502

15271503
#[test]
15281504
#[serial_test::serial]
@@ -1547,7 +1523,7 @@ mod tests {
15471523
let _userprofile = EnvGuard::unset("USERPROFILE");
15481524
let first_a =
15491525
std::env::join_paths([gopath_a.path().as_os_str(), home_b.path().as_os_str()]).unwrap();
1550-
let _gopath = EnvGuard::set("GOPATH", first_a.to_str().unwrap());
1526+
let _gopath = EnvGuard::set("GOPATH", &first_a);
15511527
assert_eq!(
15521528
GoCrawler::get_gomodcache(),
15531529
Some(gopath_a.path().join("pkg").join("mod")),
@@ -1568,7 +1544,7 @@ mod tests {
15681544
// Case B: GOMODCACHE and GOPATH both set-but-empty fall all the way
15691545
// through to the $HOME/go/pkg/mod default.
15701546
let _gopath_empty = EnvGuard::set("GOPATH", "");
1571-
let _home_b = EnvGuard::set("HOME", home_b.path().to_str().unwrap());
1547+
let _home_b = EnvGuard::set("HOME", home_b.path());
15721548
assert_eq!(
15731549
GoCrawler::get_gomodcache(),
15741550
Some(home_b.path().join("go").join("pkg").join("mod")),
@@ -1602,7 +1578,7 @@ mod tests {
16021578
let gopath = dir.path().join("gopath");
16031579
let _gomodcache = EnvGuard::unset("GOMODCACHE");
16041580
let _gopath = EnvGuard::unset("GOPATH");
1605-
let _goenv = EnvGuard::set("GOENV", file.to_str().unwrap());
1581+
let _goenv = EnvGuard::set("GOENV", &file);
16061582

16071583
std::fs::write(&file, format!("GOMODCACHE={}\n", cache.display())).unwrap();
16081584
assert_eq!(GoCrawler::get_gomodcache(), Some(cache.clone()));
@@ -1614,7 +1590,7 @@ mod tests {
16141590
);
16151591

16161592
let env_cache = dir.path().join("env-cache");
1617-
let _env = EnvGuard::set("GOMODCACHE", env_cache.to_str().unwrap());
1593+
let _env = EnvGuard::set("GOMODCACHE", &env_cache);
16181594
assert_eq!(GoCrawler::get_gomodcache(), Some(env_cache));
16191595
}
16201596

‎crates/socket-patch-core/src/crawlers/maven_crawler.rs‎

Lines changed: 5 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2506,31 +2506,7 @@ mod tests {
25062506

25072507
// ---- m2_repo_path env tests ----
25082508

2509-
/// Save and restore an env var around a test body.
2510-
struct EnvGuard {
2511-
key: &'static str,
2512-
prev: Option<String>,
2513-
}
2514-
impl EnvGuard {
2515-
fn set(key: &'static str, value: &str) -> Self {
2516-
let prev = std::env::var(key).ok();
2517-
std::env::set_var(key, value);
2518-
Self { key, prev }
2519-
}
2520-
fn unset(key: &'static str) -> Self {
2521-
let prev = std::env::var(key).ok();
2522-
std::env::remove_var(key);
2523-
Self { key, prev }
2524-
}
2525-
}
2526-
impl Drop for EnvGuard {
2527-
fn drop(&mut self) {
2528-
match &self.prev {
2529-
Some(v) => std::env::set_var(self.key, v),
2530-
None => std::env::remove_var(self.key),
2531-
}
2532-
}
2533-
}
2509+
use crate::test_env::EnvGuard;
25342510

25352511
/// B66: with no usable home the default used to be the literal
25362512
/// RELATIVE `~/.m2/repository`, which resolves against the working
@@ -2584,7 +2560,7 @@ mod tests {
25842560
let _args = EnvGuard::unset("MAVEN_ARGS");
25852561
let _opts = EnvGuard::set(
25862562
"MAVEN_OPTS",
2587-
&format!("-Dmaven.repo.local={}", repo_local.path().display()),
2563+
format!("-Dmaven.repo.local={}", repo_local.path().display()),
25882564
);
25892565

25902566
let repo = MavenCrawler::m2_repo_path();
@@ -2612,8 +2588,8 @@ mod tests {
26122588
// Point the home directory at an empty dir so a developer's own
26132589
// `~/.m2/settings.xml` <localRepository> can't change the result.
26142590
let home = tempfile::tempdir().unwrap();
2615-
let _home = EnvGuard::set("HOME", home.path().to_str().unwrap());
2616-
let _profile = EnvGuard::set("USERPROFILE", home.path().to_str().unwrap());
2591+
let _home = EnvGuard::set("HOME", home.path());
2592+
let _profile = EnvGuard::set("USERPROFILE", home.path());
26172593

26182594
let repo = MavenCrawler::m2_repo_path();
26192595
assert_ne!(
@@ -2871,7 +2847,7 @@ mod tests {
28712847
.await
28722848
.unwrap();
28732849
let missing = project.path().join("no-such-m2-repo");
2874-
let _local = EnvGuard::set("MAVEN_REPO_LOCAL", missing.to_str().unwrap());
2850+
let _local = EnvGuard::set("MAVEN_REPO_LOCAL", &missing);
28752851

28762852
let crawler = MavenCrawler::new();
28772853
let options = CrawlerOptions {

‎crates/socket-patch-core/src/crawlers/npm_crawler.rs‎

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -5899,29 +5899,8 @@ mod tests {
58995899
}
59005900
}
59015901

5902-
/// Save and restore an env var around a test body (drop-safe).
59035902
#[cfg(target_os = "macos")]
5904-
struct EnvGuard {
5905-
key: &'static str,
5906-
prev: Option<String>,
5907-
}
5908-
#[cfg(target_os = "macos")]
5909-
impl EnvGuard {
5910-
fn set(key: &'static str, value: &std::ffi::OsStr) -> Self {
5911-
let prev = std::env::var(key).ok();
5912-
std::env::set_var(key, value);
5913-
Self { key, prev }
5914-
}
5915-
}
5916-
#[cfg(target_os = "macos")]
5917-
impl Drop for EnvGuard {
5918-
fn drop(&mut self) {
5919-
match &self.prev {
5920-
Some(v) => std::env::set_var(self.key, v),
5921-
None => std::env::remove_var(self.key),
5922-
}
5923-
}
5924-
}
5903+
use crate::test_env::EnvGuard;
59255904

59265905
/// Pins the literal volta (`.volta/tools/image/node/*/lib/
59275906
/// node_modules`) and fnm (`.fnm/node-versions/*/installation/lib/

0 commit comments

Comments
 (0)