Generalize subprocess stub tests to support multiple platforms#1362
Merged
Conversation
`src/daemon/__tests__/runtime-hints.test.ts` and `src/platforms/apple/core/__tests__/index.test.ts` inject stub `adb`/`xcrun` binaries by mutating `process.env.PATH` and then spawn them, so each case waits real subprocess time. Run in the `unit-core` project at ~7x file parallelism they contend for CPU with every other stub-spawning file; a starved stub spawn pushes production down a generic failure path that returns a different error than the assertion expects. The failures present as assertion errors (not timeouts) and the failing subset shifts between runs — a pre-existing contention flake, reproducible on a clean tree. The repo already serializes the android scripted-adb tests into their own `fileParallelism: false, maxWorkers: 1` project for exactly this reason. This generalizes that project (renamed `android-adb` -> `subprocess-stub`) and adds the two files, so at most one real-stub-spawning file runs at a time across the whole suite. Assertions no longer let host wall-clock decide which error path runs. Option of injecting the spawn budget instead was not viable: runtime-hints passes no timeout to inject, and adding an exec-options seam purely for the test would be a test-only DI seam the CI gate forbids (AGENTS.md). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFepuH3Rh6ciRkvYJSCCtU
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
Member
Author
|
Reviewed |
thymikee
added
the
ready-for-human
thymikee
pushed a commit
that referenced
this pull request
Jul 22, 2026
…lakes src/__tests__/client-metro.test.ts stubs npx and the package managers on PATH and spawns a real Metro dev server per case, so each case waits real subprocess time (~570-910ms measured). Run in the unit-core project at ~7x file parallelism it contends for CPU with every other stub-spawning file; a starved spawn pushes production down a generic failure path that returns a different error than the assertion expects. This is the same contention flake #1362 serialized runtime-hints.test.ts and apple/core/index.test.ts for, but this file was not included in that batch (observed failing a full test:unit run while passing 20/20 in isolation and green on a re-run). Add it to SUBPROCESS_STUB_TESTS so it joins the fileParallelism:false, maxWorkers:1 subprocess-stub project, so at most one real-stub-spawning file spawns at a time. Injecting the spawn budget is not the lever here: the fake dev server becomes ready fast (no waited-out timeout to inject), the per-case cost is a genuine subprocess spawn, and each case is already under the 2.5s slow-test budget so the gate never flags it. Removing the cost would mean mocking the very spawn/args/package-manager-detection path the file exists to verify, and an exec-options DI seam is forbidden by the CI gate (AGENTS.md). Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK
azaj01
pushed a commit
to azaj01/agent-device
that referenced
this pull request
Jul 23, 2026
…t daemon env (callstack#1365) * test: serialize client-metro subprocess-stub test to end contention flakes src/__tests__/client-metro.test.ts stubs npx and the package managers on PATH and spawns a real Metro dev server per case, so each case waits real subprocess time (~570-910ms measured). Run in the unit-core project at ~7x file parallelism it contends for CPU with every other stub-spawning file; a starved spawn pushes production down a generic failure path that returns a different error than the assertion expects. This is the same contention flake callstack#1362 serialized runtime-hints.test.ts and apple/core/index.test.ts for, but this file was not included in that batch (observed failing a full test:unit run while passing 20/20 in isolation and green on a re-run). Add it to SUBPROCESS_STUB_TESTS so it joins the fileParallelism:false, maxWorkers:1 subprocess-stub project, so at most one real-stub-spawning file spawns at a time. Injecting the spawn budget is not the lever here: the fake dev server becomes ready fast (no waited-out timeout to inject), the per-case cost is a genuine subprocess spawn, and each case is already under the 2.5s slow-test budget so the gate never flags it. Removing the cost would mean mocking the very spawn/args/package-manager-detection path the file exists to verify, and an exec-options DI seam is forbidden by the CI gate (AGENTS.md). Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK * test: make the vitest suite hermetic against ambient daemon env A machine actually running agent-device — including this repo's own remote dev containers — exports AGENT_DEVICE_DAEMON_BASE_URL and AGENT_DEVICE_DAEMON_AUTH_TOKEN pointing at a live daemon. Production flag-default resolution folds those into every command's input and connection config (resolveConfigBackedFlagDefaults -> readEnvFlagDefaults, plus the daemon client's own env fallbacks in daemon-client-lifecycle), so a configured host silently diverges from CI, which runs with them unset: - command-tools and cloud-connect-profile assert exact command input / profile shapes and gain phantom daemonBaseUrl/daemonAuthToken keys. - daemon-client and daemon-client-lifecycle take the remote-daemon path ("Remote daemon is unavailable") instead of the local one they exercise. 28 tests across those four files fail deterministically on such a host — fast, in isolation, not contention — while passing on CI. Add a shared setup file that deletes the two ambient daemon-connection vars before each test file, wired into every vitest project alongside the existing process-memo reset, so a configured host matches CI. Tests that need these set assign their own value or pass an explicit env object, which runs after this setup module loads and is therefore unaffected. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK * test: add a regression guard for the hermetic-env setup CI runs with AGENT_DEVICE_DAEMON_* unset, so it can never exercise the scrub that hermetic-env-setup.ts performs — deleting the setup file or forgetting to wire a project would leave the whole suite green. Add a guard that closes that gap two ways: - A static check that every configured vitest project lists the setup file in its setupFiles (catches an unwired project). - A real vitest child, launched with both daemon vars set, running a probe fixture that proves a wired project sees them scrubbed — plus a negative control (same vars, setup disabled) proving the probe actually detects the leak, so a green result is the setup working, not a no-op. A control var the setup must not touch proves the child inherited the injected environment. The two children run concurrently to stay within the unit wall-clock budget, and the file joins the serialized subprocess-stub project since it spawns real vitest processes. Ignore the fixture config's default export in fallow — vitest loads it by path rather than importing it, the same class as the other tool-config default exports already listed there. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK * fix(test): route the hermetic-env guard's vitest child through runCmd AGENTS.md hard rule: TypeScript process execution must go through src/utils/exec.ts, never raw spawn/spawnSync. The guard's runProbe spawned the vitest child with node:child_process spawn directly, bypassing the shared timeout, process-tree cleanup, normalized failure, and diagnostics behavior. Replace it with runCmd invoking the repo-local vitest CLI (node node_modules/vitest/vitest.mjs) with allowFailure: true and a bounded timeoutMs, asserting the returned exitCode. allowFailure surfaces the negative control's non-zero exit as data instead of throwing, so that assertion keeps working; the two probes still run concurrently to stay under the unit budget. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK * fix(test): make the hermetic-env guard's probe cleanup timely and complete Follow-up to review. Two process-tree cleanup gaps in the guard's runProbe: - It gave the nested vitest a 60s timeout while the enclosing test used vitest's default (5s) timeout, so a hung probe would time the parent test out first and leave the child running (orphaned). - runCmd only process-group-kills when detached:true; without it a timeout kills only the direct child, so vitest worker descendants could survive. Run the probe detached so a timeout kills the whole process group (the vitest child and its workers), and set the child timeout (20s) strictly below an explicit enclosing test timeout (40s) so a hang is reaped by runCmd before vitest abandons the parent test. The repo-local node/vitest invocation, allowFailure, exit-code assertions, and the concurrent positive/negative probes are unchanged. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK * test: replace the child-process hermetic-env guard with an in-process test The previous guard spawned a real vitest child to exercise the setup through vitest's setupFiles machinery, which dragged in process-group cleanup, timeout ordering, a fixture config + probe, and a fallow suppression — a pile of scaffolding around a scrub that is two delete statements. Prove the same contract in-process instead: - Behavior: set the daemon vars, vi.resetModules(), re-import hermetic-env-setup, and assert they are gone. This exercises the real import-time scrub on any host (CI included, where the vars are otherwise absent) and fails if it regresses. - Wiring: assert every configured vitest project lists the setup in setupFiles. Runs in ~5ms in unit-core with no subprocess, so it also drops out of SUBPROCESS_STUB_TESTS and removes the fixtures and the fallow ignore entry. Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGCchHyrPhTqtXniWuERYK --------- Co-authored-by: Claude <noreply@anthropic.com>
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
2 participants
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.
Summary
Rename and expand the Android-specific ADB stub test configuration to support subprocess stubbing across multiple platforms (Android, iOS, and daemon). This change reflects the actual scope of tests that stub real binaries and wait for real subprocess execution time.
Key Changes
ANDROID_ADB_STUB_TESTSconstant toSUBPROCESS_STUB_TESTSand converted to an array to support multiple test file patternssrc/daemon/__tests__/runtime-hints.test.tssrc/platforms/apple/core/__tests__/index.test.tsandroid-adbtosubprocess-stubto reflect its broader scopepackage.jsontest scripts andAGENTS.mddocumentationImplementation Details
The tests in this group stub real binaries (adb/xcrun) by mutating
process.env.PATHand spawn them, causing real subprocess/retry/poll delays. Running these tests with standard parallelism causes CPU contention that starves the stub spawns, leading to flaky failures. By running this group serialized with per-file isolation (fileParallelism: false,isolate: true), only one test file spawns stubs at a time, preventing contention-based flakes.https://claude.ai/code/session_01NFepuH3Rh6ciRkvYJSCCtU