fix(runner): report failing lifecycle hooks consistently

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)

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" ]
 }
 
 ##

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
 }
 

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
 

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"
+}

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"
 }

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"

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)"

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.

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; }

tests/acceptance/fixtures/hook_failure/later.sh

@@ -0,0 +1,3 @@
+#!/usr/bin/env bash
+
+function test_later_file_still_runs() { assert_true true; }

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; }

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"
 }

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"