test: stop tests binding machine-wide resources they do not own - #714
Draft
latekvo wants to merge 8 commits into
Draft
test: stop tests binding machine-wide resources they do not own#714latekvo wants to merge 8 commits into
latekvo wants to merge 8 commits into
Conversation
…owns them Three flow/profiler tests reached into the machine-wide tmpdir under a fixed name, so a second concurrent run of the same file perturbed them. - flow-visual: the sub-pixel crop test scanned os.tmpdir() for leftover `argent-flow-crop-*` dirs. A neighbour's in-flight crop dir is not in the pre-run snapshot, so it read as this run's leak. Point TMPDIR at a scratch dir for the test and sweep that instead. - flow-chromium-boot: writeFlow tagged its own files with the pid, but a `run:` sibling must keep the name the directive targets, so writeSiblingFlow wrote a shared <tmpdir>/nested-chromium.yaml that a neighbour's afterEach deleted mid-test. Move every flow file into a per-run mkdtemp dir. - react-profiler/dump: getDebugDir() resolves the shared /tmp/argent-profiler-cwd, into which the tests write and unlink fixed filenames. Redirect TMPDIR per test, matching profiler-load-list-e2e.
Both ran real, unmocked code against a fixed endpoint they do not own. - stop-tools: the "no process on port" case drove the real stop-metro tool at port 59999, which `lsof`s the port and SIGTERMs every pid it finds. Anything listening there failed the assertion and got killed. Probe port 1 instead — privileged, so the lookup finds nothing and the kill path stays unreachable. - native-devtools-factory-cleanup: the fixed UDID resolved to the shared /tmp/argent-nd-FACTORY1.sock, and the factory's bind unlinks and rebinds over the current holder, orphaning a concurrent run's live socket. Derive the UDID from the pid so each run binds its own path.
…l home - bind-failure-telemetry, lens-relay-telemetry: start() gates its event log on the `tool-server-event-log` flag, read from the real ~/.argent/flags.json. With that documented flag enabled all four tests fail and unit-test records are appended to the developer's own event log. Pin the flag read off, the way artifacts and four other suites already do. - debugger log-writer suites: LogFileWriter's constructor mkdir -p's os.homedir()/.argent/tmp, so running the suite created that directory in the real home. Redirect HOME/USERPROFILE to a temp tree, as avd-snapshot does. - file-inputs, http-upload: both snapshot os.tmpdir() by prefix and diff it after, so a concurrent run's in-flight scratch entry reads as a leak. In http-upload the check sits inside vi.waitFor, turning that into a timeout. Scope TMPDIR to the test and scan only what it owns.
… SDK tv-remote's Vega block and the two native-profiler analyze suites drive tools whose dependency gate resolves a real `adb` from PATH, $ANDROID_HOME or the default SDK install locations. On a host without the Android SDK those 19 tests fail with DependencyMissingError, so they were asserting the machine had an SDK rather than anything about the code. Prime the dep cache the way boot-device, await-ui-element and await-screen-idle already do.
Each of these read an environment variable a developer or CI plausibly exports, so the assertion described the machine rather than the code. - telemetry/index: DO_NOT_TRACK is a consortium-standard opt-out; exported, it disables consent and fails 13 tests. Snapshot and clear it, as consent and notice already do. - telemetry/base-props: detectCloudAgent ranks claude_code, cursor and copilot ahead of replit, and those are the literal env of such a cloud runner, so the replit case resolved to the wrong agent there. Clear every higher-ranked signal before setting REPLIT_AGENT. - argent-installer: detectPackageManager reads npm_config_user_agent, so running the suite under pnpm/yarn/bun changed the asserted command and failed 6 tests. It also silenced uninstall's `bin === "npm"` throw, leaving the "uninstall fails" tests exercising the success path. Pin the agent unset. - adb-resolve-avd-path: pinned only ANDROID_AVD_HOME, rank 2 of the 5 roots resolveAvdPath consults. Pin the whole set avd-snapshot pins. - android-binary-windows: saved HOME but never assigned it, so the home-derived Windows SDK root stayed ambient. Point it at the temp root.
startCapture names its output join(os.tmpdir(),
`argent-screen-recording-${deviceId}-${Date.now()}.mp4`). The device ids are
fixtures and the suite's fake timers freeze Date.now(), so every run derived
one identical path — and each test ends by removing it. Two concurrent runs
deleted each other's recording mid-test, failing stopCapture's non-empty-output
guard with ENOENT (2 of 3 paired runs before, 0 of 4 after). Scope TMPDIR per
test so each run writes into a directory it owns.
…al home LogFileWriter's constructor mkdir -p's os.homedir()/.argent/tmp and takes only a port, so there is no seam but HOME. Five more suites reached it through the JS-runtime-debugger, chromium-JS-runtime-debugger and network-inspector blueprints, so the earlier per-file redirect covered only the three that build a writer directly and a full suite run still created that directory. Extract the redirect as scopeTempHome (the shape telemetry's test helpers already use) and apply it to all eight. A full tool-server run now leaves ~/.argent untouched.
…t base sun_path caps a unix socket path at 108 bytes. An ambient TMPDIR of 80-odd characters pushed the mkdtemp-based fixture paths past that and failed three binds with EINVAL. Production pins the same short base for this reason.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Three flaky or failing tests were diagnosed separately this week, and all three turned out to be one defect wearing different clothes: a test binding a machine-wide resource it does not own, so a second concurrent run — another checkout, a CI matrix cell, a developer's own app — perturbs it. Those three are fixed in #707, #709 and #711.
Each of those was found by chasing a symptom, and each audit covered only its own resource type. This is the systematic sweep: all 400 test files across 13 packages, plus the 4
node --testscripts.Findings
os.tmpdir()namespace$HOME/~/.argentARGENT_*)srcreaderspgrep/pkill/ fixed-pattern sweeps)Every finding was reproduced with a concrete instrument before being touched, and the instrument re-run after. Representative:
stop-tools.test.tsran the realstop-metroagainst port 59999, whichlsofs and SIGTERMs whatever it finds. With a squatter there:16 passed→1 failed, and the squatter loggedreceived SIGTERM #1. Now port 1.bind-failure-telemetry/lens-relay-telemetrywere gated on the real~/.argent/flags.json. Enabling the documentedtool-server-event-logflag:4 passed→4 failed, and records were appended to the developer's real event log.mkdir -psos.homedir()/.argent/tmp.rm -rf ~/.argent/tmpthen a full suite run: recreated. After: not recreated.screen-recording.test.ts— fake timers freezeDate.now()and device ids are fixtures, so every run derived one identical.mp4path that each test then deletes. Self-concurrent: 2 of 3 rounds failed → 0 of 4.DO_NOT_TRACK=1→ telemetry282 passed→13 failed.npm_config_user_agent=pnpm|yarn|bun→ installer535 passed→ 4/5/3 failed. 19 tests required a realadb— PATH shadow + empty HOME:30 passed→19 failed./tmp/argent-nd-FACTORY1.sockdestroyed a concurrent holder's live socket (proven by inode).flow-visual,flow-chromium-boot,react-profiler/dump,file-inputs,http-upload— shared/tmppaths and prefix scans, each reproduced with a squatter. Inhttp-uploadthe collision surfaced as a timeout, insidevi.waitFor.Clean, with the evidence
LD_PRELOADexec interposer over a full run (507 execs) plus all 12 other suites: nopgrep/pkill/killall/fixed-pattern sweep anywhere, and no test signals a pid it did not spawn (excludingvega-cli-timeout.test.ts, which is test(tool-server): scope vega-cli-timeout sentinels to the running process #711's).adb/simctl/emulator/avdmanager/vega; no test asserts a device list.pool: "forks"/isolate: true, confirmed by a scratch experiment (distinct pids, no env leak, moduleMap= 1,1,1). 120 default-mode runs across 12 packages produced 1 failure.scripts/— 4 files, 86 tests:mkdtempthroughout, no ports, no sweeps, writes nothing.Verification
Each fix was mutated back and killed by a named test under its instrument — e.g.
× reads a plain absolute path,× returns null when only the extensionless binary exists,× errors a nested e2e flow's launch…. Restores verified withgit diff/git status, never a substring grep.One honest exception: the
/tmp/argent-nd-FACTORY1.sockfix is not pinned by any suite assertion — reverting it still passes, and only the external inode instrument catches it. It is a pollution fix, in the same category as the~/.argentwrites.Gates, unpiped and read by exit code:
eslint .0,prettier --check .0,tsc --build0,typecheck:tests0.Suite: 298 files, 3107 passed, 1 skipped, 0 failed. All 12 other packages and
test:scriptsgreen, identical to baseline. Two concurrent full suites now fail only inboot-device-hotbootandvega-cli-timeout— both already fixed in #707 and #711.Scope
This branch is test-only — no
src/file is touched. The one production defect the sweep turned up (pswithout-wworphaning live tool-servers) is separated into #713, so it can be reviewed as the user-facing fix it is.Left untouched on purpose:
boot-electron-spawn-error.test.ts,chromium-discovery.test.ts,vega-cli-timeout.test.ts,boot-device-hotboot.test.tsandvitest.config.ts— all owned by #707 / #709 / #711, and all still carrying their defects on this base.Not fixed, deliberately — an adjacent class (test-order dependence, not machine-global resources), reproducible if wanted:
link-config.test.ts(deterministic:-t "returns null when required field"→ 4 failed),network/network-integration.test.tsandandroid-perfetto/dispatch.test.ts(fail under--sequence.shuffleseeds).Also observed, not addressed:
/tmpis accumulating roughly 2650argent-flow-crop-*, 1168argent-events-*and 584argent-file-input-*directories. A separate leak.