diff --git a/CHANGELOG.md b/CHANGELOG.md index f07ed27d..737d345c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - Internal: Split `src/runner.sh` and `src/coverage.sh` into focused modules with no behavior change; see [ADR-010](adrs/adr-010-src-module-directories.md) (#924, #925) ### Fixed +- A failing test whose output quotes a shell-error phrase is no longer also reported as a runtime `Error`. The classifier scanned the whole capture -- which includes bashunit's own failure rendering -- for strings like `command not found`, so one cause was reported twice. It now requires the source-and-line prefix bash puts on a real diagnostic (#992) - `assert_have_been_called_times` and `assert_have_been_called_nth_with` report a usage error when their numeric argument is not a number, instead of leaking `[: my_cmd: integer expression expected` from inside bashunit. The common cause is swapping the count and the spy, which the message now names (#984) - `assert_false` no longer passes when the command does not exist. Exit code 127 is non-zero, so a typo in the command name satisfied the assertion while testing nothing; 127 and 126 now fail both `assert_true` and `assert_false`, because they mean the command never ran - `assert_within_delta` accepts a leading `+` on any operand (#979) diff --git a/src/assert/core.sh b/src/assert/core.sh index 71d25557..45eefa33 100755 --- a/src/assert/core.sh +++ b/src/assert/core.sh @@ -137,10 +137,12 @@ _BASHUNIT_ASSERT_EXIT_DESC_OUT="" # word rather than evaluated, and the failure used to point nowhere near the # cause. # -# The wording deliberately avoids the literal phrase "command not found". -# runner/diagnostics.sh classifies a test as a runtime error by scanning its -# output for that exact string, so using it here made every such failure report -# as both Failed and Error for the one cause. +# The wording avoided the literal phrase "command not found" because +# runner/diagnostics.sh used to classify a test as a runtime error by scanning +# its output for that exact string, which made every such failure report as both +# Failed and Error. That constraint is gone -- the classifier now requires a +# shell diagnostic's source-and-line prefix as well (#992). The phrasing stays +# as it is because it is already documented and released, not because it must. # Arguments: $1 - exit code, $2 - the command as written ## function bashunit::assert::_describe_exit_code() { diff --git a/src/runner/diagnostics.sh b/src/runner/diagnostics.sh index acf66b27..f79a8aab 100644 --- a/src/runner/diagnostics.sh +++ b/src/runner/diagnostics.sh @@ -75,20 +75,67 @@ $usage_after" return fi + # Conditions the shell reports without a source-and-line prefix, because they + # come from job control rather than the parser. Matched anywhere in the + # capture, as before. + case "$runtime_output" in + *"killed"* | *"segmentation fault"* | *"cannot allocate memory"*) + local runtime_error="${runtime_output#*: }" + _BASHUNIT_RUNNER_RUNTIME_ERROR_OUT="${runtime_error//$'\n'/}" + return + ;; + esac + + # Everything else is a diagnostic bash emits with its source and line + # ("file.sh: line 12: foo: command not found"). The cheap gate first: most + # failing tests contain none of these phrases and pay one glob. case "$runtime_output" in *"command not found"* | *"unbound variable"* | *"permission denied"* | \ *"no such file or directory"* | *"syntax error"* | *"bad substitution"* | \ - *"division by 0"* | *"cannot allocate memory"* | *"bad file descriptor"* | \ - *"segmentation fault"* | *"illegal option"* | *"argument list too long"* | \ - *"readonly variable"* | *"missing keyword"* | *"killed"* | \ + *"division by 0"* | *"bad file descriptor"* | \ + *"illegal option"* | *"argument list too long"* | \ + *"readonly variable"* | *"missing keyword"* | \ *"cannot execute binary file"* | *"invalid arithmetic operator"* | \ *"ambiguous redirect"* | *"integer expression expected"* | \ *"too many arguments"* | *"value too great"* | \ - *"not a valid identifier"* | *"unexpected EOF"*) - local runtime_error="${runtime_output#*: }" - _BASHUNIT_RUNNER_RUNTIME_ERROR_OUT="${runtime_error//$'\n'/}" - ;; + *"not a valid identifier"* | *"unexpected EOF"*) ;; + *) return ;; esac + + # A phrase alone is not enough. The capture also carries bashunit's own + # rendering of the failure, and a test whose subject is error handling will + # legitimately quote one of these strings as data -- both used to be misread + # as runtime errors and reported twice, as Failed and as Error, for one cause. + # Requiring the prefix on the same line separates what the shell said from what + # we said about it; bashunit's own output never carries it. + local line + while IFS= read -r line; do + case "$line" in + *": line "[0-9]*": "*) ;; + *) continue ;; + esac + + case "$line" in + *"command not found"* | *"unbound variable"* | *"permission denied"* | \ + *"no such file or directory"* | *"syntax error"* | *"bad substitution"* | \ + *"division by 0"* | *"bad file descriptor"* | \ + *"illegal option"* | *"argument list too long"* | \ + *"readonly variable"* | *"missing keyword"* | \ + *"cannot execute binary file"* | *"invalid arithmetic operator"* | \ + *"ambiguous redirect"* | *"integer expression expected"* | \ + *"too many arguments"* | *"value too great"* | \ + *"not a valid identifier"* | *"unexpected EOF"*) + # Extract from the whole capture, not the matched line: the message shape + # (leading source stripped, newlines removed) is pinned by + # tests/unit/runner/diagnostics_test.sh. + local runtime_error="${runtime_output#*: }" + _BASHUNIT_RUNNER_RUNTIME_ERROR_OUT="${runtime_error//$'\n'/}" + return + ;; + esac + done <&1) || exit_code=$? + + assert_same 1 "$exit_code" + assert_contains "✗ Failed: Quotes a shell diagnostic as data" "$output" + assert_not_contains "✗ Error: Quotes a shell diagnostic as data" "$output" +} + +# The regression guard: a test that really does hit a shell error must still be +# classified as one. +# +# Pinned to LC_ALL=C on purpose. bash translates its diagnostics, and the phrase +# list this classifier matches on is English only, so under a Spanish or +# Japanese locale a genuine "command not found" is not recognised at all. That +# is a pre-existing limitation, not something this test should assert about; +# forcing the C locale keeps it testing the classification mechanics. +function test_a_real_shell_error_is_still_reported() { + local fixture=tests/acceptance/fixtures/runtime_error/real_error.sh + local output exit_code=0 + + output=$(LC_ALL=C NO_COLOR=1 ./bashunit --no-parallel --skip-env-file "$fixture" 2>&1) || exit_code=$? + + assert_same 1 "$exit_code" + assert_contains "✗ Error: Hits a real shell error" "$output" + assert_contains "command not found" "$output" +} diff --git a/tests/acceptance/fixtures/runtime_error/quotes_a_diagnostic.sh b/tests/acceptance/fixtures/runtime_error/quotes_a_diagnostic.sh new file mode 100644 index 00000000..0f4b8b44 --- /dev/null +++ b/tests/acceptance/fixtures/runtime_error/quotes_a_diagnostic.sh @@ -0,0 +1,10 @@ +#!/usr/bin/env bash + +# A test whose subject is error handling: its output legitimately contains the +# text of a shell diagnostic, as data. The test fails on its assertion, but +# nothing went wrong at runtime -- it must not also be reported as an Error. +function test_quotes_a_shell_diagnostic_as_data() { + local captured="bash: some_tool: command not found" + + assert_same "expected something else" "$captured" +} diff --git a/tests/acceptance/fixtures/runtime_error/real_error.sh b/tests/acceptance/fixtures/runtime_error/real_error.sh new file mode 100644 index 00000000..0361872b --- /dev/null +++ b/tests/acceptance/fixtures/runtime_error/real_error.sh @@ -0,0 +1,6 @@ +#!/usr/bin/env bash + +function test_hits_a_real_shell_error() { + definitely_not_a_real_command_xyz + assert_same 1 1 +}