test(tool-server): stop unit tests inheriting the developer's ARGENT_* overrides - #707
test(tool-server): stop unit tests inheriting the developer's ARGENT_* overrides#707latekvo wants to merge 1 commit into
Conversation
…* overrides `selectGpuMode` reads ARGENT_EMULATOR_GPU_MODE before falling back to the platform default, so a shell exporting `ARGENT_EMULATOR_GPU_MODE=host` — the documented escape hatch for hosts where swiftshader is too slow — made the six platform-parameterised `-gpu` assertions in boot-device-hotboot resolve `host` instead of `swiftshader`/`auto` and fail. The same class bites elsewhere: ARGENT_EMULATOR_NO_WINDOW fails four more in that file, ARGENT_SIMULATOR_NO_WINDOW fails the Simulator.app attach assertion in boot-device, and ARGENT_PORT/ARGENT_HOST fail both bind-failure-telemetry tests. Clear the whole ARGENT_* prefix from a suite-wide setup file so assertions test the shipped defaults. Tests that exercise an override already set it themselves. Also reorder the four `it.each(PLATFORMS)` titles, whose `%s` placeholders ran against the [platform, gpu] tuple and rendered as "-gpu linux on swiftshader".
|
Arrived at this independently while sweeping out-of-scope findings from the #610 review, and opened a duplicate (#708) three minutes after this one - closing mine in favour of this. Same diagnosis, same fix shape (blanket Per-variable attribution, one full-suite run each, on
Two things that may be worth folding in:
Verified against this PR's approach: with the setup file in place, the full suite is Unrelated flake found by the same sweep, recording it rather than guessing at it: |
Problem
The unit suite inherits the developer's
ARGENT_*shell configuration, so tests that assert argent's shipped defaults instead assert a property of the machine running them.I hit this as six failures in
boot-device-hotboot.test.tson a Linux box:swiftshaderis the Linux default andautothe macOS one.hostis neither — it came fromARGENT_EMULATOR_GPU_MODE=hostexported in my shell, which is the documented escape hatch for hosts where swiftshader is too slow.selectGpuMode(boot-device.ts:210) reads the env override and returns before theprocess.platform === "linux" ? "swiftshader" : "auto"fallback. That precedence is correct and is not what this changes. Theit.each(PLATFORMS)parameterisation is also fine —setPlatformreally does controlprocess.platform; the env override simply shadows it.Proof that the source is not implicated:
The failure reads as a source regression
This is the reason it is worth fixing rather than documenting. Someone who has ever set
ARGENT_EMULATOR_GPU_MODE— the variable we tell people to set — gets six red tests naming a-gpuvalue they did not choose, in a file they did not touch, and nothing in the output mentions the environment. The obvious next move is to go readselectGpuMode, which is correct.Not one file
ARGENT_EMULATOR_GPU_MODEis the one I tripped over; the class is everyARGENT_*override, since each is by definition a user-facing switch that changes asserted behaviour. Measured by exporting each and re-running:ARGENT_EMULATOR_GPU_MODEboot-device-hotboot.test.tsARGENT_EMULATOR_NO_WINDOWboot-device-hotboot.test.tsARGENT_SIMULATOR_NO_WINDOWboot-device.test.tsopen -a Simulator.appit asserts)ARGENT_PORT/ARGENT_HOSTbind-failure-telemetry.test.tsbind-failure-telemetry.test.tsis the pointed one: its header comment already worries about colliding with the developer's real tool-server and mocks the probe for exactly that reason — but it never considered the env override that retargets the bind.With all eleven
ARGENT_*variables set at once, across the three files:Fix
A suite-wide setup file deletes every
ARGENT_*key fromprocess.envbefore the test module graph is imported, so module-level env reads see the cleared state too.Clearing the prefix rather than a hand-maintained allowlist is deliberate: an override added to
srclater is covered without a second edit here, which is precisely the kind of follow-up edit that does not happen. A test that exercises an override sets it itself, so nothing depends on the ambient value — verified by the full suite staying green.Also reorders the four
it.each(PLATFORMS)titles. Their%splaceholders ran against the[platform, gpu]tuple and rendered as"picks the hot-boot spawn args + -gpu linux on swiftshader"— platform and GPU mode transposed, in the first string a diagnoser reads.Deliberately not scrubbed
ANDROID_HOME,ANDROID_SDK_ROOT,ANDROID_AVD_HOME,ANDROID_USER_HOME,ANDROID_SDK_HOME,XDG_CONFIG_HOME,LOCALAPPDATA. I ran the full package suite with all seven pointed at nonexistent paths: 297/297 green, so those tests already stub what they need. Scrubbing them would be defensiveness nothing pins, and they are shared SDK variables a test could legitimately want to read.Tests that pin
process.platformexplicitly and restore it (android-binary-windows,command-on-path,lens-tools-platform-gate,linux-preflight) are correct as written and untouched.adb-resolve-avd-path.test.ts:54'sit.skipIf(process.platform !== "win32")is a deliberate gate on Windows-only path parsing and stays.Verification
-gpufailures reproduced atorigin/mainwith the variable exported, and pass with it cleared — same command, same shell.selectGpuModefromswiftshadertoautois killed by four namedon linux …tests, while thedarwinarm stays green. Without this change that mutation is masked whenever the variable is set.packages/tool-server: 297 files, 3090 passed, 1 skipped, 0 failed (was6 failed | 3084 passed).eslint .0,prettier --check .0,tsc --build0 — run unpiped, exit codes read directly.Unrelated, found while sweeping
test/vega-cli-timeout.test.tsfails roughly 1 run in 5 atorigin/mainwith this change absent — it touches onlyPATH, neverARGENT_*. Two real-subprocess timing tests (rejects on its own deadline when the CLI never returns,reaps the ENTIRE worker tree on timeout) racing under parallel load. Not caused by this branch and not fixed here.