From f871e61135dd618fe92138ea00029c9653e2ac1e Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Mon, 20 Jul 2026 01:28:48 +0200 Subject: [PATCH] fix(runner): report failing lifecycle hooks consistently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failing set_up_before_script produced three different outcomes depending on incidental details: attributed failure (plain failing command), silently ignored (failing `cmd && var=x` guard on Bash 3.2 — execute_file_hook read only the ERR-trap global and discarded the hook's real exit status), or silent failures with an off-by-one count (Bash >= 4, where the ERR trap re-fired in the executor's own scope and returned before record_file_hook_failure ran). - capture the hook's real status from $? and fall back to the trap global; guard the trap's return by FUNCNAME so the executor-scope re-fire records instead of silently bailing (both hook executors) - reset the runner loop to set +euo after sourcing each file: a test file's top-level `set -euo pipefail` leaked into the loop and let a failing hook abort the whole run mid-suite with no summary - count hook-failure casualties from the file's own function list and run the #829 function cleanup on the failure path too — previously leftover functions from earlier files inflated the totals - harden two src helpers that broke callers under strict mode: is_diff_enabled read an unset var, cleanup_testcase_temp_files returned 1 on its skip path - make 13 latently strict-broken tests capture nested failures with `|| code=$?` — the loop reset makes --strict honest and deterministic, which exposed them (they already failed standalone on main) - report_json: a jq-less box now skips instead of failing the hook Closes #836 Claude-Session: https://claude.ai/code/session_01JSeB8UzLCpqst55hAK6dED --- CHANGELOG.md | 2 + src/env.sh | 4 +- src/globals.sh | 6 +- src/runner.sh | 68 +++++++++++++++---- .../acceptance/bashunit_hook_failure_test.sh | 43 ++++++++++++ tests/acceptance/bashunit_report_json_test.sh | 11 +-- tests/acceptance/bashunit_retry_test.sh | 4 +- tests/acceptance/bashunit_run_forks_test.sh | 2 +- tests/acceptance/bashunit_timeout_test.sh | 4 +- .../fixtures/hook_failure/guard_hook.sh | 13 ++++ .../acceptance/fixtures/hook_failure/later.sh | 3 + .../fixtures/hook_failure/plain_hook.sh | 9 +++ tests/unit/assert_basic_test.sh | 12 ++-- tests/unit/globals_test.sh | 6 +- 14 files changed, 154 insertions(+), 33 deletions(-) create mode 100644 tests/acceptance/bashunit_hook_failure_test.sh create mode 100644 tests/acceptance/fixtures/hook_failure/guard_hook.sh create mode 100644 tests/acceptance/fixtures/hook_failure/later.sh create mode 100644 tests/acceptance/fixtures/hook_failure/plain_hook.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 11958d3d..4d844cf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ ## Unreleased ### Fixed +- A failing `set_up_before_script`/`set_up` is now reported consistently: previously the same failure could be attributed (plain failing command), silently ignored (failing `cmd && var=x` guard on Bash 3.2 — the hook's real exit status was discarded), or silently counted with an off-by-one total and no message (Bash ≥ 4, where the ERR trap re-fired in the runner's scope). Every test in the affected file is now marked failed with the hook message, totals match the declared count, and the suite continues — a strict test file can no longer abort the whole run mid-suite via its top-level `set -euo pipefail` (#836) +- `bashunit::env::is_diff_enabled` and `bashunit::cleanup_testcase_temp_files` no longer break callers under `set -u`/`set -e` (unbound read after `unset BASHUNIT_NO_DIFF`; skip path returned 1) (#836) - `--stop-on-failure`, `--log-junit`, `--report-html`, `--report-tap` and `--report-json` no longer leak into nested bashunit runs via the environment: a script under test that itself calls bashunit used to inherit the parent's stop-on-failure mode (aborting before the rerun cache was written) and overwrite the parent's report files (#834) - `./bashunit bench` works again when running from a repository checkout: the dev entrypoint never sourced `src/benchmark.sh`, so every bench run crashed with `command not found` (the built binary was unaffected) (#834) - `./build.sh --verify` now exits non-zero when the built binary fails the test suite — previously a red verification run still reported success to CI. The gate exposed that verification had been silently crashing mid-suite since 2025-06: six test files resolved repo paths through the running binary's root dir, which has no `src/` in a build folder; they now resolve paths relative to the test file or repo cwd (#834) diff --git a/src/env.sh b/src/env.sh index 4980bf9e..387a8665 100644 --- a/src/env.sh +++ b/src/env.sh @@ -351,7 +351,9 @@ function bashunit::env::is_no_color_enabled() { } function bashunit::env::is_diff_enabled() { - [ "$BASHUNIT_NO_DIFF" != "true" ] + # :- guard: a user (or test) may have unset BASHUNIT_NO_DIFF; a bare read + # dies under set -u (--strict) (#836) + [ "${BASHUNIT_NO_DIFF:-}" != "true" ] } ## diff --git a/src/globals.sh b/src/globals.sh index cadee505..ccaa78c1 100644 --- a/src/globals.sh +++ b/src/globals.sh @@ -77,7 +77,11 @@ function bashunit::cleanup_testcase_temp_files() { # assignment attached to `local`, it stores the literal "(glob)" instead. local matches matches=("$BASHUNIT_TEMP_DIR/${BASHUNIT_CURRENT_TEST_ID}"_*) - [ -e "${matches[0]:-}" ] && rm -rf "${matches[@]}" + # if-form, not `[ ] && rm`: as the function's last statement the skip path + # would return 1, which is a death sentence for callers under set -e (#836) + if [ -e "${matches[0]:-}" ]; then + rm -rf "${matches[@]}" + fi fi } diff --git a/src/runner.sh b/src/runner.sh index bb1b08da..b07ff49d 100755 --- a/src/runner.sh +++ b/src/runner.sh @@ -400,6 +400,12 @@ function bashunit::runner::load_test_files() { # shellcheck source=/dev/null source "$test_file" 2>"$source_err_file" source_status=$? + # A test file may enable `set -euo pipefail` at its top level; sourcing + # runs that in THIS shell, so a later non-zero status in the loop (e.g. a + # failing set_up_before_script) would kill the whole run mid-suite with no + # summary. Strictness is applied per-test in execute_test_body — reset the + # runner loop to its set +euo invariant (see main.sh exec_tests) (#836). + set +euo pipefail source_err="" if [ -s "$source_err_file" ]; then source_err="$(cat "$source_err_file")" @@ -462,21 +468,25 @@ function bashunit::runner::load_test_files() { bashunit::runner::run_set_up_before_script "$test_file" local setup_before_script_status=$? if [ $setup_before_script_status -ne 0 ]; then - # Count the test functions that couldn't run due to set_up_before_script failure - # and add them as failed (minus 1 since the hook failure already counts as 1) - local filtered_functions - filtered_functions=$(bashunit::helper::get_functions_to_run "test" "$filter" "$_BASHUNIT_CACHED_ALL_FUNCTIONS") - if [ -n "$filtered_functions" ]; then + # Count the test functions that couldn't run due to set_up_before_script + # failure and add them as failed (minus 1 since the hook failure already + # counts as 1). Use this file's own function list — scanning the cached + # ALL-functions set would also count fns left over from earlier files + # and inflate the totals (#836). + if [ -n "$functions_for_script" ]; then # Bash 3.0 compatible: separate declaration and assignment for arrays local functions_to_run # shellcheck disable=SC2206 - functions_to_run=($filtered_functions) + functions_to_run=($functions_for_script) local additional_failures=$((${#functions_to_run[@]} - 1)) local i for ((i = 0; i < additional_failures; i++)); do bashunit::state::add_tests_failed done fi + # Same cleanup as the success path: without it the file's test functions + # leak into the next iteration's counts and the main shell (#829, #836). + 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 bashunit::cleanup_script_temp_files @@ -537,6 +547,9 @@ function bashunit::runner::load_bench_files() { export BASHUNIT_CURRENT_SCRIPT_ID="$_BASHUNIT_HELPER_ID_OUT" # shellcheck source=/dev/null source "$bench_file" + # Reset the loop's shell-mode invariant; a bench file may set -euo at top + # level and sourcing runs that in this shell (see the test loop) (#836). + set +euo pipefail # Update function cache after sourcing new bench file (compgen is a builtin) _BASHUNIT_CACHED_ALL_FUNCTIONS=$(compgen -A function) # Call hook directly (not with `if !`) to preserve errexit behavior inside the hook @@ -1847,14 +1860,32 @@ function bashunit::runner::execute_file_hook() { if bashunit::env::is_strict_mode_enabled; then set -uo pipefail fi - trap '_BASHUNIT_HOOK_ERR_STATUS=$?; set +Eu +o pipefail; trap - ERR; return $_BASHUNIT_HOOK_ERR_STATUS' ERR + # The trap returns from the function where the failure occurred (early-exit + # semantics for intermediate failing commands) — but only when that frame is + # NOT this executor: on Bash >= 4 the trap also fires HERE when the hook call + # itself returns non-zero, and an unconditional return skipped + # record_file_hook_failure entirely (silent failures, off-by-one counts, #836). + # shellcheck disable=SC2154 + trap '_BASHUNIT_HOOK_ERR_STATUS=$? + if [ "${FUNCNAME[0]:-}" != "bashunit::runner::execute_file_hook" ]; then + set +Eu +o pipefail + trap - ERR + return $_BASHUNIT_HOOK_ERR_STATUS + fi' ERR { "$hook_name" } >"$hook_output_file" 2>&1 + # Real exit status of the hook, read from $? (this function runs without -e, + # so a failing compound does not exit). The ERR-trap global alone is not + # enough: a hook ending in a failing `cmd && var=x` guard returns non-zero + # without ever firing the trap (&& lists are ERR-exempt), which silently + # swallowed the failure (#836). + status=$? + if [ "$status" -eq 0 ]; then + status=$_BASHUNIT_HOOK_ERR_STATUS + fi - # Capture exit status from global variable and clean up - status=$_BASHUNIT_HOOK_ERR_STATUS trap - ERR set +Eu +o pipefail @@ -1952,14 +1983,27 @@ function bashunit::runner::execute_test_hook() { if bashunit::env::is_strict_mode_enabled; then set -uo pipefail fi - trap '_BASHUNIT_HOOK_ERR_STATUS=$?; set +Eu +o pipefail; trap - ERR; return $_BASHUNIT_HOOK_ERR_STATUS' ERR + # See the twin comment in execute_file_hook: conditional return keeps the + # early-exit semantics for intermediate failures without silently returning + # from THIS executor when the trap re-fires here on Bash >= 4 (#836). + # shellcheck disable=SC2154 + trap '_BASHUNIT_HOOK_ERR_STATUS=$? + if [ "${FUNCNAME[0]:-}" != "bashunit::runner::execute_test_hook" ]; then + set +Eu +o pipefail + trap - ERR + return $_BASHUNIT_HOOK_ERR_STATUS + fi' ERR { "$hook_name" } >"$hook_output_file" 2>&1 + # Real hook status from $?; the trap global alone misses failing + # `cmd && var=x` guards (&& lists are ERR-exempt) (#836). + status=$? + if [ "$status" -eq 0 ]; then + status=$_BASHUNIT_HOOK_ERR_STATUS + fi - # Capture exit status from global variable and clean up - status=$_BASHUNIT_HOOK_ERR_STATUS trap - ERR set +Eu +o pipefail diff --git a/tests/acceptance/bashunit_hook_failure_test.sh b/tests/acceptance/bashunit_hook_failure_test.sh new file mode 100644 index 00000000..167625a3 --- /dev/null +++ b/tests/acceptance/bashunit_hook_failure_test.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Regression guards for #836. A failing set_up_before_script used to produce +# three different outcomes depending on the shape of the hook's last statement +# and the bash version: visible failure (ERR-trap path), silently ignored +# (&&-guard on bash 3.2, real hook status was discarded), or silent failures +# with an off-by-one count (bash >= 4, where the ERR trap fired a second time +# in execute_file_hook's own scope and returned before the failure was +# recorded). One defined behavior now: every test in the file is marked failed +# with an attributed message, counts stay consistent, and the suite continues. + +FIXTURES="tests/acceptance/fixtures/hook_failure" + +function test_hook_failure_is_attributed_and_suite_continues() { + local output + local exit_code=0 + # --detailed: a parent running --simple still exports it (#837), and simple + # mode would drop the attributed progress lines this test greps for. + output=$(./bashunit --no-parallel --detailed --skip-env-file \ + "$FIXTURES/plain_hook.sh" "$FIXTURES/guard_hook.sh" "$FIXTURES/later.sh" 2>&1) || exit_code=$? + + assert_general_error "" "" "$exit_code" + # One attributed error line per failing file (the message repeats in the + # deferred failure blocks, so count the normalized progress lines). + assert_equals "2" "$(printf '%s\n' "$output" | grep -c "Set up before script")" + assert_contains "Later file still runs" "$output" + assert_contains "1 passed" "$output" + assert_contains "4 failed" "$output" + assert_contains "5 total" "$output" +} + +function test_hook_failure_counts_match_in_parallel() { + local output + local exit_code=0 + output=$(./bashunit --parallel --detailed --skip-env-file \ + "$FIXTURES/plain_hook.sh" "$FIXTURES/guard_hook.sh" "$FIXTURES/later.sh" 2>&1) || exit_code=$? + + assert_general_error "" "" "$exit_code" + assert_contains "1 passed" "$output" + assert_contains "4 failed" "$output" + assert_contains "5 total" "$output" +} diff --git a/tests/acceptance/bashunit_report_json_test.sh b/tests/acceptance/bashunit_report_json_test.sh index a7fda219..c8a62dae 100644 --- a/tests/acceptance/bashunit_report_json_test.sh +++ b/tests/acceptance/bashunit_report_json_test.sh @@ -4,14 +4,15 @@ function set_up_before_script() { TEST_ENV_FILE="tests/acceptance/fixtures/.env.default" FIXTURE="tests/acceptance/fixtures/test_bashunit_report_json.sh" JQ_AVAILABLE=false - command -v jq >/dev/null 2>&1 && JQ_AVAILABLE=true + # `|| true` so a jq-less box means "skip", not "hook failed" (#836) + { command -v jq >/dev/null 2>&1 && JQ_AVAILABLE=true; } || true } function test_report_json_writes_valid_json_with_correct_counts() { if [ "$JQ_AVAILABLE" = false ]; then bashunit::skip "jq required"; return; fi local report report="$(mktemp)" - ./bashunit --no-parallel --env "$TEST_ENV_FILE" --report-json "$report" "$FIXTURE" >/dev/null 2>&1 + ./bashunit --no-parallel --env "$TEST_ENV_FILE" --report-json "$report" "$FIXTURE" >/dev/null 2>&1 || true assert_successful_code "$(jq empty "$report" 2>&1)" assert_same "2" "$(jq '.summary.total' "$report")" @@ -24,7 +25,7 @@ function test_report_json_escapes_special_characters_in_messages() { if [ "$JQ_AVAILABLE" = false ]; then bashunit::skip "jq required"; return; fi local report report="$(mktemp)" - ./bashunit --no-parallel --env "$TEST_ENV_FILE" --report-json "$report" "$FIXTURE" >/dev/null 2>&1 + ./bashunit --no-parallel --env "$TEST_ENV_FILE" --report-json "$report" "$FIXTURE" >/dev/null 2>&1 || true # A double quote inside the failure message must round-trip as valid JSON. local message @@ -39,7 +40,7 @@ function test_report_json_is_valid_json_under_parallel() { if [ "$JQ_AVAILABLE" = false ]; then bashunit::skip "jq required"; return; fi local report report="$(mktemp)" - ./bashunit --parallel --env "$TEST_ENV_FILE" --report-json "$report" "$FIXTURE" >/dev/null 2>&1 + ./bashunit --parallel --env "$TEST_ENV_FILE" --report-json "$report" "$FIXTURE" >/dev/null 2>&1 || true assert_successful_code "$(jq empty "$report" 2>&1)" rm -f "$report" @@ -49,7 +50,7 @@ function test_report_json_is_not_written_without_the_flag() { local report report="$(mktemp)" rm -f "$report" - ./bashunit --no-parallel --env "$TEST_ENV_FILE" "$FIXTURE" >/dev/null 2>&1 + ./bashunit --no-parallel --env "$TEST_ENV_FILE" "$FIXTURE" >/dev/null 2>&1 || true assert_file_not_exists "$report" } diff --git a/tests/acceptance/bashunit_retry_test.sh b/tests/acceptance/bashunit_retry_test.sh index ba826f32..1bad866c 100644 --- a/tests/acceptance/bashunit_retry_test.sh +++ b/tests/acceptance/bashunit_retry_test.sh @@ -39,7 +39,7 @@ function test_bashunit_without_retry_a_flaky_test_fails() { export BASHUNIT_RETRY_FIXTURE_PASS_ON=2 local output output="$(./bashunit --no-parallel --env "$TEST_ENV_FILE" \ - --retry 0 --filter test_a_flaky "$FIXTURE")" + --retry 0 --filter test_a_flaky "$FIXTURE")" || true assert_contains "1 failed" "$output" assert_same "1" "$(cat "$COUNTER_FILE")" @@ -49,7 +49,7 @@ function test_bashunit_retry_gives_up_after_exhausting_attempts() { export BASHUNIT_RETRY_FIXTURE_PASS_ON=99 local output output="$(./bashunit --no-parallel --env "$TEST_ENV_FILE" \ - --retry 2 --filter test_a_flaky "$FIXTURE")" + --retry 2 --filter test_a_flaky "$FIXTURE")" || true # Counted once, not once per attempt. assert_contains "1 failed" "$output" diff --git a/tests/acceptance/bashunit_run_forks_test.sh b/tests/acceptance/bashunit_run_forks_test.sh index e07b81e8..2fd12eaa 100644 --- a/tests/acceptance/bashunit_run_forks_test.sh +++ b/tests/acceptance/bashunit_run_forks_test.sh @@ -78,7 +78,7 @@ function test_failure_source_context_does_not_fork_sed_per_line() { } >"$fixture" local trace - trace="$(PS4='+ ' bash -x ./bashunit --no-parallel "$fixture" 2>&1 >/dev/null)" + trace="$(PS4='+ ' bash -x ./bashunit --no-parallel "$fixture" 2>&1 >/dev/null)" || true local sed_forks sed_forks="$(printf '%s\n' "$trace" | grep -cE '^\++ +/?[a-z/]*sed ' || true)" diff --git a/tests/acceptance/bashunit_timeout_test.sh b/tests/acceptance/bashunit_timeout_test.sh index e58c964f..e9708209 100644 --- a/tests/acceptance/bashunit_timeout_test.sh +++ b/tests/acceptance/bashunit_timeout_test.sh @@ -8,14 +8,14 @@ function set_up_before_script() { function test_bashunit_terminates_a_hanging_test_with_timeout() { local output - output="$(./bashunit --no-parallel --env "$TEST_ENV_FILE" --test-timeout 1 "$FIXTURE")" + output="$(./bashunit --no-parallel --env "$TEST_ENV_FILE" --test-timeout 1 "$FIXTURE")" || true assert_contains "Test timed out after 1s" "$output" } function test_bashunit_keeps_running_tests_after_a_timed_out_one() { local output - output="$(./bashunit --no-parallel --env "$TEST_ENV_FILE" --test-timeout 1 "$FIXTURE")" + output="$(./bashunit --no-parallel --env "$TEST_ENV_FILE" --test-timeout 1 "$FIXTURE")" || true # The fast test still ran and passed and the run reached its summary instead # of hanging forever on the blocked test. diff --git a/tests/acceptance/fixtures/hook_failure/guard_hook.sh b/tests/acceptance/fixtures/hook_failure/guard_hook.sh new file mode 100644 index 00000000..7cc4ae80 --- /dev/null +++ b/tests/acceptance/fixtures/hook_failure/guard_hook.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash +# shellcheck disable=SC2034 # the guard variable is the point of the fixture +set -euo pipefail + +# The hook's last statement is a failing `cmd && assignment` guard, so the hook +# returns 1 without triggering the ERR trap (the failure is inside a && list). +function set_up_before_script() { + GUARD_FLAG=false + command -v definitely_missing_tool_bashunit >/dev/null 2>&1 && GUARD_FLAG=true +} + +function test_guard_one() { assert_true true; } +function test_guard_two() { assert_true true; } diff --git a/tests/acceptance/fixtures/hook_failure/later.sh b/tests/acceptance/fixtures/hook_failure/later.sh new file mode 100644 index 00000000..fbe9d308 --- /dev/null +++ b/tests/acceptance/fixtures/hook_failure/later.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env bash + +function test_later_file_still_runs() { assert_true true; } diff --git a/tests/acceptance/fixtures/hook_failure/plain_hook.sh b/tests/acceptance/fixtures/hook_failure/plain_hook.sh new file mode 100644 index 00000000..88ed9739 --- /dev/null +++ b/tests/acceptance/fixtures/hook_failure/plain_hook.sh @@ -0,0 +1,9 @@ +#!/usr/bin/env bash + +# The hook fails with a plain failing command (ERR-trap path). +function set_up_before_script() { + command -v definitely_missing_tool_bashunit >/dev/null 2>&1 +} + +function test_plain_one() { assert_true true; } +function test_plain_two() { assert_true true; } diff --git a/tests/unit/assert_basic_test.sh b/tests/unit/assert_basic_test.sh index 9c0aece8..01102c2e 100644 --- a/tests/unit/assert_basic_test.sh +++ b/tests/unit/assert_basic_test.sh @@ -59,8 +59,8 @@ function test_run_command_or_eval_runs_alias_non_zero() { # shellcheck disable=SC2139 alias bashunit_alias_ko='return 3' - bashunit::run_command_or_eval "bashunit_alias_ko" - local exit_code=$? + local exit_code=0 + bashunit::run_command_or_eval "bashunit_alias_ko" || exit_code=$? assert_same "3" "$exit_code" unalias bashunit_alias_ko @@ -77,8 +77,8 @@ function test_run_command_or_eval_runs_function_not_treated_as_alias() { function test_run_command_or_eval_name_value_is_not_defined_as_alias() { # Regression: "name=value" must NOT be probed with `alias` (it would define # the alias and wrongly succeed). It has to be run directly and fail. - bashunit::run_command_or_eval "bashunit_x=1" - local exit_code=$? + local exit_code=0 + bashunit::run_command_or_eval "bashunit_x=1" || exit_code=$? local side_effect="absent" if alias bashunit_x >/dev/null 2>&1; then @@ -91,8 +91,8 @@ function test_run_command_or_eval_name_value_is_not_defined_as_alias() { function test_run_command_or_eval_multiword_command_is_not_treated_as_alias() { # A multi-word string can never be an alias name: run it directly. - bashunit::run_command_or_eval "bashunit_missing_cmd --flag" - local exit_code=$? + local exit_code=0 + bashunit::run_command_or_eval "bashunit_missing_cmd --flag" || exit_code=$? assert_not_same "0" "$exit_code" } diff --git a/tests/unit/globals_test.sh b/tests/unit/globals_test.sh index 966c9e4d..fcb39646 100644 --- a/tests/unit/globals_test.sh +++ b/tests/unit/globals_test.sh @@ -93,7 +93,7 @@ function test_globals_cleanup_testcase_skips_and_preserves_others_when_none_crea local unrelated="$BASHUNIT_TEMP_DIR/unrelated_${$}.keep" : >"$unrelated" - bashunit::cleanup_testcase_temp_files + bashunit::cleanup_testcase_temp_files || true assert_file_exists "$unrelated" rm -f "$unrelated" @@ -107,8 +107,8 @@ function test_globals_cleanup_testcase_skip_path_is_safe_under_nullglob() { # With nullglob on the non-matching glob yields an empty array; the skip must # not trip set -u on ${matches[0]}. shopt -s nullglob - bashunit::cleanup_testcase_temp_files - local status=$? + local status=0 + bashunit::cleanup_testcase_temp_files || status=$? shopt -u nullglob assert_successful_code "$status"