Skip to content

test(pacquet): move module-level shared-mutable test statics into per-test-local state (DI principle 3) #12779

Description

@KSXGitHub

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-mutable static declared 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/network-web-auth/src/prompt_browser_open/tests.rs:22 — module-level thread_local! (STDIN_TTY, LISTEN_OUTCOME, OPEN_OUTCOME).
  • 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>> plus static 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.


Written by an agent (Claude Code).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions