Repository navigation
Keep YarnEnv unit tests off the host yarn env - #2065
Mikola Lysenko (mikolalysenko) wants to merge 2 commits into
Conversation
YarnEnv::current reads YARN_NODE_LINKER, YARN_PNPM_STORE_FOLDER, YARN_RC_FILENAME and YARN_INJECT_ENVIRONMENT_FILES from the process env and the rc file in the home folder. #2054 moved the detector tests onto a hermetic variant, but every crawler, vendor and vex path below it still built YarnEnv from the host, so on origin/main: - YARN_NODE_LINKER=node-modules fails 2 npm_crawler tests (yarn 4 pnpm-linker relocated store); - HOME with a .yarnrc.yml setting nodeLinker: node-modules fails 10 vendor tests (lock_inventory, npm_flavor, refusals, yarn_berry_lock). Under cfg(test), read only the YARN_* vars and HOME/USERPROFILE a live EnvGuard sets, the same way utils::go_env reads Go vars through test_env::guarded_var. Production builds (and the CLI integration tests, which link the non-test core) keep reading the real env. Refs #1919. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aeSopYBBeiUVSewPUqN
|
bugbot run Generated by Claude Code |
PR SummaryLow Risk Overview Production builds still use Adds serial test Reviewed by Cursor Bugbot for commit 3007a1c. Configure here. |
Review follow-ups on the YarnEnv test seam:
- base_var fell back to std::env::var under cfg(test), so a host
variable named in a fixture's ${NAME} reference still leaked. It
now reads only guarded variables in unit tests.
- Windows keeps a key's existing spelling when a guard sets it in
another case, so match the upper-case spelling there too.
- Read the process env once per YarnEnv::current, not three times.
- Cover YARN_RC_FILENAME and the ${NAME} lookup in the new test.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011f9aeSopYBBeiUVSewPUqN
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3007a1c. Configure here.
Problem
CI was healthy on this run: the merge queue was empty, the last 28 merge_group runs (since 11-12 18:56 UTC) all passed, main's latest push run (43324491043) and today's nightly (43373720347) are green, nothing was queued, and no PR run since 11-09 failed on a flake. So this run takes a priority-4 robustness target in the same class as #2054–#2057, #2063 and #2064: tests that pass only because CI's env is clean.
#2054 moved the
pkg_managers/pnpm_layoutdetector tests onto a hermetic detector. But every crawler, vendor and vex path below it still buildsYarnEnvfrom the host'sYARN_*vars and~/.yarnrc.yml. Onorigin/main(de81f1b):YARN_NODE_LINKER=node-modulesnpm_crawler::tests::global_scope_reads_a_yarn_store_configured_above_the_repository,test_yarn4_pnpm_linker_relocated_store_transitive_packages_are_foundHOMEwith.yarnrc.yml=nodeLinker: node-modulesvendor::lock_inventory(3),vendor::npm_flavor(5),vendor::refusals::tests::vlt(1),vendor::yarn_berry_lock(1)YARN_NODE_LINKER=pnp yarn_node_linker=node-modules YARN_PNPM_STORE_FOLDER=… YARN_RC_FILENAME=… YARN_INJECT_ENVIRONMENT_FILES=…(crawlers::)(Counts are
cargo test -p socket-patch-core --lib.)Root cause
YarnEnv::current/for_project(crates/socket-patch-core/src/crawlers/pkg_managers.rs) readstd::env::vars_os()andutils::fs::home_dir()directly, with no test seam.Fix
Under
cfg(test)only,YarnEnvreads theYARN_*vars,HOME/USERPROFILEand${NAME}expansion variables throughtest_env::guarded_var, i.e. only values a liveEnvGuardsets. That is the patternutils::go_envalready uses for Go (#2057). Tests that set these vars viaEnvGuardstill see them (the existingHOME-guarded npm_crawler test passes). Non-test builds are unchanged, including the CLI integration tests, which link the non-test core.New test
yarn_env_sees_only_guarded_variables(#[serial]) pins both halves: guarded values are seen, and nothing is seen once the guard drops.Proof
HOME-yarnrc +YARN_NODE_LINKER=node-modules+YARN_RC_FILENAME=.nope.ymlcombined.cargo clippy --locked -p socket-patch-core --all-features -- -D warnings(the CI clippy invocation) is clean.--all-targetsreports pre-existing lints in other files only, none in this file.rustfmt --checkis clean on the touched file.Review (
/code-review high+ Bugbot)Fixed in 3007a1c:
${NAME}expansion (base_var) still read the host env; Windows key-case matching (also Bugbot's finding); env read once perYarnEnv::current; the test now coversYARN_RC_FILENAMEand${NAME}.Not changed, and why:
cfg(test)forks. This is the same seamutils::go_envuses (Keep Go unit tests off the host's Go env #2057). The non-test bodies are one-linestd::env::vars_os()/home_dir()/std::env::var. Threading an env through every crawler, vendor and vex caller is a much larger change.test_env.rs. Open Keep composer unit tests off the host's env #2064 edits that file. Sharing the helper can follow once it merges.BerryRegistrySettings::read(patch/redirect/upstream/npm.rs) still readsYARN_RC_FILENAME/HOME/YARN_NPM_REGISTRY_SERVERfrom the host. That's the redirect restore path, a separate follow-up below. No core test failed under the hostile envs above.Not in this PR (left for #1919)
BerryRegistrySettings::readhost env reads (see above).VIRTUAL_ENVset to a real venv still fails 5vendor::pypitests (pdm/pipenv stale-install). That's a different code path (python site discovery).A
/tmp/.npmrc(an ancestor of the test tempdir) still failspnpm_layout::tests::every_reader_finds_a_store_in_the_configured_modules_dirthrough the unbounded.npmrcancestor walk.Where tests run
No test was removed or moved. One unit test was added; it runs wherever the core lib tests already run.
https://claude.ai/code/session_011f9aeSopYBBeiUVSewPUqN
Generated by Claude Code