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