From bb91166e5a7b1fe6f5d8e7b0d3a58a2902ed01b5 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Wed, 9 Sep 2026 10:32:33 +0200 Subject: [PATCH] perf(runner): count the header's tests without sourcing every file again Printing "Running N tests" sourced the whole suite a second time and ran every data provider a second time, in a double subshell per file, before the run began: 1.72s over this repo's 241 files. The provider scan already walks every line of every file with awk, and is already memoized so the runner reuses it. Have that same pass also emit the file's column-0 function definitions, and count the names that survive get_functions_to_run. 124 of 241 files are counted this way and the pass drops to 1.51s. The other 117 keep sourcing, because a static scan would be wrong about them, in both directions. A provider's row count is only knowable by running it. eval, a nested source and an indented definition can define a test the scan cannot see, which would undercount. A heredoc or a multi-line string can contain a `function test_x()` that is text rather than a definition, which would overcount -- acceptance tests write their fixtures exactly that way. Each of those marks the file dynamic; the scan does not try to parse where a string ends, because it also builds the provider map and a line-skipping bug there would mis-wire real tests. The gate is the differential the issue asked for, kept as a test: for every file in tests/, under three filter settings, the static count must equal the sourcing count. It has to run in a shell that has sourced no test file, since the sourcing path asks `compgen -A function` and would otherwise also count the test functions of whichever file is running. That gate immediately found a latent bug. get_functions_to_run checked for duplicates with a substring match, so a name another selected name starts with -- `test_a` against `test_ab` -- read as a duplicate, and the function returns 1, which made the file select nothing. `compgen` output is sorted, so a prefix always arrived first and nothing had reached it; definition order does. Closes #1347 Claude-Session: https://claude.ai/code/session_01EXYWTGLjf7qM8Ru3GakDRm --- CHANGELOG.md | 2 + src/helper/discovery.sh | 99 +++++++++++++++++-- src/helper/naming.sh | 6 ++ src/helper/provider.sh | 59 ++++++++++- tests/unit/helper/helper_test.sh | 84 ++++++++++++++++ tests/unit/project/total_tests_parity_test.sh | 95 ++++++++++++++++++ 6 files changed, 335 insertions(+), 10 deletions(-) create mode 100644 tests/unit/project/total_tests_parity_test.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index d3af5303..f71a4454 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,11 +3,13 @@ ## Unreleased ### Changed +- Performance: startup is faster on suites of plain test files. Printing "Running N tests" sourced every file a second time and re-ran every data provider before the run began. A file whose functions the provider scan can already see is counted from that scan instead: over this repo's 241 files the counting pass went from 1.72s to 1.51s, and 124 of them no longer source or run a provider twice. Files with a data provider, a heredoc, a multi-line string, an `eval`, a nested `source` or a conditional definition keep the old path, so the count can never disagree with the run (#1347) - Performance: a sequential run is about 0.7ms faster per test — a 500-test file went from 2.75s to 2.40s on macOS arm64, bash 3.2. Every test forked a subshell to read its own definition line, for a `:` that only a failure message and a report row ever use. It is resolved on demand now (#1346) - A standalone `bashunit assert …` no longer reports a source location belonging to another process. Launched from inside a test, it inherited that test's exported location and printed it as its own (#1346) - 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 +- A test function whose name another selected name starts with is no longer rejected as a duplicate. `test_a` alongside `test_ab` read as one name twice, which made the whole file select nothing. Only definition order reached it, so nothing hit it before (#1347) - 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) - A `--parallel` run no longer replays bash's `child setpgid` warning as the stderr of a file that wrote nothing. Job control puts each test in its own process group. The parent-side `setpgid` then loses a harmless race with a child that already exec'd. The child sets the group first, so only the message was ever wrong (#1344) diff --git a/src/helper/discovery.sh b/src/helper/discovery.sh index 59257669..75113303 100644 --- a/src/helper/discovery.sh +++ b/src/helper/discovery.sh @@ -147,7 +147,11 @@ function bashunit::helper::get_functions_to_run() { fi if [ "$_fn_match" = true ]; then local _dup=false - case "$filtered_functions" in *" $fn"*) _dup=true ;; esac + # Both delimiters, or a name is "already present" whenever an earlier one + # merely starts with it: `test_a` reads as a duplicate of `test_ab`. That + # stayed hidden while the only caller fed this `compgen` output, which is + # sorted, so a prefix always arrived first (#1347). + case "$filtered_functions " in *" $fn "*) _dup=true ;; esac if [ "$_dup" = true ]; then return 1 fi @@ -206,6 +210,62 @@ function bashunit::helper::find_files_recursive() { _BASHUNIT_HELPER_VARNAME_OUT="" + +_BASHUNIT_HELPER_FILE_COUNT_OUT=0 + +## +# True when the provider map just built for a file is enough to count its +# tests without sourcing it. +# +# Sourcing every file a second time, only to run `compgen -A function` in a +# subshell, cost 671ms over this repo's 240 files before a single test ran, and +# re-ran every data provider on the way (#1347). Two shapes still need it: a +# provider's row count is only knowable by running it, and a file that could +# define a test out of the scan's sight (eval, a nested source, a conditional +# definition) would otherwise be undercounted -- a header that disagrees with +# the run is worse than a slow one. +## +function bashunit::helper::_can_count_statically() { + if [ "$_BASHUNIT_PROVIDER_MAP_DYNAMIC" = true ]; then + return 1 + fi + + if [ "${#_BASHUNIT_PROVIDER_MAP_FNS[@]}" -ne 0 ]; then + return 1 + fi + + return 0 +} + +## +# Counts the current provider map's test functions into +# _BASHUNIT_HELPER_FILE_COUNT_OUT, applying the same selection the runner does. +# +# Every function without a provider is one test, which is what the sourcing +# path counts too; only the names come from the scan rather than from +# `compgen`. `get_functions_to_run` returns 1 on a duplicate name, exactly as +# it does for the sourcing path, where `|| true` swallows it. +# +# Arguments: $1 - the --filter value +## +function bashunit::helper::_count_tests_statically() { + local filter=$1 + # Set before the call, not after: get_functions_to_run word-splits its third + # argument with whatever IFS is current. + local IFS=$' \t\n' + local filtered_functions + filtered_functions=$(bashunit::helper::get_functions_to_run \ + "test" "$filter" "$_BASHUNIT_PROVIDER_MAP_TEST_FNS") || true + + local count=0 + local fn + for fn in $filtered_functions; do + count=$((count + 1)) + done + + _BASHUNIT_HELPER_FILE_COUNT_OUT=$count +} + function bashunit::helper::find_total_tests() { local filter=${1:-} shift || true @@ -230,8 +290,35 @@ function bashunit::helper::find_total_tests() { # file is a cache hit too — one awk scan per file instead of two. bashunit::helper::build_provider_map "$file" - local file_count - file_count=$( ( + if bashunit::helper::_can_count_statically; then + bashunit::helper::_count_tests_statically "$filter" + else + bashunit::helper::_count_tests_by_sourcing "$file" "$filter" + fi + total_count=$((total_count + _BASHUNIT_HELPER_FILE_COUNT_OUT)) + done + + _BASHUNIT_HELPER_TOTAL_TESTS_OUT=$total_count + echo "$total_count" +} + +## +# Counts a file's tests into _BASHUNIT_HELPER_FILE_COUNT_OUT by sourcing it in +# a subshell and asking `compgen`, then running each data provider to learn how +# many rows it yields. +# +# The only way to count a provider, and the only way to see a function this +# file defines through eval, a nested source or a condition. Everything else +# takes the static path, because this one costs a double subshell, a re-source +# and a second run of every provider in the suite (#1347). +# +# Arguments: $1 - the test file, $2 - the --filter value +## +function bashunit::helper::_count_tests_by_sourcing() { + local file=$1 + local filter=$2 + + _BASHUNIT_HELPER_FILE_COUNT_OUT=$( ( # shellcheck source=/dev/null source "$file" local all_fn_names @@ -271,12 +358,6 @@ function bashunit::helper::find_total_tests() { echo "$count" )) - - total_count=$((total_count + file_count)) - done - - _BASHUNIT_HELPER_TOTAL_TESTS_OUT=$total_count - echo "$total_count" } diff --git a/src/helper/naming.sh b/src/helper/naming.sh index ba1fa3ef..92266618 100644 --- a/src/helper/naming.sh +++ b/src/helper/naming.sh @@ -206,4 +206,10 @@ _BASHUNIT_PROVIDER_FN_OUT="" # Set true when the scanned file carries the "# bashunit: no-parallel-tests" # opt-out; detected in the same awk pass to avoid a per-file grep fork (#774). _BASHUNIT_PROVIDER_MAP_NO_PARALLEL=false +# Every function the scanned file defines at column 0, space-separated, and +# whether the file could also define one out of the scan's sight. The header +# counts a file's tests from these instead of sourcing it a second time and +# re-running its providers (#1347). +_BASHUNIT_PROVIDER_MAP_TEST_FNS="" +_BASHUNIT_PROVIDER_MAP_DYNAMIC=false diff --git a/src/helper/provider.sh b/src/helper/provider.sh index 1f6e06d1..b12417ed 100644 --- a/src/helper/provider.sh +++ b/src/helper/provider.sh @@ -49,6 +49,8 @@ function bashunit::helper::build_provider_map() { _BASHUNIT_PROVIDER_MAP_FNS=() _BASHUNIT_PROVIDER_MAP_PROVIDERS=() _BASHUNIT_PROVIDER_MAP_NO_PARALLEL=false + _BASHUNIT_PROVIDER_MAP_TEST_FNS="" + _BASHUNIT_PROVIDER_MAP_DYNAMIC=false bashunit::helper::annotations_reset @@ -80,6 +82,14 @@ function bashunit::helper::build_provider_map() { [ "$provider" = "1" ] && _BASHUNIT_PROVIDER_MAP_NO_PARALLEL=true continue fi + if [ "$fn" = "@@testfns@@" ]; then + _BASHUNIT_PROVIDER_MAP_TEST_FNS="$provider" + continue + fi + if [ "$fn" = "@@dynamic@@" ]; then + _BASHUNIT_PROVIDER_MAP_DYNAMIC=true + continue + fi if [ "$fn" = "@@annot@@" ]; then [ "$annot_timeout" = "@@none@@" ] && annot_timeout="" [ "$annot_retry" = "@@none@@" ] && annot_retry="" @@ -132,6 +142,49 @@ function bashunit::helper::build_provider_map() { sub(/[[:space:]]*\(\).*/, "", fn) } + # A definition at column 0 is one this scan can be sure the file defines + # unconditionally, which is what lets the header count without sourcing. + # Anything that could define a function this scan cannot see -- eval, + # a nested source, an indented (so conditional or nested) definition -- + # marks the file dynamic and sends the count back to sourcing. + if (match($0, /^(function[[:space:]]+)?[A-Za-z_][A-Za-z0-9_:]*[[:space:]]*\(\)/)) { + # Accumulated, not emitted per definition: the read loop below runs + # once per row, and a row per function cost about as much as counting + # statically saved. + top_level_fns = top_level_fns " " fn + } else if (is_fn) { + dynamic = 1 + } + # A heredoc marks the file dynamic too, for the opposite reason: an + # acceptance test writes its fixtures with one, and a `function test_x()` + # in that body is text, not a definition -- counted statically it would + # inflate the header. So does a line whose quotes do not balance, which + # is how the rest of them write a fixture: a multi-line string whose + # continuation lines start at column 0 and look exactly like definitions. + # Escapes and balanced spans come out first, so an apostrophe inside a + # double-quoted string is not mistaken for one -- that alone was flagging + # a fifth of the suite. + # + # Both are flagged rather than parsed. This pass also builds the provider + # map, so a line-skipping bug here would mis-wire real tests; and a rule + # that guesses wrong about where a string ends can hide a real function, + # which is the one error a count must never make. Over-flagging only + # costs the file its fast path. + if (index($0, "<<") > 0) { + dynamic = 1 + } + _t = $0 + gsub(/\\./, "", _t) + gsub(/"[^"]*"/, "", _t) + gsub(/\047[^\047]*\047/, "", _t) + if (index(_t, "\047") > 0 || index(_t, "\"") > 0) { + dynamic = 1 + } + if (match($0, /(^|[^[:alnum:]_])eval([^[:alnum:]_]|$)/) || + match($0, /^[[:space:]]*(\.|source)[[:space:]]/)) { + dynamic = 1 + } + if (pending != "" && NR - pending_line <= 2) { if (is_fn) { printf "%s\t%s\n", fn, pending @@ -158,7 +211,11 @@ function bashunit::helper::build_provider_map() { a_skip = "" a_reason = "" } - END { printf "@@no_parallel@@\t%d\n", no_parallel } + END { + printf "@@no_parallel@@\t%d\n", no_parallel + if (dynamic) { printf "@@dynamic@@\t1\n" } + else { printf "@@testfns@@\t%s\n", top_level_fns } + } ' "$script" 2>/dev/null)" } diff --git a/tests/unit/helper/helper_test.sh b/tests/unit/helper/helper_test.sh index 6e59a3d5..9943e0da 100644 --- a/tests/unit/helper/helper_test.sh +++ b/tests/unit/helper/helper_test.sh @@ -502,6 +502,90 @@ function test_find_total_tests_with_filter() { assert_same "3" "$(helpers_test::find_total_in_subshell "with_provider" "$file1" "$file2")" } +# Counting used to source every file a second time, just to run `compgen -A +# function` in a subshell -- 671ms over this repo's 240 files, before a single +# test ran, and it re-ran every data provider on the way (#1347). A file whose +# functions the provider scan can already see is counted from that scan. +# The duplicate guard used to be a substring match, so a name that another +# already-selected name starts with read as a duplicate and the whole file +# selected nothing. Only definition order exposes it: `compgen` is sorted, so a +# prefix always came first there (#1347). +function test_get_functions_to_run_does_not_call_a_prefix_a_duplicate() { + local actual + actual="$(bashunit::helper::get_functions_to_run "test" "" \ + "test_alpha_extended test_alpha")" + + assert_same "test_alpha_extended test_alpha" "$actual" +} + +function test_get_functions_to_run_still_rejects_a_real_duplicate() { + local status=0 + bashunit::helper::get_functions_to_run "test" "" \ + "test_alpha test_alpha" >/dev/null || status=$? + + assert_same "1" "$status" +} + +function test_find_total_tests_does_not_source_a_static_file() { + local dir + dir="$(bashunit::temp_dir)" + local marker="$dir/was_sourced" + local fixture="$dir/static_count_fixture.sh" + { + echo "echo x >>\"$marker\"" + echo 'function test_a() { :; }' + echo 'function test_b() { :; }' + } >"$fixture" + + local total + total="$(helpers_test::find_total_in_subshell "" "$fixture")" + + assert_same "2" "$total" + assert_file_not_exists "$marker" +} + +# A provider's row count is only knowable by running it, so those files keep +# the sourcing path. +function test_find_total_tests_sources_a_file_with_a_provider() { + local dir + dir="$(bashunit::temp_dir)" + local marker="$dir/was_sourced" + local fixture="$dir/provider_count_fixture.sh" + { + echo "echo x >>\"$marker\"" + echo 'function provider_rows() { echo 1; echo 2; echo 3; }' + echo '# @data_provider provider_rows' + echo 'function test_rows() { :; }' + } >"$fixture" + + local total + total="$(helpers_test::find_total_in_subshell "" "$fixture")" + + assert_same "3" "$total" + assert_file_exists "$marker" +} + +# A static scan cannot see a function `eval`, a nested `source` or a +# conditional definition creates, and a header that disagrees with the run is +# worse than a slow one. Those files fall back to sourcing. +function test_find_total_tests_sources_a_file_that_defines_functions_dynamically() { + local dir + dir="$(bashunit::temp_dir)" + local marker="$dir/was_sourced" + local fixture="$dir/dynamic_count_fixture.sh" + { + echo "echo x >>\"$marker\"" + echo 'eval "function test_evaled() { :; }"' + echo 'function test_plain() { :; }' + } >"$fixture" + + local total + total="$(helpers_test::find_total_in_subshell "" "$fixture")" + + assert_same "2" "$total" + assert_file_exists "$marker" +} + function test_parse_file_path_filter_plain_path() { local result result=$(bashunit::helper::parse_file_path_filter "tests/unit/example_test.sh") || true diff --git a/tests/unit/project/total_tests_parity_test.sh b/tests/unit/project/total_tests_parity_test.sh new file mode 100644 index 00000000..02c5699d --- /dev/null +++ b/tests/unit/project/total_tests_parity_test.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash + +# The "Running N tests" header counts a file's tests from the provider scan +# instead of sourcing the file a second time and re-running every data provider +# (#1347). A static scan cannot see a function that eval, a nested source or a +# condition defines, and files like that keep the sourcing path -- but nothing +# at runtime checks that the two paths agree on the files that do not. +# +# This is that check, over every test file in the repo: for each file the static +# path claims, its count must equal what sourcing the file produces. A +# disagreement means the header lies about how many tests are about to run. + +function set_up_before_script() { + ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)" +} + +# Both counts for every repo test file, compared in a shell that has sourced +# nothing but src/. The comparison cannot run in this one: the sourcing path +# asks `compgen -A function`, which would also see the test functions of +# whichever file is currently running -- every file would come out four tests +# short, and a real mismatch would hide in the noise. +# +# Reports each offender, so a failure names the file instead of just a number. +# Arguments: $1 - the --filter value +function _parity_mismatches() { + local filter=$1 + + ( + cd "$ROOT_DIR" || return 1 + find tests -name '*[tT]est.sh' -type f -print0 | + xargs -0 bash -c ' + BASHUNIT_ROOT_DIR="$PWD" + export BASHUNIT_ROOT_DIR + # find_total_tests needs "data_set", so globals come first. + # shellcheck source=/dev/null + source src/api/globals.sh + # shellcheck source=/dev/null + source src/helper/index.sh + filter=$1 + shift + for file in "$@"; do + bashunit::helper::build_provider_map "$file" + bashunit::helper::_can_count_statically || continue + bashunit::helper::_count_tests_statically "$filter" + static=$_BASHUNIT_HELPER_FILE_COUNT_OUT + bashunit::helper::_count_tests_by_sourcing "$file" "$filter" + sourced=$_BASHUNIT_HELPER_FILE_COUNT_OUT + if [ "$static" != "$sourced" ]; then + echo "$file: static=$static sourced=$sourced" + fi + done + ' bash "$filter" + ) +} + +function test_static_and_sourced_counts_agree_on_every_test_file() { + assert_empty "$(_parity_mismatches "")" +} + +function test_static_and_sourced_counts_agree_under_a_filter() { + assert_empty "$(_parity_mismatches "should")" +} + +function test_static_and_sourced_counts_agree_under_an_exclude_filter() { + local original=${BASHUNIT_EXCLUDE_FILTER:-} + export BASHUNIT_EXCLUDE_FILTER="should" + + local mismatches + mismatches="$(_parity_mismatches "")" + + export BASHUNIT_EXCLUDE_FILTER="$original" + assert_empty "$mismatches" +} + +# A check that cannot fail proves nothing: the static path must actually be +# claiming files, or the three tests above would pass over an empty set. +function test_the_static_path_claims_most_of_the_suite() { + local file + local claimed=0 + local total=0 + local files + files="$(find "$ROOT_DIR/tests" -name '*[tT]est.sh' -type f)" + + while IFS= read -r file; do + [ -z "$file" ] && continue + total=$((total + 1)) + bashunit::helper::build_provider_map "$file" + if bashunit::helper::_can_count_statically; then + claimed=$((claimed + 1)) + fi + done <<<"$files" + + assert_greater_than 0 "$total" + assert_greater_than $((total / 2)) "$claimed" +}