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