fix(benchmark): reject a malformed annotation instead of ignoring it

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

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"

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

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