You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
pacquet's DI convention (pacquet/CODE_STYLE_GUIDE.md, "Dependency injection for tests", principle 3) says stateful test fakes should keep their state in an interior-mutable staticdeclared inside each #[test] body, so "other tests get independent buffers and never race on it." A review on #12691 (r3518711611) flagged a violation in pacquet-network-web-auth-testing (being addressed in that PR). A codebase scan turns up the same module-level shared-mutable test state in several other places.
The correct pattern is already used widely — e.g. crates/package-manager/**/tests.rs declare static EVENTS: Mutex<Vec<LogEvent>> = Mutex::new(Vec::new());inside each recording-reporter test fn. This issue is only about the instances that hoist such state to module scope (a thread_local! block or a column-0 static), where it is shared and reconfigured across tests.
Instances found (pacquet)
crates/network-web-auth/src/poll_for_web_auth_token/tests.rs:36 — module-level thread_local! (TIME, fetch script, …), the same web-auth fake shape as the flagged PR.
crates/config/src/tests.rs:159 — module-level thread_local!FAKE_ENV: RefCell<HashMap<..>>, reconfigured via set_fake_env and read by a shared FakeEnv.
crates/config/src/store_path/tests.rs:88 — module-level static ALLOW_PREFIXES: Mutex<Vec<PathBuf>>plusstatic PREFIX_PROBE_SCENARIO_LOCK: Mutex<()> (a serialization lock — the "EnvGuard" shape the guide says DI is meant to retire). Added deliberately per a CodeRabbit review on fix(pacquet/config): pick the store on the project's volume #11804.
crates/env-installer/src/tests.rs:790 — module-level static CONFIG_DEP_EVENTS: Mutex<Vec<..>> backing a module-level RecordingReporter.
crates/package-manager/src/installability/tests.rs:19 — module-level thread_local!RECORDED_EVENTS: RefCell<Vec<LogEvent>> (carries a comment defending the thread-local choice).
pnpr was scanned and has none.
Ask
Move each of the above into the per-#[test] local-static form (or, where the fake is reused across many tests — as with the web-auth capabilities — a macro that expands fn-local state, as introduced for pacquet-network-web-auth-testing in #12691). Leave the already-correct in-fn static EVENTS recording reporters untouched.
Two entries were deliberate and should be decided case by case rather than mechanically rewritten:
installability carries an explicit justification (per-thread buffer, zero cross-test contention) — confirm whether the module-level thread_local! should still be localized.
store_path's PREFIX_PROBE_SCENARIO_LOCK serialization lock exists specifically to serialize a process-global probe; localizing it likely means removing the need for the lock (the point of the DI seam), so it needs a small redesign, not just a move.
Refs: #12691 (r3518711611), pacquet/CODE_STYLE_GUIDE.md DI principle 3.
Context
pacquet's DI convention (
pacquet/CODE_STYLE_GUIDE.md, "Dependency injection for tests", principle 3) says stateful test fakes should keep their state in an interior-mutablestaticdeclared inside each#[test]body, so "other tests get independent buffers and never race on it." A review on #12691 (r3518711611) flagged a violation inpacquet-network-web-auth-testing(being addressed in that PR). A codebase scan turns up the same module-level shared-mutable test state in several other places.The correct pattern is already used widely — e.g.
crates/package-manager/**/tests.rsdeclarestatic EVENTS: Mutex<Vec<LogEvent>> = Mutex::new(Vec::new());inside each recording-reporter test fn. This issue is only about the instances that hoist such state to module scope (athread_local!block or a column-0static), where it is shared and reconfigured across tests.Instances found (pacquet)
crates/network-web-auth/src/poll_for_web_auth_token/tests.rs:36— module-levelthread_local!(TIME, fetch script, …), the same web-auth fake shape as the flagged PR.crates/network-web-auth/src/prompt_browser_open/tests.rs:22— module-levelthread_local!(STDIN_TTY,LISTEN_OUTCOME,OPEN_OUTCOME).crates/config/src/tests.rs:159— module-levelthread_local!FAKE_ENV: RefCell<HashMap<..>>, reconfigured viaset_fake_envand read by a sharedFakeEnv.crates/config/src/store_path/tests.rs:88— module-levelstatic ALLOW_PREFIXES: Mutex<Vec<PathBuf>>plusstatic PREFIX_PROBE_SCENARIO_LOCK: Mutex<()>(a serialization lock — the "EnvGuard" shape the guide says DI is meant to retire). Added deliberately per a CodeRabbit review on fix(pacquet/config): pick the store on the project's volume #11804.crates/env-installer/src/tests.rs:790— module-levelstatic CONFIG_DEP_EVENTS: Mutex<Vec<..>>backing a module-levelRecordingReporter.crates/package-manager/src/installability/tests.rs:19— module-levelthread_local!RECORDED_EVENTS: RefCell<Vec<LogEvent>>(carries a comment defending the thread-local choice).pnprwas scanned and has none.Ask
Move each of the above into the per-
#[test]local-staticform (or, where the fake is reused across many tests — as with the web-auth capabilities — a macro that expands fn-local state, as introduced forpacquet-network-web-auth-testingin #12691). Leave the already-correct in-fnstatic EVENTSrecording reporters untouched.Two entries were deliberate and should be decided case by case rather than mechanically rewritten:
installabilitycarries an explicit justification (per-thread buffer, zero cross-test contention) — confirm whether the module-levelthread_local!should still be localized.store_path'sPREFIX_PROBE_SCENARIO_LOCKserialization lock exists specifically to serialize a process-global probe; localizing it likely means removing the need for the lock (the point of the DI seam), so it needs a small redesign, not just a move.Refs: #12691 (r3518711611),
pacquet/CODE_STYLE_GUIDE.mdDI principle 3.Written by an agent (Claude Code).