test: propagate the client's environment to forked test processes - #621
Merged
Conversation
`FOO=bar bleep test` could not reach a test. Tests are forked by the BSP daemon, which is long-lived and shared across workspaces, so its environment belongs to whichever shell cold-started it — possibly days ago in another directory. The daemon does not mutate its own environment to fix this. That would race between concurrent requests and leak one workspace's values into another's test run. Instead the client sends its environment on the BSP request (`TestOptions.env`, on the existing `bleep-test-options` dataKind) and the daemon applies it as an overlay on the forked child only. The hole was already there: `JvmPool.spawnJvm` applies an env map to the child's ProcessBuilder and `JvmKey` already hashes it, so a differing environment forces a distinct fork rather than reusing a poisoned one. Every call site simply passed `Map.empty`. Applied on every platform — JVM, Scala.js, Scala Native, Kotlin/JS, Kotlin/Native. The four non-JVM runners already accepted an env map and applied it to their ProcessBuilder; because nothing ever passed one they did not honour `platform.jvmEnvironment` either. Same omission in all three `bleep run` paths and in the BSP run handler, which additionally ignored the standard `RunParams.environmentVariables` outright. Precedence, weakest first: NO_COLOR=1, then the client's shell, then `platform.jvmEnvironment`. The build deliberately outranks the ambient shell: the environment is forwarded wholesale, so letting it win would let a stray AWS_REGION in someone's profile silently override a region the build states on purpose, failing on exactly one machine. Variables the build does not mention pass through untouched. CLASSPATH is never forwarded — JvmPool uses it as the long-classpath channel on Windows and a forwarded copy would replace the test classpath. PWD/OLDPWD/_ are dropped as stale shell bookkeeping. PATH and JAVA_HOME are forwarded so tests that shell out find the same tools. The TestOptions codec is now hand-written and absent-tolerant. A derived codec does not fill in case-class defaults (verified: `Missing required field`), so adding `env` would make every older client's request fail to decode — and the failure path degrades to `TestOptions.empty`, silently dropping that run's --only/--exclude/--jvm-opt and running the wrong tests. Every field now reads through getOrElse. Also: add the missing `jvmEnvironment` to schema.json, document the behaviour, and delete `ReactiveTestRunner` (dead, no callers, and the second reader of jvmEnvironment that would have drifted). Not covered: Scala Native's TestAdapter path receives the environment but is untested here — the repo's own test for it has been ignored as a "known RPC protocol issue" since #496. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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
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.
FOO=bar bleep testcould not reach a test. Tests are forked by the BSP daemon, which is long-lived and shared across workspaces, so its environment belongs to whichever shell cold-started it — possibly days ago in another directory.Approach
The daemon does not mutate its own environment. That would race between concurrent requests and leak one workspace's values into another's test run.
Instead the client sends its environment on the BSP request (
TestOptions.env, on the existingbleep-test-optionsdataKind) and the daemon applies it as an overlay on the forked child only. The hole was already there —JvmPool.spawnJvmapplies an env map to the child'sProcessBuilder, andJvmKeyalready hashes it so a differing environment forces a distinct fork rather than reusing a poisoned one. Every call site simply passedMap.empty.Scope
Applied on every platform: JVM, Scala.js, Scala Native, Kotlin/JS, Kotlin/Native.
The four non-JVM runners already accepted an
envmap and applied it to theirProcessBuilder; because nothing ever passed one, they did not honourplatform.jvmEnvironmenteither. The same omission existed in all threebleep runpaths (Run.scala:101,132,164) and in the BSP run handler, which additionally ignored the standardRunParams.environmentVariablesoutright. All fixed.Precedence
Weakest first:
NO_COLOR=1→ the client's shell →platform.jvmEnvironment.The build deliberately outranks the ambient shell. The environment is forwarded wholesale, so letting it win would let a stray
AWS_REGIONin someone's profile silently override a region the build states on purpose — failing on exactly one machine. Variables the build does not mention pass through untouched.CLASSPATHis never forwarded (JvmPool uses it as the long-classpath channel on Windows; a forwarded copy would replace the test classpath).PWD/OLDPWD/_are dropped as stale shell bookkeeping.PATHandJAVA_HOMEare forwarded so tests that shell out find the same tools.Wire compatibility
The
TestOptionscodec is now hand-written and absent-tolerant. A derived codec does not fill in case-class defaults — verified rather than assumed:So adding
envwould have made every older client's request fail to decode, and the failure path inhandleTestdegrades toTestOptions.empty— silently dropping that run's--only/--exclude/--jvm-optand running the wrong tests without saying so. Every field now reads throughgetOrElse, so the next addition stays non-breaking too. (The pre-existingflamegraph = falsedefault was never actually tolerant either.)Tests
Every new test has a negative control, so a runner that ignored
enventirely could not pass a one-sided assertion. The end-to-end test also asserts the probe variable is absent from the harness JVM — otherwise the fork would just inherit it and the test would prove nothing.TestEnvPropagationIT(5) — end-to-end through the real BSP server, including precedenceScalaJsTestIntegrationTest(+2),KotlinTestRunnerEnvTest(4) — real Node / real binariesTestOptionsCodecTest(6),ClientEnvTest(6)Full build compiles (25 projects);
bleep-bsp-tests629 passed / 0 failed.Also
schema.jsonwas missingjvmEnvironmententirely — addeddocs/usage/environment-variables.mdx, wired into the sidebarReactiveTestRunner(dead, no callers, and the second reader ofjvmEnvironmentthat would have drifted). Verified with a clean rebuild ofbleep-core.Not covered
Scala Native's
runTestsViaAdapterreceives the environment (previouslyMap.empty) but is untested here: the toolchain artifacts were never fetched in this worktree, and the repo's own test for that path has beenignored as a "known RPC protocol issue" since #496. I also did not verify whether scala-native'swithEnvVarsreplaces or merges the environment.🤖 Generated with Claude Code