test(e2e): Never let a failing command hide its output - #1097
Open
KeyZer wants to merge 1 commit into
Open
Conversation
Both E2E failures on master (run 32400669447) produced logs that stop on
`Verifying shim version...` — no exit code, no output, no assertion message,
290 KB of trace logging and the actual error nowhere in it. The cause was in
`test_shim`:
ver=$("$shim" "$version_arg" 2>&1) # set -e aborts the test here
shim_rc=$? # never runs
echo " exit=$shim_rc" # never runs
echo " output=$ver" # never runs, the error dies with $ver
Under `set -e` a failing command substitution aborts immediately, so every
`if [[ $rc -ne 0 ]]` below it was dead code. `test_bin`, `install_tool`,
`install_backend` and 9 test scripts had the same shape.
`run_probe` now runs a command and records `RUN_RC` / `RUN_OUT` / `RUN_ERR` /
`RUN_ALL` without aborting, `echo_probe` writes all of it to the log, and
`assert_probe_ok` names the command that failed. Keeping the streams apart also
stops `proto bin`'s stdout path from being polluted by whatever it logs.
An `errtrace` ERR trap reports anything still unhandled, with the failing
command, its file:line and the call stack. Failures inside a command
substitution are skipped via `BASH_SUBSHELL`, so tests that deliberately
tolerate a failure (41-bin-shim) stay quiet.
On failure the harness now leads with the `ASSERT FAIL` / `COMMAND FAILED`
lines instead of burying them under the tail, and writes
`.logs/<name>.postmortem.txt` with the shared `.prototools`, the shim registry,
the shims dir and `proto status` — captured while that state still exists, and
none of it visible in a test's own log. Under GitHub Actions it also emits
`::error` annotations and a step summary, so a red job no longer says only
"Process completed with exit code 1".
New knobs: `E2E_SERIAL=1` (never run a group in parallel — the first thing to
reach for when a failure smells like tests interfering with each other),
`E2E_TAIL`, `E2E_KEEP_SCRATCH`.
Two things found while testing this:
`PROTO_HOME` pointed at `$HOME/.proto`, so `just test-e2e` installed 20+ tools
into the developer's own store, then 90-uninstall removed their `uv` and
99-clean pruned it. `e2e/.gitignore` already lists `.proto-home/` and the README
already documented that location, so this is drift being closed rather than a
new choice; `E2E_USE_REAL_HOME=1` opts back in.
proto switches to NDJSON when it detects an AI agent, so running the suite from
inside Claude Code or Codex failed on `assert_executable` with a JSON envelope
where a path should be. `env.sh` now blanks the same env vars
`crates/core/src/test_utils.rs` does and pins `PROTO_REPORTER=text`.
The shared scratch cwd is deliberately left alone: 9 parallel installs pinning
into one `.prototools` is what caught a real lost-update bug in proto, and
isolating each test would have hidden it.
Contributor
Author
|
The It is also the clearest demonstration of why this PR exists. Everything needed to diagnose it is in the run without opening a single log:
The same two legs are green on #1096, which carries the fix. So this one should go green once #1096 lands — happy to rebase on top of it if you'd rather see that before merging, or it can just wait its turn. |
Contributor
|
@KeyZer Can you rebase master and try again. |
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.
Refs #1095, independent of #1096 (either can land first).
The E2E logs for both failures in run 32400669447 end on
Verifying shim version...— no exit code, no output, no assertion message, 290 KB of trace logging and the actual error nowhere in it. The cause is inlib/utils.sh:Under
set -ea failing command substitution aborts immediately, so everyif [[ $rc -ne 0 ]]below it was dead code, and the same pattern was intest_bin,install_toolandinstall_backend.Changes
run_probe/echo_probe(lib/assert.sh) — run a command, recordRUN_RC/RUN_OUT/RUN_ERRkeeping the two streams apart, never abort the caller.test_binandtest_shimnow log the exit code, the output and the stderr before asserting on any of it. Keeping stderr separate also meansproto bin's path can't be polluted by whatever it logs.ERRtrap with a stack trace (lib/assert.sh) — any unhandled failure now prints the command,file:lineand the call stack. Failures inside a command substitution are skipped (BASH_SUBSHELL), as they are either handled by the caller or reported again by the assignment around them, so tests like41-bin-shimthat deliberately tolerate failures stay quiet.Post-mortem on failure (
run.sh) — writes.logs/<name>.postmortem.txtwith the shared.prototools, the shim registry, the shims dir andproto status, captured while the state still exists. None of that is visible in a test's own log, and it is where cross-test failures actually live. The failure report also leads with theASSERT FAIL/COMMAND FAILEDlines instead of burying them under the tail.GitHub Actions annotations + step summary (
run.sh) — a red E2E job showed nothing beyondProcess completed with exit code 1, not even which test failed. Failures now emit::errorannotations carrying the assertion message, and the step summary lists every failed and skipped test with its cause. Both are guarded onGITHUB_ACTIONS/GITHUB_STEP_SUMMARY, so a local run is unchanged.E2E_SERIAL=1(run.sh) — never run a group in parallel. The first thing to reach for when a failure smells like tests interfering with their peers.E2E_TAIL,E2E_KEEP_SCRATCH(run.sh) — inline tail size, and keeping the shared work dir so it can be poked at afterwards.PROTO_HOMEdefaults toe2e/.proto-home(lib/env.sh) — it pointed at$HOME/.proto, sojust test-e2einstalled 20+ tools into the developer's own store, then90-uninstallremoved theiruvand99-cleanpruned it.e2e/.gitignorealready lists.proto-home/and the README already documented that location, so this is the drift being closed rather than a new choice.E2E_USE_REAL_HOME=1opts back in.README —
PROTO_BIN_DIRdoesn't exist in any script, the group names were stale (install-base/install-depsvstools/tools-secondary/backends), and the store is not "wiped at the start of each run". Documents the knobs and how to debug a failure.Also found while testing this
Running the suite from inside an AI agent (Claude Code, Codex, Cursor) failed on
assert_executable, because proto detects the agent and switches its output to NDJSON — soproto bin nodereturned a JSON envelope instead of a path.lib/env.shnow blanks the same env varscrates/core/src/test_utils.rsdoes and pinsPROTO_REPORTER=text, so the suite behaves the same whoever runs it.Deliberately not changing
The shared scratch cwd. Nine parallel installs pinning into one
.prototoolsis what caught a real lost-update bug in proto; isolating each test would have hidden it.