From c31948e21307f6706f7f414ccc3aaee707b48878 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Sat, 25 Jul 2026 15:07:22 +0200 Subject: [PATCH] fix(benchmark): reject a malformed annotation instead of ignoring it Each marker is extracted with a numeric sed pattern and fell back to the default when it did not match, so a malformed value was indistinguishable from an absent annotation: @revs=abc ran 1 revolution @its=abc ran 1 iteration @max_ms=abc dropped the threshold, so it could never fail all reporting success while measuring something other than what was written. A marker that is present but yielded no value is now an error; an absent annotation still takes its default. The call site captured into `read <<<"$(...)"`, which discards the command substitution's status, so it now captures into a variable first. Closes #884 --- CHANGELOG.md | 1 + src/benchmark.sh | 28 ++++++++++++++++++++++++++ src/runner.sh | 6 +++++- tests/unit/benchmark_test.sh | 39 ++++++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 290b431c..8129ad44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - The `--parallel` unsupported-OS warning no longer claims Alpine is excluded: Alpine has been a supported parallel platform since the race conditions were fixed, the message was simply never updated ### Fixed +- A malformed benchmark annotation is now an error instead of a silent fallback to the default. `@revs=abc` quietly ran a single revolution, `@its=abc` a single iteration, and `@max_ms=abc` dropped the threshold entirely so the benchmark could never fail — each reporting success while measuring something other than what was written (#884) - An empty entry in `.env` no longer overrides a value the caller exported or set on the command line. `.env` is sourced under `set -o allexport`, so every line was an unconditional assignment: merely *listing* a name blanked it, and `BASHUNIT_OUTPUT_FORMAT=tap ./bashunit` silently stopped working in any project whose `.env` mentioned that setting. An empty entry now means "not configured here"; an entry with a value still takes effect for the project. This had been actively concealing defects — two of the bugs fixed in #879 were not reproducible from inside a repo checkout for exactly this reason (#865) - `BASHUNIT_COVERAGE_THRESHOLD_LOW`/`BASHUNIT_COVERAGE_THRESHOLD_HIGH` (env-only, no CLI flag) now validate as non-negative integers like the other numeric settings. They are compared with `[ -ge ]` in the coverage class lookup, which errors instead of returning false on a non-integer value: a bad threshold leaked a raw `integer expression expected` into the coverage report and silently mis-bucketed every file's high/medium/low class - `@max_ms` benchmark annotations with a decimal value (e.g. `@max_ms=100.5`) no longer leak a raw `integer expression expected` into the results table and always render as failing (`>`) regardless of the actual average; the threshold comparison now tolerates fractional operands like the average itself already could diff --git a/src/benchmark.sh b/src/benchmark.sh index 457193c6..d13c3b26 100644 --- a/src/benchmark.sh +++ b/src/benchmark.sh @@ -6,6 +6,29 @@ _BASHUNIT_BENCH_ITS=() _BASHUNIT_BENCH_AVERAGES=() _BASHUNIT_BENCH_MAX_MILLIS=() +## +# Fails when a marker is present in the annotation but produced no value, which +# only happens when its argument is malformed. Silently falling back to the +# default ran a different benchmark than the one asked for: `@revs=abc` quietly +# became one revolution, and `@max_ms=abc` dropped the threshold entirely (#884). +# Arguments: $1 annotation line, $2 marker name, $3 value extracted so far +## +function bashunit::benchmark::reject_malformed_marker() { + local annotation=$1 + local marker=$2 + local extracted=$3 + + [ -n "$extracted" ] && return 0 + case "$annotation" in + *"@$marker="*) ;; + *) return 0 ;; + esac + + printf "%sError: @%s in '%s' is not a valid value.%s\n" \ + "${_BASHUNIT_COLOR_FAILED}" "$marker" "$annotation" "${_BASHUNIT_COLOR_DEFAULT}" >&2 + return 1 +} + function bashunit::benchmark::parse_annotations() { local fn_name=$1 local script=$2 @@ -26,6 +49,8 @@ function bashunit::benchmark::parse_annotations() { revs="$_extracted" fi fi + bashunit::benchmark::reject_malformed_marker "$annotation" "revs" "$_extracted" || return 1 + bashunit::benchmark::reject_malformed_marker "$annotation" "revolutions" "$_extracted" || return 1 _extracted=$(echo "$annotation" | sed -n 's/.*@its=\([0-9][0-9]*\).*/\1/p') if [ -n "$_extracted" ]; then @@ -36,11 +61,14 @@ function bashunit::benchmark::parse_annotations() { its="$_extracted" fi fi + bashunit::benchmark::reject_malformed_marker "$annotation" "its" "$_extracted" || return 1 + bashunit::benchmark::reject_malformed_marker "$annotation" "iterations" "$_extracted" || return 1 _extracted=$(echo "$annotation" | sed -n 's/.*@max_ms=\([0-9.][0-9.]*\).*/\1/p') if [ -n "$_extracted" ]; then max_ms="$_extracted" fi + bashunit::benchmark::reject_malformed_marker "$annotation" "max_ms" "$max_ms" || return 1 if [ -n "$max_ms" ]; then echo "$revs" "$its" "$max_ms" diff --git a/src/runner.sh b/src/runner.sh index ae58f032..460564af 100755 --- a/src/runner.sh +++ b/src/runner.sh @@ -962,7 +962,11 @@ function bashunit::runner::call_bench_functions() { local fn_name for fn_name in "${functions_to_run[@]+"${functions_to_run[@]}"}"; do - read -r revs its max_ms <<<"$(bashunit::benchmark::parse_annotations "$fn_name" "$script")" + # Capture separately so a malformed annotation aborts the run: the exit + # status of a $(...) inside `read <<<` is otherwise discarded (#884). + local parsed_annotations + parsed_annotations=$(bashunit::benchmark::parse_annotations "$fn_name" "$script") || exit 1 + read -r revs its max_ms <<<"$parsed_annotations" bashunit::benchmark::run_function "$fn_name" "$revs" "$its" "$max_ms" unset -v fn_name done diff --git a/tests/unit/benchmark_test.sh b/tests/unit/benchmark_test.sh index 69cb022f..a3af87af 100755 --- a/tests/unit/benchmark_test.sh +++ b/tests/unit/benchmark_test.sh @@ -275,3 +275,42 @@ function test_run_function_with_multiple_iterations() { assert_same "3" "${_BASHUNIT_BENCH_ITS[0]}" [[ -n "${_BASHUNIT_BENCH_AVERAGES[0]}" ]] } + +# A marker that is present but yielded no number is a typo, not an absent +# annotation. Falling back to the default silently ran a different benchmark +# than the one asked for: `@revs=abc` quietly became one revolution (#884). +function test_parse_annotations_rejects_a_malformed_revs() { + local fixture ec=0 + fixture="$(bashunit::temp_dir)/malformed_bench.sh" + printf '#!/usr/bin/env bash\n\n# @revs=abc\nfunction bench_x() { :; }\n' >"$fixture" + + local output + output=$(bashunit::benchmark::parse_annotations bench_x "$fixture" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "@revs" "$output" +} + +function test_parse_annotations_rejects_a_malformed_max_ms() { + local fixture ec=0 + fixture="$(bashunit::temp_dir)/malformed_max_bench.sh" + printf '#!/usr/bin/env bash\n\n# @max_ms=abc\nfunction bench_x() { :; }\n' >"$fixture" + + local output + output=$(bashunit::benchmark::parse_annotations bench_x "$fixture" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "@max_ms" "$output" +} + +function test_parse_annotations_accepts_a_function_with_no_annotation_at_all() { + local fixture ec=0 + fixture="$(bashunit::temp_dir)/plain_bench.sh" + printf '#!/usr/bin/env bash\n\nfunction bench_x() { :; }\n' >"$fixture" + + local output + output=$(bashunit::benchmark::parse_annotations bench_x "$fixture" 2>&1) || ec=$? + + assert_successful_code "" "" "$ec" + assert_same "1 1" "$output" +}