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..e0ad0295 100644 --- a/tests/acceptance/bashunit_annotations_test.sh +++ b/tests/acceptance/bashunit_annotations_test.sh @@ -167,3 +167,69 @@ 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" + + # 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" \ + --log-junit "$BAD_ANNOTATION_JUNIT" "$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" + + # 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" +}