From d2b8a2725d1d4e738ce8bc9974b9bb89b55c7a21 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 19 Aug 2026 20:00:27 +0200 Subject: [PATCH] fix(runner): run file teardown after the file's tests in parallel Under --parallel the runner dispatched the file's tests as a background worker and ran tear_down_after_script in the parent on the next line, so the hook released the fixture its own tests were still reading. The same file passed sequentially and failed in parallel. Move the hook into the worker, after call_test_functions, which already waits for its per-test workers before returning. A hook failure recorded in a worker dies with the subshell, so publish it as a .result payload for the aggregator to count, the way a test publishes its own. Waiting on the worker from the parent was the other option. Measured on this suite it cost 12s of 52s: two slow acceptance files that define the hook stopped overlapping with anything. Closes #1320 --- CHANGELOG.md | 1 + docs/ai-agents.md | 2 - src/runner/discovery.sh | 15 ++++++- src/runner/result.sh | 42 +++++++++++++++++++ .../bashunit_parallel_consistency_test.sh | 36 ++++++++++++++++ ...shunit_teardown_after_script_error_test.sh | 27 ++++++++++++ 6 files changed, 119 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b11b9af3..574762cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed - `tear_down_after_script` runs when `set_up_before_script` fails, so it can release file-scoped resources acquired before the failure (#1318) +- Under `--parallel`, `tear_down_after_script` runs after the file's own tests instead of alongside them, so a fixture it releases stays alive for the tests that read it. The same file no longer passed sequentially and failed in parallel (#1320) ## [0.50.0](https://github.com/TypedDevs/bashunit/compare/0.49.0...0.50.0) - 2026-08-18 diff --git a/docs/ai-agents.md b/docs/ai-agents.md index ea9c9fa7..e339a93a 100644 --- a/docs/ai-agents.md +++ b/docs/ai-agents.md @@ -141,8 +141,6 @@ actually make against this API: tell `cmd "a b"` from `cmd a b` — a quoting bug passes it. Use `assert_have_been_called_with_args ...` (no `call_index`) to compare argument by argument whenever an argument may contain a space. -- Do not delete a shared fixture in `tear_down_after_script`: under `--parallel` the - file's tests may still be running, and they will vanish from the totals silently. - Re-record snapshots with `--snapshot-update`, scoped by `--filter`; do not delete snapshot files. A missing snapshot is written silently and never fails, so a wrong `rm` turns a real assertion into a rubber stamp. Read `git diff` afterwards either way. diff --git a/src/runner/discovery.sh b/src/runner/discovery.sh index 1b232702..9d980ab2 100644 --- a/src/runner/discovery.sh +++ b/src/runner/discovery.sh @@ -228,11 +228,22 @@ function bashunit::runner::load_test_files() { 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" & + # The file's teardown belongs inside the worker. Run from this shell it + # released the fixture while the worker's tests were still reading it, so + # the same file passed sequentially and failed under --parallel (#1320). + # call_test_functions waits for its own per-test workers before it + # returns, which is what makes this ordering hold. + { + bashunit::runner::call_test_functions "$test_file" "$_cached_fns" + bashunit::runner::run_tear_down_after_script "$test_file" + # A hook failure recorded in here dies with the subshell (#1147), so + # publish it the way a test publishes its result. + bashunit::runner::publish_file_hook_failure "$?" "$test_file" + } 2>"$_worker_stderr" & else bashunit::runner::call_test_functions "$test_file" "$_cached_fns" + bashunit::runner::run_tear_down_after_script "$test_file" fi - bashunit::runner::run_tear_down_after_script "$test_file" bashunit::runner::clean_script_test_functions "$_script_fns_to_clean" bashunit::runner::clean_set_up_and_tear_down_after_script if ! bashunit::parallel::is_enabled; then diff --git a/src/runner/result.sh b/src/runner/result.sh index 2474adf2..900c7d40 100644 --- a/src/runner/result.sh +++ b/src/runner/result.sh @@ -62,6 +62,48 @@ function bashunit::runner::parse_result_parallel() { echo "$execution_result" >"$unique_test_result_file" } +## +# Publishes a file-scoped hook failure from a --parallel worker, so the parent's +# summary counts it. +# +# tear_down_after_script runs inside the file's worker (#1320) and +# record_file_hook_failure's counter lives in that subshell, so without this the +# hook failed, said so on the terminal, and left the run green. The aggregator +# reads one `.result` per test out of the file's suite dir, so the hook publishes +# its own. A fixed basename: the per-test ordinals are scoped inside +# call_test_functions, and the aggregator globs `*.result`. +# +# Written only on failure. A zero-assertion, exit-0 payload is what the +# aggregator reads as a risky test, which would add a phantom risky test to every +# file that defines the hook. +# +# Arguments: $1 - the hook's exit status, $2 - the test file +## +function bashunit::runner::publish_file_hook_failure() { + local status=$1 + local test_file=$2 + + [ "$status" -ne 0 ] || return 0 + bashunit::parallel::is_enabled || return 0 + + bashunit::runner::parallel_suite_dir_to_slot "$test_file" + local test_suite_dir=$_BASHUNIT_RUNNER_SUITE_DIR_OUT + [ -d "$test_suite_dir" ] || mkdir -p "$test_suite_dir" + + # Hand-built rather than exported: export_subshell_context would carry the + # assertion totals this file's own tests already published, and the aggregator + # would count every one of them twice. The hook contributes a failed test, no + # assertions. + local payload="\ +##ASSERTIONS_FAILED=0\ +##ASSERTIONS_PASSED=0\ +##ASSERTIONS_SKIPPED=0\ +##ASSERTIONS_INCOMPLETE=0\ +##ASSERTIONS_SNAPSHOT=0\ +##TEST_EXIT_CODE=$status##" + printf '%s\n' "$payload" >"$test_suite_dir/tear_down_after_script.result" +} + function bashunit::runner::parse_result_sync() { local fn_name=$1 local execution_result=$2 diff --git a/tests/acceptance/bashunit_parallel_consistency_test.sh b/tests/acceptance/bashunit_parallel_consistency_test.sh index eb4e7f4e..9a03b3ac 100644 --- a/tests/acceptance/bashunit_parallel_consistency_test.sh +++ b/tests/acceptance/bashunit_parallel_consistency_test.sh @@ -104,3 +104,39 @@ Assertions: 5 passed, 5 total" assert_same "3 passed 3 total 5 passed 5 total " "$(summary_counts "$capture")" } + +# A file-scoped teardown has to run after the file's tests, not alongside them. +# Under --parallel the runner dispatched the worker and ran the hook on the very +# next line, so a resource acquired in set_up_before_script vanished mid-test and +# the file failed in parallel while passing sequentially (#1320). +function test_parallel_runs_tear_down_after_script_after_the_files_tests() { + local dir fixture + dir="$(bashunit::temp_dir parallel_file_teardown)" + fixture="$dir/shared_fixture_test.sh" + { + printf 'function set_up_before_script() {\n' + printf ' : >"$SHARED_FIXTURE"\n' + printf '}\n' + printf 'function tear_down_after_script() {\n' + printf ' rm -f "$SHARED_FIXTURE"\n' + printf '}\n' + printf 'function test_reads_the_shared_fixture() {\n' + printf ' sleep 0.5\n' + printf ' assert_file_exists "$SHARED_FIXTURE"\n' + printf '}\n' + } >"$fixture" + + local sequential_output parallel_output + sequential_output=$(SHARED_FIXTURE="$dir/sequential.fixture" NO_COLOR=1 \ + ./bashunit --no-parallel --env "$TEST_ENV_FILE" "$fixture" 2>&1) || true + parallel_output=$(SHARED_FIXTURE="$dir/parallel.fixture" NO_COLOR=1 \ + ./bashunit --parallel --env "$TEST_ENV_FILE" "$fixture" 2>&1) || true + + local sequential_counts parallel_counts + sequential_counts=$(summary_counts "$sequential_output") + parallel_counts=$(summary_counts "$parallel_output") + + assert_same "$sequential_counts" "$parallel_counts" + # Guard against both sides failing the same way and matching vacuously. + assert_contains "1 passed 1 total" "$sequential_counts" +} diff --git a/tests/acceptance/bashunit_teardown_after_script_error_test.sh b/tests/acceptance/bashunit_teardown_after_script_error_test.sh index 30dfb2e6..15a51dc2 100644 --- a/tests/acceptance/bashunit_teardown_after_script_error_test.sh +++ b/tests/acceptance/bashunit_teardown_after_script_error_test.sh @@ -84,3 +84,30 @@ test_bashunit_when_teardown_after_script_with_intermediate_failing_command.sh assert_contains "$assertions_summary" "$actual" assert_general_error "$(./bashunit --no-parallel --env "$TEST_ENV_FILE" "$test_file")" } + +# Under --parallel the hook runs inside the file's worker (#1320), where its own +# counter dies with the subshell the way #1147 describes. The count reaches the +# parent through publish_file_hook_failure. Drop that and the hook error still +# prints while the run reports "All tests passed", so pin the count here. +function test_bashunit_when_tear_down_after_script_errors_in_parallel() { + local test_file=./tests/acceptance/fixtures/test_bashunit_when_teardown_after_script_errors.sh + + local error_line="✗ Error: Tear down after script" + local message_line="missing_cleanup_command" + local tests_summary="Tests: 1 passed, 1 failed, 2 total" + local assertions_summary="Assertions: 1 passed, 0 failed, 1 total" + + local actual_raw + set +e + actual_raw="$(./bashunit --parallel --detailed --env "$TEST_ENV_FILE" "$test_file")" + set -e + + local actual + actual="$(printf "%s" "$actual_raw" | strip_ansi)" + + assert_contains "$error_line" "$actual" + assert_contains "$message_line" "$actual" + assert_contains "$tests_summary" "$actual" + assert_contains "$assertions_summary" "$actual" + assert_general_error "$(./bashunit --parallel --env "$TEST_ENV_FILE" "$test_file")" +}