From 06b03d3d546990cbd0b27b398d3796bbc04a1549 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Mon, 27 Jul 2026 10:47:36 +0200 Subject: [PATCH] fix(runner): capture parallel worker stderr instead of discarding it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each file worker was spawned as `call_test_functions … 2>/dev/null &` since #358, to keep worker noise off the progress line. That also made the same run report differently depending on --parallel. The worker's stderr is now redirected to a per-file capture under the run output dir and rendered after aggregation as a `Stderr from ` block. The capture cannot live under TEMP_DIR_PARALLEL_TEST_SUITE: state::aggregate_parallel_results walks every entry there and would report "No tests found" for a stray file. Cleanup is the existing run-dir EXIT trap, so no per-file `rm` fork is added. Scope note: a test *body*'s stderr was never lost. execute_test_body runs the function as `"$fn_name" "$@" 2>&1`, so it is merged into the captured stdout and already surfaced in that test's failure block. What the redirect dropped is stderr with no owning test — data providers, hook plumbing, scratch-dir errors — which is why the new block is attributed to the file rather than to a test. The regression test compares occurrence counts between modes rather than just grepping for the message: the fixture's provider also runs in the main-shell counting pass, so the diagnostic leaks in either way and a bare assert_contains passes against the unfixed code. Closes #864 --- CHANGELOG.md | 1 + src/console_results.sh | 18 ++++++++ src/env.sh | 5 +++ src/runner.sh | 23 +++++++++- .../acceptance/fixtures/test_worker_stderr.sh | 16 +++++++ tests/acceptance/worker_stderr_test.sh | 42 +++++++++++++++++++ 6 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 tests/acceptance/fixtures/test_worker_stderr.sh create mode 100644 tests/acceptance/worker_stderr_test.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 473ffc44..09071efa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - The `--parallel` unsupported-OS warning no longer claims Alpine is excluded: Alpine has been a supported parallel platform since the race conditions were fixed, the message was simply never updated ### Fixed +- `--parallel` no longer discards stderr written inside a worker but outside a test body, which made the same run report differently depending on the mode. Each file worker had been spawned with `2>/dev/null` since #358 to keep its noise off the progress line; its stderr is now captured per file and rendered afterwards as a `Stderr from ` block, so data-provider diagnostics, hook-plumbing messages and scratch-dir errors survive. Output written by a test *body* was never affected — it is merged into that test's captured stdout and already appeared in its failure block (#864) - The minimum-bash gate now compares the minor version as well as the major, so it enforces whatever `BASHUNIT_MIN_BASH_VERSION` declares. It previously accepted any `3.x` regardless of the stated minimum, and a version string carrying a suffix (`5.2.37(1)-release`) is now parsed instead of tripping the comparison. The floor itself is unchanged at **Bash 3.0+**; `printf -v`, `+=` and `[[ =~ ]]` are now rejected in `src/` by the compatibility gate, since 3.0/3.1 lack the first two and 3.2 changed quoted-pattern semantics for the third - An empty entry in `.env` no longer overrides a value the caller exported or set on the command line. `.env` is sourced under `set -o allexport`, so every line was an unconditional assignment: merely *listing* a name blanked it, and `BASHUNIT_OUTPUT_FORMAT=tap ./bashunit` silently stopped working in any project whose `.env` mentioned that setting. An empty entry now means "not configured here"; an entry with a value still takes effect. This had been actively concealing defects — two of the bugs fixed in #879 were not reproducible from inside a repo checkout for exactly this reason (#865) - A malformed benchmark annotation is now an error instead of a silent fallback to the default: `@revs=abc` quietly ran a single revolution, `@its=abc` a single iteration, and `@max_ms=abc` dropped the threshold entirely so the benchmark could never fail — each reporting success while measuring something other than what was written (#884) diff --git a/src/console_results.sh b/src/console_results.sh index 4db4ff2f..5485e4a8 100644 --- a/src/console_results.sh +++ b/src/console_results.sh @@ -559,6 +559,24 @@ function bashunit::console_results::print_error_test() { bashunit::state::print_line "error" "$line" } +## +# Render stderr a parallel worker wrote outside any test body. +# A sequential run lets this straight through to the terminal; parallel workers +# have it captured per file so concurrent writes cannot shred the progress +# line. Attributed to the file, not a test: it is emitted where no test owns it +# (data providers, hook plumbing). Test-body stderr is merged into the captured +# stdout and still surfaces in that test's own failure block. +# Arguments: $1 - test file the worker ran, $2 - captured stderr file +## +function bashunit::console_results::print_worker_stderr() { + local test_file="$1" + local stderr_file="$2" + + printf "\n%sStderr from %s%s\n" \ + "$_BASHUNIT_COLOR_SKIPPED" "$test_file" "$_BASHUNIT_COLOR_DEFAULT" + sed 's/^/|/' "$stderr_file" +} + function bashunit::console_results::print_failing_tests_and_reset() { if [ -s "$FAILURES_OUTPUT_PATH" ]; then local total_failed diff --git a/src/env.sh b/src/env.sh index e9711351..3302c7ab 100644 --- a/src/env.sh +++ b/src/env.sh @@ -654,6 +654,11 @@ SKIPPED_OUTPUT_PATH="$_BASHUNIT_RUN_OUTPUT_DIR/skipped" INCOMPLETE_OUTPUT_PATH="$_BASHUNIT_RUN_OUTPUT_DIR/incomplete" RISKY_OUTPUT_PATH="$_BASHUNIT_RUN_OUTPUT_DIR/risky" PROFILE_OUTPUT_PATH="$_BASHUNIT_RUN_OUTPUT_DIR/profile" +# Prefix for one file-per-worker capture of stderr written inside a parallel +# worker but outside a test body. An ordinal is appended per spawned worker; +# these must not live under TEMP_DIR_PARALLEL_TEST_SUITE, whose every entry is +# walked by state::aggregate_parallel_results (#864). +WORKER_STDERR_OUTPUT_PREFIX="$_BASHUNIT_RUN_OUTPUT_DIR/worker-stderr" # Collects ":" for every failing test in a run so the # next --rerun-failed can replay just those. Shared across parallel subshells. RERUN_FAILED_OUTPUT_PATH="$_BASHUNIT_RUN_OUTPUT_DIR/rerun-failed" diff --git a/src/runner.sh b/src/runner.sh index 460564af..a01b5f48 100755 --- a/src/runner.sh +++ b/src/runner.sh @@ -396,6 +396,9 @@ function bashunit::runner::load_test_files() { files=("$@") local -a scripts_ids=() local scripts_ids_count=0 + local -a worker_stderr_paths=() + local -a worker_stderr_owners=() + local worker_stderr_count=0 # Randomize file execution order (deterministic for the resolved seed). if bashunit::env::is_random_order_enabled; then @@ -538,7 +541,15 @@ function bashunit::runner::load_test_files() { local _cached_fns="$functions_for_script" if bashunit::parallel::is_enabled; then bashunit::runner::wait_for_job_slot - bashunit::runner::call_test_functions "$test_file" "$_cached_fns" 2>/dev/null & + # Capture rather than discard: a worker's stderr cannot be written + # straight to the terminal without shredding the progress line, but + # dropping it made the same run report differently under --parallel + # (#358 added the discard, #864 replaced it with this capture). + local _worker_stderr="${WORKER_STDERR_OUTPUT_PREFIX}.${worker_stderr_count}" + worker_stderr_paths[worker_stderr_count]="$_worker_stderr" + worker_stderr_owners[worker_stderr_count]="$test_file" + worker_stderr_count=$((worker_stderr_count + 1)) + bashunit::runner::call_test_functions "$test_file" "$_cached_fns" 2>"$_worker_stderr" & else bashunit::runner::call_test_functions "$test_file" "$_cached_fns" fi @@ -561,6 +572,16 @@ function bashunit::runner::load_test_files() { disown "$spinner_pid" 2>/dev/null || true kill "$spinner_pid" 2>/dev/null || true printf "\r \r" # Clear the spinner output + + local _stderr_idx=0 + while [ "$_stderr_idx" -lt "$worker_stderr_count" ]; do + if [ -s "${worker_stderr_paths[_stderr_idx]:-}" ]; then + bashunit::console_results::print_worker_stderr \ + "${worker_stderr_owners[_stderr_idx]:-}" "${worker_stderr_paths[_stderr_idx]:-}" + fi + _stderr_idx=$((_stderr_idx + 1)) + done + local script_id for script_id in "${scripts_ids[@]+"${scripts_ids[@]}"}"; do export BASHUNIT_CURRENT_SCRIPT_ID="${script_id}" diff --git a/tests/acceptance/fixtures/test_worker_stderr.sh b/tests/acceptance/fixtures/test_worker_stderr.sh new file mode 100644 index 00000000..a9bf602e --- /dev/null +++ b/tests/acceptance/fixtures/test_worker_stderr.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash + +# A data provider runs inside the parallel file worker but outside any test +# body, so its stderr is the plain case that the worker-level redirect used to +# discard. Test-body stderr is a different path: it is merged into the captured +# stdout and already surfaces in the failure block. + +function data_provider_noisy() { + echo "WORKER-SCOPE-DIAGNOSTIC" >&2 + echo "1" +} + +# @data_provider data_provider_noisy +function test_uses_a_noisy_provider() { + assert_equals "1" "$1" +} diff --git a/tests/acceptance/worker_stderr_test.sh b/tests/acceptance/worker_stderr_test.sh new file mode 100644 index 00000000..1e26a8d4 --- /dev/null +++ b/tests/acceptance/worker_stderr_test.sh @@ -0,0 +1,42 @@ +#!/usr/bin/env bash + +# Parallel workers used to run with `2>/dev/null`, so anything written to +# stderr inside the worker but outside a test body vanished, and the same run +# reported differently depending on --parallel (#864). +# +# The fixture's provider is also executed by the test-counting pass in the main +# shell, so merely *finding* the diagnostic proves nothing — it leaks in from +# that pass either way. What the bug actually broke is the two modes agreeing, +# so that is what these compare. + +FIXTURE="tests/acceptance/fixtures/test_worker_stderr.sh" + +function _count_diagnostic_in() { + local parallel_flag="$1" + + ./bashunit "$parallel_flag" "$FIXTURE" 2>&1 | + grep -c "WORKER-SCOPE-DIAGNOSTIC" +} + +function test_parallel_reports_worker_stderr_as_often_as_sequential() { + local sequential parallel + sequential="$(_count_diagnostic_in --no-parallel)" + parallel="$(_count_diagnostic_in --parallel)" + + assert_not_equals "0" "$sequential" + assert_equals "$sequential" "$parallel" +} + +function test_parallel_run_attributes_worker_stderr_to_its_file() { + local output + output=$(./bashunit --parallel "$FIXTURE" 2>&1) + + assert_contains "Stderr from $FIXTURE" "$output" +} + +function test_parallel_run_stays_quiet_when_no_worker_writes_stderr() { + local output + output=$(./bashunit --parallel tests/acceptance/fixtures/test_coverage_engine.sh 2>&1) + + assert_not_contains "Stderr from" "$output" +}