From 6ca638045103a5cb4f834204ab5b9dc5bc913b7c Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Fri, 21 Aug 2026 16:20:53 +0200 Subject: [PATCH 1/2] fix(runner): fail a parallel run aborted by a malformed annotation A malformed `@timeout` or `@retry` aborts the file it is in. Sequentially that exits the run, but under `--parallel` the abort happened inside the file's worker and never reached the parent: alongside one passing file the run printed the error and still reported "All tests passed" with exit 0, so the failure never reached CI. The abort is now recorded as one failed test, which is the question #1335 left open. It travels on both of the channels that cross the fork, because the console summary and the report writers read different ones: the named entry is spooled for the parent to replay, the counter goes in the payload the parent aggregates. Console, json, junit, html and markdown now agree, and the JUnit `testcase` carries the offending function and the real reason. Recorded from the aborting frame for two reasons. It is the only frame that still holds the function and the message, the caller having nothing but an exit status. And it is the worker under `--parallel`, whose own counter is discarded by design, so each channel contributes exactly once; recording in the parent instead would land on top of the published payload and re-create #1301. The wording is built once, so the report entry and the stderr line cannot drift. No console line is printed from there: the parent already replays the worker's stderr, and printing again would report one problem twice in two shapes. Sequential behaviour, the error text and the exit code are unchanged. Closes #1335 --- CHANGELOG.md | 1 + src/helper/annotations.sh | 13 ++++- src/runner/discovery.sh | 12 +++-- src/runner/exec.sh | 7 ++- src/runner/result.sh | 34 ++++++++++++- tests/acceptance/bashunit_annotations_test.sh | 49 +++++++++++++++++++ 6 files changed, 107 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffe0fe5f..c35269bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - A test file that fails to source sweeps its script temp files, so a `bashunit::temp_file` it created at top level no longer survives the run. `bashunit bench` already did this (#1325) - A malformed `@timeout` or `@retry` runs `tear_down_after_script` before it aborts the run, so the file releases what `set_up_before_script` acquired. Sequential and `--parallel` both leaked it (#1329) - Ctrl-C releases what an interrupted `--parallel` run acquired: the file's `tear_down_after_script` and the `tear_down` of a test in flight. The worker that owns the file's hook now handles the signal and reaches its test bodies, which a kill from the parent could not (#1331) +- A malformed `@timeout` or `@retry` fails a `--parallel` run alongside a passing file. The abort happened inside the file's worker and never reached the parent, so the run printed the error and still exited 0, which kept it out of CI (#1335) ## [0.50.0](https://github.com/TypedDevs/bashunit/compare/0.49.0...0.50.0) - 2026-08-18 diff --git a/src/helper/annotations.sh b/src/helper/annotations.sh index b3e70502..357fc5fb 100644 --- a/src/helper/annotations.sh +++ b/src/helper/annotations.sh @@ -118,10 +118,19 @@ function bashunit::helper::annotations_validate() { done } +# The rejection the validator last reported. The caller records it as a failed +# test, and building the wording once here is what keeps that report entry and +# the stderr line above from drifting apart (#1335). +_BASHUNIT_ANNOT_REJECT_FN_OUT="" +_BASHUNIT_ANNOT_REJECT_MSG_OUT="" + ## # Arguments: $1 - script, $2 - function, $3 - marker, $4 - offending value ## function bashunit::helper::_annotations_reject() { - printf "%sError: @%s '%s' above %s in %s is not a non-negative integer.%s\n" \ - "${_BASHUNIT_COLOR_FAILED}" "$3" "$4" "$2" "$1" "${_BASHUNIT_COLOR_DEFAULT}" >&2 + _BASHUNIT_ANNOT_REJECT_FN_OUT=$2 + _BASHUNIT_ANNOT_REJECT_MSG_OUT="@$3 '$4' above $2 in $1 is not a non-negative integer." + printf "%sError: %s%s\n" \ + "${_BASHUNIT_COLOR_FAILED}" "$_BASHUNIT_ANNOT_REJECT_MSG_OUT" \ + "${_BASHUNIT_COLOR_DEFAULT}" >&2 } diff --git a/src/runner/discovery.sh b/src/runner/discovery.sh index 22909fbc..6e0ce025 100644 --- a/src/runner/discovery.sh +++ b/src/runner/discovery.sh @@ -253,16 +253,20 @@ function bashunit::runner::load_test_files() { # as run_with_timeout's watchdog. set -m # An aborting annotation returns non-zero here, and this shell runs with - # errexit off, so the hook below still runs. The status is dropped: the - # worker has no channel to abort the parent with, which is a separate - # exit-code defect and not this teardown leak (#1329). + # errexit off, so the hook below still runs. The status needs no reading: + # the aborting frame already recorded the failure on both channels, since + # it is the only one holding the reason (#1335). Never guard this with + # `||` to read it -- a command on the left of `||` runs with errexit + # ignored, and bash carries that down into every function and subshell it + # calls, which stopped the `set -e` that aborts a failing set_up. bashunit::runner::call_test_functions "$test_file" "$_cached_fns" # Settles the debt the parent recorded before dispatch, so a signal that # lands from here on cannot run the hook a second time. _BASHUNIT_FILE_TEARDOWN_PENDING="" bashunit::runner::run_tear_down_after_script "$test_file" # A hook failure recorded in here dies with the subshell (#1147), so - # publish it the way a test publishes its result. + # publish it the way a test publishes its result. Reads $? directly, so + # nothing may come between it and the call above. bashunit::runner::publish_file_hook_failure "$?" "$test_file" } 2>"$_worker_stderr" & else diff --git a/src/runner/exec.sh b/src/runner/exec.sh index ea5b064d..8c166fa0 100644 --- a/src/runner/exec.sh +++ b/src/runner/exec.sh @@ -132,7 +132,12 @@ function bashunit::runner::call_test_functions() { # Before anything runs: a value the runner cannot honour would otherwise run # a different test than the annotation asked for. Reported up to the caller, # which owns the file's teardown, rather than exited from here (#1329). - bashunit::helper::annotations_validate "$script" || return 1 + if ! bashunit::helper::annotations_validate "$script"; then + # Recorded here rather than by the caller, which only has the exit status: + # this frame still holds the offending function and the reason (#1335). + bashunit::runner::report_annotation_abort "$script" + return 1 + fi local allow_test_parallel=true if [ "$_BASHUNIT_PROVIDER_MAP_NO_PARALLEL" = true ]; then diff --git a/src/runner/result.sh b/src/runner/result.sh index 900c7d40..654a6dd9 100644 --- a/src/runner/result.sh +++ b/src/runner/result.sh @@ -77,11 +77,15 @@ function bashunit::runner::parse_result_parallel() { # aggregator reads as a risky test, which would add a phantom risky test to every # file that defines the hook. # -# Arguments: $1 - the hook's exit status, $2 - the test file +# Arguments: $1 - the hook's exit status, $2 - the test file, +# $3 - result basename (optional, default: tear_down_after_script) ## function bashunit::runner::publish_file_hook_failure() { local status=$1 local test_file=$2 + # Named per source of failure, so a file that both aborts on an annotation and + # fails its teardown does not have one payload overwrite the other. + local slot=${3:-tear_down_after_script} [ "$status" -ne 0 ] || return 0 bashunit::parallel::is_enabled || return 0 @@ -101,7 +105,33 @@ function bashunit::runner::publish_file_hook_failure() { ##ASSERTIONS_INCOMPLETE=0\ ##ASSERTIONS_SNAPSHOT=0\ ##TEST_EXIT_CODE=$status##" - printf '%s\n' "$payload" >"$test_suite_dir/tear_down_after_script.result" + printf '%s\n' "$payload" >"$test_suite_dir/$slot.result" +} + +## +# Records a file the annotation validator rejected as one failed test. +# +# Two channels, because the report writers and the console summary read +# different ones: add_test_failed spools the named entry the parent replays, +# publish_file_hook_failure writes the counter payload the parent aggregates. +# Both belong in this frame -- under --parallel it is the worker, whose own +# counter dies with it, and recording either in the parent instead would land on +# top of the other and count the failure twice (#1301, #1335). +# +# No console line, unlike report_unusable_provider: the validator already printed +# the reason to stderr, and the parent replays a worker's stderr for the file, so +# printing here would report the same problem twice in two shapes. +# Arguments: $1 - the test file +## +function bashunit::runner::report_annotation_abort() { + local test_file=$1 + + local normalized_fn + normalized_fn="$(bashunit::helper::normalize_test_function_name \ + "$_BASHUNIT_ANNOT_REJECT_FN_OUT")" + bashunit::reports::add_test_failed \ + "$test_file" "$normalized_fn" 0 0 "$_BASHUNIT_ANNOT_REJECT_MSG_OUT" + bashunit::runner::publish_file_hook_failure 1 "$test_file" "annotation_abort" } function bashunit::runner::parse_result_sync() { diff --git a/tests/acceptance/bashunit_annotations_test.sh b/tests/acceptance/bashunit_annotations_test.sh index d0b50152..ac26383d 100644 --- a/tests/acceptance/bashunit_annotations_test.sh +++ b/tests/acceptance/bashunit_annotations_test.sh @@ -167,3 +167,52 @@ function test_a_malformed_retry_runs_the_file_teardown_under_parallel() { assert_contains "@retry 'abc'" "$BAD_ANNOTATION_OUTPUT" assert_file_not_exists "$BAD_ANNOTATION_MARKER" } + +function run_bad_annotation_alongside_a_passing_file() { # $1 = annotation line + local dir + dir="$(bashunit::temp_dir annotation_abort)" + BAD_ANNOTATION_MARKER="$dir/resource" + { + printf 'function set_up_before_script() { : >"$CLEANUP_MARKER"; }\n' + printf 'function tear_down_after_script() { rm -f "$CLEANUP_MARKER"; }\n' + printf '%s\n' "$1" + printf 'function test_bad_annotation_beside_a_good_file() { assert_same "ok" "ok"; }\n' + } >"$dir/bad_annotation_test.sh" + printf 'function test_good_beside_a_bad_annotation() { assert_same "ok" "ok"; }\n' \ + >"$dir/good_test.sh" + + BAD_ANNOTATION_EC=0 + BAD_ANNOTATION_OUTPUT=$(CLEANUP_MARKER="$BAD_ANNOTATION_MARKER" NO_COLOR=1 \ + ./bashunit --parallel --env "$TEST_ENV_FILE" "$dir" 2>&1) || BAD_ANNOTATION_EC=$? +} + +# Sequentially the abort exits the run. Under --parallel the parent is several +# files ahead, so the worker's abort has to travel to it: without that the run +# reported "All tests passed" and exited 0 over a file that never ran (#1335). +function test_a_malformed_timeout_fails_a_parallel_run_beside_a_passing_file() { + run_bad_annotation_alongside_a_passing_file '# @timeout abc' + + assert_general_error "" "" "$BAD_ANNOTATION_EC" + assert_contains "@timeout 'abc'" "$BAD_ANNOTATION_OUTPUT" + assert_not_contains "All tests passed" "$BAD_ANNOTATION_OUTPUT" + assert_file_not_exists "$BAD_ANNOTATION_MARKER" +} + +function test_a_malformed_retry_fails_a_parallel_run_beside_a_passing_file() { + run_bad_annotation_alongside_a_passing_file '# @retry abc' + + assert_general_error "" "" "$BAD_ANNOTATION_EC" + assert_contains "@retry 'abc'" "$BAD_ANNOTATION_OUTPUT" + assert_not_contains "All tests passed" "$BAD_ANNOTATION_OUTPUT" +} + +# #1301 fixed a file-level failure being counted twice: two failed tests in the +# reports against one in the console summary. The abort travels the same channel, +# so it has to stay counted once. +function test_a_malformed_annotation_is_counted_once_under_parallel() { + run_bad_annotation_alongside_a_passing_file '# @timeout abc' + + assert_contains "1 passed" "$BAD_ANNOTATION_OUTPUT" + assert_contains "1 failed" "$BAD_ANNOTATION_OUTPUT" + assert_contains "2 total" "$BAD_ANNOTATION_OUTPUT" +} From e9f512de0a73253ee93042b632e98b9432db9e69 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Fri, 21 Aug 2026 16:49:00 +0200 Subject: [PATCH 2/2] test(runner): pin the report half of the annotation abort count The console assertion covers only one of the two channels the count travels on. Recording the abort in the parent instead of the worker would leave the report at one failure and the console at two, which is the shape #1301 fixed, and nothing would have caught it. --log-junit has to follow --env: the env file blanks BASHUNIT_LOG_JUNIT as it is parsed, so a flag ahead of it is read and then overwritten. --- tests/acceptance/bashunit_annotations_test.sh | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/acceptance/bashunit_annotations_test.sh b/tests/acceptance/bashunit_annotations_test.sh index ac26383d..e0ad0295 100644 --- a/tests/acceptance/bashunit_annotations_test.sh +++ b/tests/acceptance/bashunit_annotations_test.sh @@ -181,9 +181,16 @@ function run_bad_annotation_alongside_a_passing_file() { # $1 = annotation line printf 'function test_good_beside_a_bad_annotation() { assert_same "ok" "ok"; }\n' \ >"$dir/good_test.sh" + # JUnit alongside the console run: the count has to match in both, and the + # report channel is the half a console-only assertion cannot see. The report + # goes outside $dir, which is the directory being scanned, and --log-junit has + # to follow --env: the env file blanks BASHUNIT_LOG_JUNIT as it is parsed, so a + # flag ahead of it is read and then overwritten. + BAD_ANNOTATION_JUNIT="$(bashunit::temp_file)" BAD_ANNOTATION_EC=0 BAD_ANNOTATION_OUTPUT=$(CLEANUP_MARKER="$BAD_ANNOTATION_MARKER" NO_COLOR=1 \ - ./bashunit --parallel --env "$TEST_ENV_FILE" "$dir" 2>&1) || BAD_ANNOTATION_EC=$? + ./bashunit --parallel --env "$TEST_ENV_FILE" \ + --log-junit "$BAD_ANNOTATION_JUNIT" "$dir" 2>&1) || BAD_ANNOTATION_EC=$? } # Sequentially the abort exits the run. Under --parallel the parent is several @@ -215,4 +222,14 @@ function test_a_malformed_annotation_is_counted_once_under_parallel() { assert_contains "1 passed" "$BAD_ANNOTATION_OUTPUT" assert_contains "1 failed" "$BAD_ANNOTATION_OUTPUT" assert_contains "2 total" "$BAD_ANNOTATION_OUTPUT" + + # The console summary and the reports read different channels, so agreeing here + # is the whole point: recording the abort in the parent instead of the worker + # would leave this at one failure and the console at two. + local junit + junit=$(cat "$BAD_ANNOTATION_JUNIT") + + assert_contains 'tests="2" failures="1"' "$junit" + assert_contains 'name="Bad annotation beside a good file"' "$junit" + assert_contains "is not a non-negative integer" "$junit" }