test(highway): build the socket-derivation env explicitly instead of inheriting it (etuz) - #312
Merged
Conversation
…inheriting it model_performance-etuz, found by lane teel. This is MY defect: I wrote and merged this file in 3bb0104 (#310), and it passed in CI while failing inside every highway lane -- the worst possible direction for a test to be wrong in. CAUSE. _derived_socket() ran the scripts' own `${HIGHWAY_TMUX_SOCKET:-...}` derivation through `bash -c`, inheriting the ambient environment. A highway lane EXPORTS HIGHWAY_TMUX_SOCKET (that is the whole point of the per-batch socket cutover), so inside a lane the ambient value satisfied the `:-` default and the derivation under test never ran. Both default-derivation tests then asserted against the ambient socket name and failed. FIX. Build the subprocess env explicitly: strip HIGHWAY_TMUX_SOCKET for the default-derivation cases, and pass it deliberately via an `explicit=` argument for the backward-compatibility case that is actually about an explicit value winning. The tests no longer depend on what the caller's shell happens to export. Also adds test_the_default_derivation_ignores_an_ambient_socket, which sets a decoy ambient value and asserts the derivation still wins -- so this failure mode is a test rather than a surprise the next lane rediscovers. Verified BOTH ways, because one direction alone would have hidden the original bug: with HIGHWAY_TMUX_SOCKET=hw-model-performance set (the condition that failed) -> 12 passed; with it unset (how CI ran, green all along) -> 12 passed.
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.
My own defect, found by another lane
I wrote and merged
tests/test_ten_lane_highway_socket_default.pyin3bb0104(#310). It passed in CI and failed inside every highway lane — the worst direction for a test to be wrong in. Laneteelhit it and filedmodel_performance-etuzrather than absorbing it.Cause
_derived_socket()ran the scripts' own\${HIGHWAY_TMUX_SOCKET:-...}derivation throughbash -c, inheriting the ambient environment. A highway lane exportsHIGHWAY_TMUX_SOCKET— that is precisely what #310 introduced — so inside a lane the ambient value satisfied the:-default and the derivation under test never ran. Both default-derivation tests then asserted against the ambient socket name.Fix
Build the subprocess env explicitly: strip
HIGHWAY_TMUX_SOCKETfor the default-derivation cases, and pass it deliberately viaexplicit=for the backward-compatibility case that is genuinely about an explicit value winning.Adds
test_the_default_derivation_ignores_an_ambient_socket, which sets a decoy ambient value and asserts the derivation still wins — so this failure mode is now a test rather than a surprise the next lane rediscovers.Verified both ways
Checking only one direction is what let the original bug through:
HIGHWAY_TMUX_SOCKET=hw-model-performance(the failing condition, inside a lane)Test-only change; no source file touched.