diff --git a/.claude/rules/bash-style.md b/.claude/rules/bash-style.md index baccfc84..2976e4e3 100644 --- a/.claude/rules/bash-style.md +++ b/.claude/rules/bash-style.md @@ -49,6 +49,94 @@ Constants -> Globals -> Private functions -> Public functions Source deps relative to script: `"$(dirname "${BASH_SOURCE[0]}")/dep.sh"` +## Returning values from a helper (dynamic-scope safety) + +Bash `local` is **dynamically scoped**, not lexically scoped. A `local` inside a callee +shadows the caller's same-named variable for the duration of the call. Same trap fires +with `${!name}` reads and `eval "$name=..."`/`printf -v "$name"`/`export "$name"=...` +writes — they all resolve against the dynamic scope. + +We need **all three** properties on the hot path: + +1. **No subshell fork.** Returning via stdout and capturing with `$(...)` costs a fork + per call, which dominates per-test cost. +2. **No collision with caller locals.** A helper that silently overwrites or reads + the wrong variable is the worst kind of bug — no error, just stale data. +3. **Bash 3.0+ portable.** Rules out `declare -n` (4.3+) and `printf -v` (3.1+, and + it has the same dynamic-scope shadowing as `eval "$name=..."` anyway). + +### Preferred: dedicated global return slot + +```bash +_BASHUNIT_PKG_THING_OUT="" + +# Writes the result into _BASHUNIT_PKG_THING_OUT. +# Arguments: $1 input +function bashunit::pkg::do_thing() { + local input=$1 # natural name, no prefix needed + local val="${input%%]*}" + val="${val#[}" + _BASHUNIT_PKG_THING_OUT=$val +} + +# Caller +bashunit::pkg::do_thing "$payload" +local thing=$_BASHUNIT_PKG_THING_OUT +``` + +- Zero forks per call. +- Helper has natural local names; no `__bu_` noise. +- Caller cannot accidentally shadow the slot because the `_BASHUNIT_*` namespace + is reserved for the framework. +- A dedicated slot per helper (rather than one shared `_BASHUNIT_OUT`) means + adjacent or nested calls can't clobber each other. Cheap: globals are free. + +Examples in tree: `src/runner.sh` (`_BASHUNIT_RUNNER_FIELD_OUT`, +`_BASHUNIT_RUNNER_TOTAL_OUT`, `_BASHUNIT_RUNNER_TYPE_OUT`, `_BASHUNIT_RUNNER_OUTPUT_OUT`), +`src/coverage.sh` (`_BASHUNIT_BRANCH_ARMS_OUT`). + +### When the helper builds dynamic variable names (mock/spy state) + +Use a clear namespace prefix on the **constructed** name, not on the helper's locals: + +```bash +# Spy state lives in _BASHUNIT_SPY_${variable}_TIMES_FILE, not ${variable}_times_file. +# A caller doing `local foo_times_file=...` is therefore harmless: the helper +# resolves a different global. +export "_BASHUNIT_SPY_${variable}_TIMES_FILE"="$times_file" +``` + +Example in tree: `src/test_doubles.sh` (`_BASHUNIT_SPY_*`). + +### Last-resort fallback: outvar by name + `__bu_` prefix on locals + +Only use this when neither a fixed return slot nor a namespaced constructed name is +practical (e.g. a generic helper called from many call sites with different output +variables and no shared global is appropriate): + +```bash +function bashunit::pkg::do_thing() { + local __bu_out=$1 + local __bu_in=$2 + local __bu_val="${__bu_in%%]*}" + eval "$__bu_out=\$__bu_val" +} +``` + +- All internal locals MUST be `__bu_`-prefixed; otherwise dynamic-scope shadowing + silently breaks the caller's outvar (see PR #672 — that exact bug caused 12 parallel + test failures in #662). +- Include a regression test that calls the helper with one of its own documented + internal names as the outvar. + +### Intentional dynamic-scope mutation is a separate pattern + +The coverage branch helpers in `src/coverage.sh` (`_branch_push_if` and friends) deliberately +mutate caller locals (`if_decision_line`, `if_arms`, `if_depth`, `if_arm_start`). That is +documented inline at `src/coverage.sh:818-821` and is **not** the outvar pattern — the +helper has no `$1`-named outvar argument; the caller agrees to share state by convention. +Don't introduce new instances of this pattern without an inline justification comment. + ## ShellCheck All code must pass `make sa`. Use directives sparingly with reason: diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d7e5332..9588fac3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,15 @@ ## Unreleased +### Internal +- Codify global-slot return pattern for hot-path helpers; namespace mock/spy state under `_BASHUNIT_SPY_*` (#674) + ### Performance -- Faster runtime-error detection: single `case` glob instead of 23-iteration loop in `detect_runtime_error` (#668) -- Hot-path coverage flag now cached in `_BASHUNIT_COVERAGE_ON`, removing a function dispatch per call (#664) -- Parallel runner blocks on `wait -n` on Bash 4.3+ instead of polling `jobs -r`, removing sleep-induced slot-release latency (#667) -- Hot-path result helpers (`extract_encoded_field`, `extract_subshell_type`, `format_subshell_output`, `compute_total_assertions`) use outvar pattern, dropping a fork per call per test (#662) -- `generate_id` and `normalize_variable_name` drop `grep` and `random_str` forks via pure-bash globbing/inlining (#663) +- Faster runtime-error detection: single `case` glob (#668) +- Cache coverage-enabled flag in hot path (#664) +- Parallel runner uses `wait -n` on Bash 4.3+ instead of polling (#667) +- Outvar pattern in hot-path result helpers, dropping a fork per call (#662) +- Drop `grep`/`random_str` forks in `generate_id` and `normalize_variable_name` (#663) ## [0.36.0](https://github.com/TypedDevs/bashunit/compare/0.35.0...0.36.0) - 2026-05-07 diff --git a/src/runner.sh b/src/runner.sh index dc071561..b65eff41 100755 --- a/src/runner.sh +++ b/src/runner.sh @@ -57,71 +57,76 @@ function bashunit::runner::apply_interpolated_title() { printf '%s' "$interpolated" } -# All four helpers below use the outvar pattern (first argument is the name of -# the variable to assign into) so callers can avoid the per-test $(...) subshell -# capture in the hot path. Internal locals use a `__bu_` prefix to avoid name -# collisions with caller variables passed by name. - -# Writes the value of an encoded field (##KEY=value##) into the named outvar. -# Arguments: $1 outvar name, $2 test_execution_result, $3 key +# Hot-path result helpers below return their value via a dedicated global slot +# (`_BASHUNIT_RUNNER_*_OUT`) instead of stdout. This avoids the per-test +# `$(...)` subshell capture that dominated the result-parsing hot path. Callers +# invoke the helper and immediately read the slot: +# +# bashunit::runner::extract_subshell_type "$subshell_output" +# type=$_BASHUNIT_RUNNER_TYPE_OUT +# +# A dedicated slot per helper (rather than one shared slot) means nested or +# adjacent calls cannot clobber each other and callers don't need to copy out +# before every other helper runs. +_BASHUNIT_RUNNER_FIELD_OUT="" +_BASHUNIT_RUNNER_TOTAL_OUT="" +_BASHUNIT_RUNNER_TYPE_OUT="" +_BASHUNIT_RUNNER_OUTPUT_OUT="" + +# Writes the value of an encoded field (##KEY=value##) into _BASHUNIT_RUNNER_FIELD_OUT. +# Arguments: $1 test_execution_result, $2 key function bashunit::runner::extract_encoded_field() { - local __bu_out=$1 - local __bu_in=$2 - local __bu_key=$3 - local __bu_marker="##${__bu_key}=" - local __bu_val="" - case "$__bu_in" in - *"$__bu_marker"*) - local __bu_rest="${__bu_in#*"$__bu_marker"}" - __bu_val="${__bu_rest%%##*}" + local test_execution_result=$1 + local key=$2 + local marker="##${key}=" + case "$test_execution_result" in + *"$marker"*) + local rest="${test_execution_result#*"$marker"}" + _BASHUNIT_RUNNER_FIELD_OUT="${rest%%##*}" ;; + *) _BASHUNIT_RUNNER_FIELD_OUT="" ;; esac - eval "$__bu_out=\$__bu_val" } -# Writes the sum of all ASSERTIONS_* counters into the named outvar. -# Arguments: $1 outvar name, $2 test_execution_result +# Writes the sum of all ASSERTIONS_* counters into _BASHUNIT_RUNNER_TOTAL_OUT. +# Arguments: $1 test_execution_result function bashunit::runner::compute_total_assertions() { - local __bu_out=$1 - local __bu_in=$2 - local __bu_failed __bu_passed __bu_skipped __bu_incomplete __bu_snapshot - __bu_failed="${__bu_in##*##ASSERTIONS_FAILED=}" - __bu_failed="${__bu_failed%%##*}" - __bu_passed="${__bu_in##*##ASSERTIONS_PASSED=}" - __bu_passed="${__bu_passed%%##*}" - __bu_skipped="${__bu_in##*##ASSERTIONS_SKIPPED=}" - __bu_skipped="${__bu_skipped%%##*}" - __bu_incomplete="${__bu_in##*##ASSERTIONS_INCOMPLETE=}" - __bu_incomplete="${__bu_incomplete%%##*}" - __bu_snapshot="${__bu_in##*##ASSERTIONS_SNAPSHOT=}" - __bu_snapshot="${__bu_snapshot%%##*}" - local __bu_val - __bu_val=$((${__bu_failed:-0} + ${__bu_passed:-0} + ${__bu_skipped:-0})) - __bu_val=$((__bu_val + ${__bu_incomplete:-0} + ${__bu_snapshot:-0})) - eval "$__bu_out=\$__bu_val" + local test_execution_result=$1 + local failed passed skipped incomplete snapshot + failed="${test_execution_result##*##ASSERTIONS_FAILED=}" + failed="${failed%%##*}" + passed="${test_execution_result##*##ASSERTIONS_PASSED=}" + passed="${passed%%##*}" + skipped="${test_execution_result##*##ASSERTIONS_SKIPPED=}" + skipped="${skipped%%##*}" + incomplete="${test_execution_result##*##ASSERTIONS_INCOMPLETE=}" + incomplete="${incomplete%%##*}" + snapshot="${test_execution_result##*##ASSERTIONS_SNAPSHOT=}" + snapshot="${snapshot%%##*}" + local total + total=$((${failed:-0} + ${passed:-0} + ${skipped:-0})) + total=$((total + ${incomplete:-0} + ${snapshot:-0})) + _BASHUNIT_RUNNER_TOTAL_OUT=$total } -# Writes the subshell type marker (text inside leading [...]) into the named outvar. -# Arguments: $1 outvar name, $2 subshell_output +# Writes the subshell type marker (text inside leading [...]) into _BASHUNIT_RUNNER_TYPE_OUT. +# Arguments: $1 subshell_output function bashunit::runner::extract_subshell_type() { - local __bu_out=$1 - local __bu_in=$2 - local __bu_val="${__bu_in%%]*}" - __bu_val="${__bu_val#[}" - eval "$__bu_out=\$__bu_val" + local subshell_output=$1 + local type="${subshell_output%%]*}" + _BASHUNIT_RUNNER_TYPE_OUT="${type#[}" } # Writes the subshell output (minus the leading [type] marker, with embedded -# status markers replaced by newlines) into the named outvar. -# Arguments: $1 outvar name, $2 subshell_output +# status markers replaced by newlines) into _BASHUNIT_RUNNER_OUTPUT_OUT. +# Arguments: $1 subshell_output function bashunit::runner::format_subshell_output() { - local __bu_out=$1 - local __bu_in=$2 - local __bu_val="${__bu_in#*]}" - __bu_val=${__bu_val//\[failed\]/$'\n'} - __bu_val=${__bu_val//\[skipped\]/$'\n'} - __bu_val=${__bu_val//\[incomplete\]/$'\n'} - eval "$__bu_out=\$__bu_val" + local subshell_output=$1 + local line="${subshell_output#*]}" + line=${line//\[failed\]/$'\n'} + line=${line//\[skipped\]/$'\n'} + line=${line//\[incomplete\]/$'\n'} + _BASHUNIT_RUNNER_OUTPUT_OUT=$line } function bashunit::runner::detect_runtime_error() { @@ -837,9 +842,10 @@ function bashunit::runner::run_test() { local subshell_output=$(bashunit::runner::decode_subshell_output "$test_execution_result") if [ -n "$subshell_output" ]; then - local type - bashunit::runner::extract_subshell_type type "$subshell_output" - bashunit::runner::format_subshell_output subshell_output "$subshell_output" + bashunit::runner::extract_subshell_type "$subshell_output" + local type=$_BASHUNIT_RUNNER_TYPE_OUT + bashunit::runner::format_subshell_output "$subshell_output" + subshell_output=$_BASHUNIT_RUNNER_OUTPUT_OUT if ! bashunit::env::is_failures_only_enabled; then bashunit::state::print_line "$type" "$subshell_output" fi @@ -854,13 +860,15 @@ function bashunit::runner::run_test() { local test_exit_code="$_BASHUNIT_TEST_EXIT_CODE" - local total_assertions - bashunit::runner::compute_total_assertions total_assertions "$test_execution_result" + bashunit::runner::compute_total_assertions "$test_execution_result" + local total_assertions=$_BASHUNIT_RUNNER_TOTAL_OUT - local encoded_test_title hook_failure encoded_hook_message - bashunit::runner::extract_encoded_field encoded_test_title "$test_execution_result" "TEST_TITLE" - bashunit::runner::extract_encoded_field hook_failure "$test_execution_result" "TEST_HOOK_FAILURE" - bashunit::runner::extract_encoded_field encoded_hook_message "$test_execution_result" "TEST_HOOK_MESSAGE" + bashunit::runner::extract_encoded_field "$test_execution_result" "TEST_TITLE" + local encoded_test_title=$_BASHUNIT_RUNNER_FIELD_OUT + bashunit::runner::extract_encoded_field "$test_execution_result" "TEST_HOOK_FAILURE" + local hook_failure=$_BASHUNIT_RUNNER_FIELD_OUT + bashunit::runner::extract_encoded_field "$test_execution_result" "TEST_HOOK_MESSAGE" + local encoded_hook_message=$_BASHUNIT_RUNNER_FIELD_OUT local test_title="" [ -n "$encoded_test_title" ] && test_title="$(bashunit::helper::decode_base64 "$encoded_test_title")" diff --git a/src/test_doubles.sh b/src/test_doubles.sh index c1d6d253..e9f82b7b 100644 --- a/src/test_doubles.sh +++ b/src/test_doubles.sh @@ -2,6 +2,11 @@ declare -a _BASHUNIT_MOCKED_FUNCTIONS=() +# Spy/mock state is keyed by the sanitized command name and stored in globals +# prefixed with `_BASHUNIT_SPY_` so they cannot collide with caller locals. +# A test that does `local foo_times_file=...` is harmless because the helper +# resolves `_BASHUNIT_SPY_foo_TIMES_FILE` instead. + function bashunit::unmock() { local command=$1 @@ -16,8 +21,8 @@ function bashunit::unmock() { unset -f "$command" local variable variable="$(bashunit::helper::normalize_variable_name "$command")" - local times_file_var="${variable}_times_file" - local params_file_var="${variable}_params_file" + local times_file_var="_BASHUNIT_SPY_${variable}_TIMES_FILE" + local params_file_var="_BASHUNIT_SPY_${variable}_PARAMS_FILE" [ -f "${!times_file_var-}" ] && rm -f "${!times_file_var}" [ -f "${!params_file_var-}" ] && rm -f "${!params_file_var}" unset "$times_file_var" @@ -54,8 +59,8 @@ function bashunit::spy() { params_file=$(bashunit::temp_file "${test_id}_${variable}_params") echo 0 >"$times_file" : >"$params_file" - export "${variable}_times_file"="$times_file" - export "${variable}_params_file"="$params_file" + export "_BASHUNIT_SPY_${variable}_TIMES_FILE"="$times_file" + export "_BASHUNIT_SPY_${variable}_PARAMS_FILE"="$params_file" local body_suffix="" if [[ "$exit_code_or_impl" =~ ^[0-9]+$ ]]; then @@ -89,7 +94,7 @@ function assert_have_been_called() { local command=$1 local variable variable="$(bashunit::helper::normalize_variable_name "$command")" - local file_var="${variable}_times_file" + local file_var="_BASHUNIT_SPY_${variable}_TIMES_FILE" local times=0 if [ -f "${!file_var-}" ]; then times=$(cat "${!file_var}" 2>/dev/null || builtin echo 0) @@ -119,7 +124,7 @@ function assert_have_been_called_with() { local variable variable="$(bashunit::helper::normalize_variable_name "$command")" - local file_var="${variable}_params_file" + local file_var="_BASHUNIT_SPY_${variable}_PARAMS_FILE" local line="" if [ -f "${!file_var-}" ]; then if [ -n "$index" ]; then @@ -147,7 +152,7 @@ function assert_have_been_called_times() { local command=$2 local variable variable="$(bashunit::helper::normalize_variable_name "$command")" - local file_var="${variable}_times_file" + local file_var="_BASHUNIT_SPY_${variable}_TIMES_FILE" local times=0 if [ -f "${!file_var-}" ]; then times=$(cat "${!file_var}" 2>/dev/null || builtin echo 0) @@ -172,8 +177,8 @@ function assert_have_been_called_nth_with() { local variable variable="$(bashunit::helper::normalize_variable_name "$command")" - local times_file_var="${variable}_times_file" - local file_var="${variable}_params_file" + local times_file_var="_BASHUNIT_SPY_${variable}_TIMES_FILE" + local file_var="_BASHUNIT_SPY_${variable}_PARAMS_FILE" local label label="$(bashunit::helper::normalize_test_function_name "${FUNCNAME[1]}")" diff --git a/tests/unit/runner_test.sh b/tests/unit/runner_test.sh index 5c1e3af0..e75f8ed9 100644 --- a/tests/unit/runner_test.sh +++ b/tests/unit/runner_test.sh @@ -128,48 +128,79 @@ function test_detect_runtime_error_matches_unexpected_eof() { assert_same "line 5: unexpected EOF while looking for matching" "$actual" } -function test_extract_encoded_field_writes_value_to_outvar() { - local out="" - bashunit::runner::extract_encoded_field out \ +function test_extract_encoded_field_writes_value_to_slot() { + bashunit::runner::extract_encoded_field \ "preamble##TEST_TITLE=hello world##ASSERTIONS_PASSED=1" "TEST_TITLE" - assert_same "hello world" "$out" + assert_same "hello world" "$_BASHUNIT_RUNNER_FIELD_OUT" } function test_extract_encoded_field_writes_empty_when_key_missing() { - local out="prior" - bashunit::runner::extract_encoded_field out "##ASSERTIONS_PASSED=1" "TEST_TITLE" + _BASHUNIT_RUNNER_FIELD_OUT="prior" + bashunit::runner::extract_encoded_field "##ASSERTIONS_PASSED=1" "TEST_TITLE" - assert_empty "$out" + assert_empty "$_BASHUNIT_RUNNER_FIELD_OUT" } -function test_compute_total_assertions_sums_into_outvar() { - local out="" - bashunit::runner::compute_total_assertions out \ +function test_compute_total_assertions_sums_into_slot() { + bashunit::runner::compute_total_assertions \ "##ASSERTIONS_FAILED=1##ASSERTIONS_PASSED=2##ASSERTIONS_SKIPPED=3##ASSERTIONS_INCOMPLETE=4##ASSERTIONS_SNAPSHOT=5" - assert_same "15" "$out" + assert_same "15" "$_BASHUNIT_RUNNER_TOTAL_OUT" } function test_compute_total_assertions_treats_missing_counters_as_zero() { - local out="" - bashunit::runner::compute_total_assertions out "##ASSERTIONS_PASSED=2" + bashunit::runner::compute_total_assertions "##ASSERTIONS_PASSED=2" - assert_same "2" "$out" + assert_same "2" "$_BASHUNIT_RUNNER_TOTAL_OUT" } -function test_extract_subshell_type_strips_brackets_into_outvar() { - local out="" - bashunit::runner::extract_subshell_type out "[failed] something happened" +function test_extract_subshell_type_strips_brackets_into_slot() { + bashunit::runner::extract_subshell_type "[failed] something happened" - assert_same "failed" "$out" + assert_same "failed" "$_BASHUNIT_RUNNER_TYPE_OUT" } function test_format_subshell_output_strips_type_and_expands_markers() { - local out="" - bashunit::runner::format_subshell_output out "[failed] line1[skipped]line2[incomplete]line3" + bashunit::runner::format_subshell_output "[failed] line1[skipped]line2[incomplete]line3" local expected expected=$' line1\nline2\nline3' - assert_same "$expected" "$out" + assert_same "$expected" "$_BASHUNIT_RUNNER_OUTPUT_OUT" +} + +# Regression for #674: caller-named locals must not be silently corrupted by +# the helpers. With the global-slot return pattern the helper never touches +# caller-named variables, so a caller can freely use natural names (e.g. +# `subshell_output`, `test_execution_result`) without any shadowing risk. +function test_format_subshell_output_does_not_touch_caller_locals() { + local subshell_output="raw" + bashunit::runner::format_subshell_output "[failed] formatted" + + assert_same " formatted" "$_BASHUNIT_RUNNER_OUTPUT_OUT" + assert_same "raw" "$subshell_output" +} + +function test_extract_subshell_type_does_not_touch_caller_locals() { + local subshell_output="[failed] payload" + bashunit::runner::extract_subshell_type "$subshell_output" + + assert_same "failed" "$_BASHUNIT_RUNNER_TYPE_OUT" + assert_same "[failed] payload" "$subshell_output" +} + +function test_extract_encoded_field_does_not_touch_caller_locals() { + local test_execution_result="##TEST_TITLE=hi##ASSERTIONS_PASSED=1" + bashunit::runner::extract_encoded_field "$test_execution_result" "TEST_TITLE" + + assert_same "hi" "$_BASHUNIT_RUNNER_FIELD_OUT" + assert_same "##TEST_TITLE=hi##ASSERTIONS_PASSED=1" "$test_execution_result" +} + +function test_compute_total_assertions_does_not_touch_caller_locals() { + local test_execution_result="##ASSERTIONS_PASSED=4##ASSERTIONS_FAILED=1" + bashunit::runner::compute_total_assertions "$test_execution_result" + + assert_same "5" "$_BASHUNIT_RUNNER_TOTAL_OUT" + assert_same "##ASSERTIONS_PASSED=4##ASSERTIONS_FAILED=1" "$test_execution_result" }