From 8a5f8667f878b177b325fc3c626f359e96f27309 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 9 Sep 2026 09:27:46 +0200 Subject: [PATCH] perf(runner): name a hook's output file without forking mktemp or rm A test in a file defining set_up or tear_down cost five process forks more than a hookless one: each hook minted its output file through bashunit::temp_file (mktemp) and removed it with rm -f, and the temp-owner marker mktemp leaves behind made the EXIT trap rm -rf the test's temp files even when the test created none. That is 14.1ms per test against 5.4ms hookless here, and this suite runs 1052 such tests. Name the file arithmetically in the run directory instead, the way discovery.sh already names its source-error capture: the folded file path (keeps two files sharing a basename apart, #959) plus the per-suite ordinal the dispatcher assigns before forking (#851), since a Bash 3 subshell inherits both $$ and the RANDOM state. The > redirect truncates, so nothing has to remove it and the run-dir cleanup takes it at the end. Hooked and hookless tests now cost the same. Reusing the run dir also inherits its known hazard, so extract the vanished-directory guard discovery.sh grew for #1137 and share it: a failed redirect otherwise reads as the hook itself failing. The census in perf-fork-budget.md called the per-test path fork-free. It measured a fixture that defines no hooks, which is why this never showed up; the rule now says so. Closes #1345 Claude-Session: https://claude.ai/code/session_01EXYWTGLjf7qM8Ru3GakDRm --- .claude/rules/perf-fork-budget.md | 9 +++- CHANGELOG.md | 3 ++ src/config/env.sh | 33 +++++++++++++ src/runner/discovery.sh | 25 ++-------- src/runner/hooks.sh | 46 +++++++++++++++--- src/runner/result.sh | 21 +++++++- tests/acceptance/bashunit_run_forks_test.sh | 53 +++++++++++++++++++++ 7 files changed, 160 insertions(+), 30 deletions(-) diff --git a/.claude/rules/perf-fork-budget.md b/.claude/rules/perf-fork-budget.md index 237fbb85..f13d7f6e 100644 --- a/.claude/rules/perf-fork-budget.md +++ b/.claude/rules/perf-fork-budget.md @@ -186,7 +186,10 @@ them: - **Not forks.** A PATH-shim census over 30 binaries counts **12 forks for 500 tests** (3 perl, 2 rm, 2 awk, and one each of uname/tput/mkdir/bc/base64). - The per-test path is genuinely fork-free. + The per-test path is genuinely fork-free -- but read the fixture before + trusting a census: this one defines no `set_up` or `tear_down`, and a file + that does used to cost five more forks per test (#1345), which is why that + never showed up here. A census fixture only measures the path it exercises. - **Not the capture subshell.** A bare `$( )` costs ~0.46 ms here, about 6% of the 7.8 ms. The rest is bash work in the per-test machinery. - **Not quadratic.** Per-test cost is 7.17 ms at 100 tests and 8.06 ms at 1000 @@ -202,7 +205,9 @@ from them rather than re-deriving them. shell since #817, the header count reads a return slot so the cache survives into the runner — plus the duplicate check), `perl` ×2 clock reads (start/end; no `EPOCHREALTIME` before Bash 5), 1 `base64` capability probe, 1 `mkdir`, -1 `tput`. Per-test cost is fork-free. +1 `tput`. Per-test cost is fork-free, hooks included: `set_up`/`tear_down` +capture their output in a run-dir file named from the folded file path and the +per-suite ordinal, so no `mktemp` mints it and no `rm` removes it (#1345). **Cold start: 3 binary forks** — `uname` (OS detect), `tput` (snapshot width), `perl` (clock before Bash 5). It was 5 until #1124: `check_os::init` ran twice, diff --git a/CHANGELOG.md b/CHANGELOG.md index 13491def..c030cc7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +### Changed +- Performance: a test in a file that defines `set_up` or `tear_down` is about 2.6x faster, which brings it level with a hookless test (14.1ms to 5.4ms per test on macOS arm64, bash 3.2). Each hook minted its output file with `mktemp` and removed it with `rm`, and the ownership marker that left behind made the runner `rm -rf` the test's temp files at exit — five forks per test, even for a test that created no temp file of its own (#1345) + ### Fixed - Coverage counts every line of a multiline statement: array literals, quoted strings, heredocs, backslash continuations. An array written one element per line cost one uncovered line per element. Bash 3.x records the assignment on its closing `)`, so there the whole array read as uncovered. A parent statement's hits no longer mark commands inside command or process substitutions as covered, even within quotes or arrays (#1338) - `--exclude-filter` keeps a comma as part of the test function name. The value was split at the comma, so `--exclude-filter 'test_a,{b}'` also excluded `test_a`. Repeat the flag to exclude several names. `BASHUNIT_EXCLUDE_FILTER` stays comma-separated, so one of its filters cannot hold a literal comma (#1340) diff --git a/src/config/env.sh b/src/config/env.sh index d619b1f2..78d3d6a2 100644 --- a/src/config/env.sh +++ b/src/config/env.sh @@ -936,6 +936,39 @@ function bashunit::env::create_scratch_dirs() { bashunit::env::create_scratch_dirs "$_BASHUNIT_RUN_OUTPUT_DIR" "$BASHUNIT_TEMP_DIR" || exit 1 +## +# Restores the run scratch directory when it vanishes mid-run. +# +# A missing directory makes a capture redirect fail, and bash reports that as +# the *command* failing: exit 1 with nothing written to the capture file. That +# read as a test file which would not source (#1137), and as a hook that failed +# (#1345). Callers fall back to /dev/null when this returns 1 -- losing one +# capture is worth far less than failing something for a reason that is not its +# own. +# +# Says so when it happens, once per run: #1137 is open precisely because a +# scratch directory goes missing on CI and nobody can say what removed it. +# Surviving it silently would keep it that way. +# +# The common case is a `[ -d ]` builtin, so callers on the per-test path pay no +# fork (.claude/rules/perf-fork-budget.md). +# Returns: 0 when the directory is there, 1 when it could not be restored +## +function bashunit::env::ensure_run_output_dir() { + if [ -d "$_BASHUNIT_RUN_OUTPUT_DIR" ]; then + return 0 + fi + + if [ "${_BASHUNIT_RUN_DIR_VANISHED:-false}" = false ]; then + _BASHUNIT_RUN_DIR_VANISHED=true + printf 'bashunit: the run scratch directory disappeared mid-run: %s\n' \ + "$_BASHUNIT_RUN_OUTPUT_DIR" >&2 + printf 'bashunit: recreating it; please report this with the run log (#1137).\n' >&2 + fi + + mkdir -p "$_BASHUNIT_RUN_OUTPUT_DIR" 2>/dev/null +} + # Removes this run's scratch directory (guarded like parallel::cleanup so a # broken variable can never turn the rm loose elsewhere). Called at the end of # a run and on SIGINT; without it every invocation leaks one directory. diff --git a/src/runner/discovery.sh b/src/runner/discovery.sh index 6e0ce025..e5fb8eac 100644 --- a/src/runner/discovery.sh +++ b/src/runner/discovery.sh @@ -71,26 +71,11 @@ function bashunit::runner::load_test_files() { # run-dir cleanup removes it, saving a mktemp and an rm fork per file. local source_err_file source_err source_status source_err_file="$_BASHUNIT_RUN_OUTPUT_DIR/source_err" - # A missing scratch dir makes the redirect below fail, and bash reports that - # as the *command* failing: exit 1 with nothing written to the capture file, - # which read as "this test file failed to source" against a file that was - # complete and valid (#1137). Restore the directory, and fall back to - # /dev/null if even that is refused -- losing a file's stderr capture is - # worth far less than failing the file for a reason that is not its own. - # - # Say so when it happens, once per run: #1137 is open precisely because a - # scratch directory goes missing on CI and nobody can say what removed it. - # Surviving it silently would keep it that way. - if [ ! -d "$_BASHUNIT_RUN_OUTPUT_DIR" ]; then - if [ "${_BASHUNIT_RUN_DIR_VANISHED:-false}" = false ]; then - _BASHUNIT_RUN_DIR_VANISHED=true - printf 'bashunit: the run scratch directory disappeared mid-run: %s\n' \ - "$_BASHUNIT_RUN_OUTPUT_DIR" >&2 - printf 'bashunit: recreating it; please report this with the run log (#1137).\n' >&2 - fi - if ! mkdir -p "$_BASHUNIT_RUN_OUTPUT_DIR" 2>/dev/null; then - source_err_file=/dev/null - fi + # A missing scratch dir makes the redirect below fail, which read as "this + # test file failed to source" against a file that was complete and valid + # (#1137). Restore it, and capture nothing if even that is refused. + if ! bashunit::env::ensure_run_output_dir; then + source_err_file=/dev/null fi # shellcheck source=/dev/null source "$test_file" 2>"$source_err_file" diff --git a/src/runner/hooks.sh b/src/runner/hooks.sh index 04eeed93..88ce1f34 100644 --- a/src/runner/hooks.sh +++ b/src/runner/hooks.sh @@ -80,6 +80,39 @@ function bashunit::runner::record_file_hook_failure() { return "$status" } +_BASHUNIT_RUNNER_HOOK_FILE="" + +## +# Names the file a hook's output is captured in, in _BASHUNIT_RUNNER_HOOK_FILE. +# +# A fixed path in the run dir, not a `temp_file`: minting one forked `mktemp`, +# removing it forked `rm -f`, and the temp-owner marker `mktemp` leaves behind +# made the EXIT trap `rm -rf` the test's temp files as well, even when the test +# created none. Five forks per test in any file defining `set_up` or +# `tear_down`, which cost 3.3x a hookless test (28.3ms vs 8.5ms) (#1345). The +# `>` redirect truncates, so the name may be reused and nothing has to remove +# it; the run-dir cleanup takes it with the rest at the end. +# +# The key has to survive concurrency, and a Bash 3 subshell inherits both `$$` +# and the `RANDOM` state from its parent. So it is built the way result.sh +# names `.result` files (#851): the folded file path, which keeps two files +# sharing a basename apart (#959), plus the per-suite ordinal the dispatcher +# assigns before forking, which keeps a file's parallel test workers apart. +# The hook name closes the last gap, between a file hook and a test hook that +# share an ordinal. +# +# Arguments: $1 - the hook name +## +function bashunit::runner::hook_output_path() { + if ! bashunit::env::ensure_run_output_dir; then + _BASHUNIT_RUNNER_HOOK_FILE=/dev/null + return 0 + fi + + local slot="${_BASHUNIT_RUNNER_FILE_SLOT}_${_BASHUNIT_RUNNER_RESULT_ORDINAL:-0}_$1" + _BASHUNIT_RUNNER_HOOK_FILE="${_BASHUNIT_RUN_OUTPUT_DIR}/hook_${slot}" +} + function bashunit::runner::execute_file_hook() { local hook_name="$1" local test_file="$2" @@ -90,7 +123,9 @@ function bashunit::runner::execute_file_hook() { local hook_output="" local status=0 local hook_output_file - hook_output_file=$(bashunit::temp_file "${hook_name}_output") + bashunit::runner::file_path_to_slot "$test_file" + bashunit::runner::hook_output_path "$hook_name" + hook_output_file=$_BASHUNIT_RUNNER_HOOK_FILE # Enable errtrace to catch any failing command in the hook. # Using -E (errtrace) without -e (errexit) prevents the main process from @@ -138,7 +173,6 @@ function bashunit::runner::execute_file_hook() { while IFS= read -r line; do [ -z "$hook_output" ] && hook_output="$line" || hook_output="$hook_output"$'\n'"$line" done <"$hook_output_file" - rm -f "$hook_output_file" fi if [ $status -ne 0 ]; then @@ -154,8 +188,8 @@ function bashunit::runner::execute_file_hook() { } function bashunit::runner::run_set_up() { - local _test_file="${1-}" bashunit::internal_log "run_set_up" + bashunit::runner::file_path_to_slot "${1-}" bashunit::runner::execute_test_hook 'set_up' } @@ -199,8 +233,8 @@ function bashunit::runner::run_set_up_before_script() { } function bashunit::runner::run_tear_down() { - local _test_file="${1-}" bashunit::internal_log "run_tear_down" + bashunit::runner::file_path_to_slot "${1-}" bashunit::runner::execute_test_hook 'tear_down' } @@ -212,7 +246,8 @@ function bashunit::runner::execute_test_hook() { local hook_output="" local status=0 local hook_output_file - hook_output_file=$(bashunit::temp_file "${hook_name}_output") + bashunit::runner::hook_output_path "$hook_name" + hook_output_file=$_BASHUNIT_RUNNER_HOOK_FILE # Enable errtrace to catch any failing command in the hook. # Using -E (errtrace) without -e (errexit) prevents the subshell from @@ -255,7 +290,6 @@ function bashunit::runner::execute_test_hook() { while IFS= read -r line; do [ -z "$hook_output" ] && hook_output="$line" || hook_output="$hook_output"$'\n'"$line" done <"$hook_output_file" - rm -f "$hook_output_file" fi if [ $status -ne 0 ]; then diff --git a/src/runner/result.sh b/src/runner/result.sh index 654a6dd9..f3ca5004 100644 --- a/src/runner/result.sh +++ b/src/runner/result.sh @@ -34,10 +34,27 @@ _BASHUNIT_RUNNER_SUITE_DIR_OUT="" # Arguments: $1 - the test file path ## function bashunit::runner::parallel_suite_dir_to_slot() { + bashunit::runner::file_path_to_slot "$1" + _BASHUNIT_RUNNER_SUITE_DIR_OUT="${TEMP_DIR_PARALLEL_TEST_SUITE}/${_BASHUNIT_RUNNER_FILE_SLOT}" +} + +_BASHUNIT_RUNNER_FILE_SLOT="" + +## +# Folds a test file path into one filename-safe key in +# _BASHUNIT_RUNNER_FILE_SLOT. +# +# Anything naming a per-file scratch path shares this, so that the reasoning +# above about keying on the whole path holds everywhere at once. +# +# Pure parameter expansion: callers sit on the per-test path, which must stay +# fork-free (.claude/rules/perf-fork-budget.md). +# Arguments: $1 - the test file path +## +function bashunit::runner::file_path_to_slot() { local key="${1#./}" key="${key%.sh}" - key="${key//\//_}" - _BASHUNIT_RUNNER_SUITE_DIR_OUT="${TEMP_DIR_PARALLEL_TEST_SUITE}/${key}" + _BASHUNIT_RUNNER_FILE_SLOT="${key//\//_}" } function bashunit::runner::parse_result_parallel() { diff --git a/tests/acceptance/bashunit_run_forks_test.sh b/tests/acceptance/bashunit_run_forks_test.sh index d21a2884..fd23913d 100644 --- a/tests/acceptance/bashunit_run_forks_test.sh +++ b/tests/acceptance/bashunit_run_forks_test.sh @@ -282,3 +282,56 @@ function test_reports_do_not_fork_base64_per_field() { # that, with room for the run's own bookkeeping; it was 56. assert_less_or_equal_than 16 "$calls" } + +# Regression guard for the per-test hook path. A test in a file that defines +# `set_up` or `tear_down` used to cost five process forks: each hook minted its +# output file with `mktemp` and removed it with `rm -f`, and the temp-owner +# marker `mktemp` left behind made the EXIT trap `rm -rf` the test's temp files +# even when the test itself created none. That is 3.3x the cost of a hookless +# test (28.3ms vs 8.5ms). The hook output file is named arithmetically inside +# the run directory now, and the `>` redirect truncates it, so a hooked test +# forks neither binary — only the run's own single `rm` of its scratch dir +# remains. +function test_test_hooks_do_not_fork_mktemp_or_rm_per_test() { + if bashunit::check_os::is_windows; then + bashunit::skip "PATH shims are unreliable under Git Bash" && return + fi + + local dir + dir="$(bashunit::temp_dir)" + local count_file="$dir/count" + local bin + for bin in mktemp rm; do + local real_bin + real_bin="$(command -v "$bin")" + { + echo '#!/usr/bin/env bash' + echo "echo $bin >> \"$count_file\"" + echo "exec \"$real_bin\" \"\$@\"" + } >"$dir/$bin" + chmod +x "$dir/$bin" + done + + local fixture="$dir/hook_forks_test.sh" + { + echo 'function set_up() { :; }' + echo 'function tear_down() { :; }' + echo 'function test_a() { assert_true true; }' + echo 'function test_b() { assert_true true; }' + echo 'function test_c() { assert_true true; }' + echo 'function test_d() { assert_true true; }' + } >"$fixture" + + PATH="$dir:$PATH" ./bashunit --no-parallel "$fixture" >/dev/null 2>&1 + + local mktemp_forks=0 + local rm_forks=0 + if [ -f "$count_file" ]; then + mktemp_forks="$(grep -c '^mktemp$' "$count_file" || true)" + rm_forks="$(grep -c '^rm$' "$count_file" || true)" + fi + + assert_equals 0 "$mktemp_forks" + # The run's own scratch-dir cleanup, and nothing per test. + assert_less_or_equal_than 1 "$rm_forks" +}