Repository navigation
bug: ineffective set -e #135
Description
Activity
i believe something like the following might be closer to what was intended
diff --git i/bash_unit w/bash_unit index 1afc3b7..2940b1d 100755 --- i/bash_unit +++ w/bash_unit @@ -218,7 +218,8 @@ run_test_suite() { if run_setup_suite then - run_tests || failure=$? + run_tests + failure=$? else failure=$? fi @@ -273,8 +274,16 @@ run_tests() { ( local status=0 declare -F | "$GREP" ' setup$' >/dev/null && setup - (__bash_unit_current_test__="$test" run_test) || status=$? - declare -F | "$GREP" ' teardown$' >/dev/null && teardown + # make sure teardown runs even if the test fails + local has_teardown=0 + # shellcheck disable=SC2034 # foo appears unused. Verify it or export it. + declare -F | "$GREP" ' teardown$' >/dev/null && has_teardown=1 + trap '((has_teardown)) && teardown' EXIT + + # NOTE: we do *not* want to use the || or && syntax with the subshell + # below because it would cause the set -e in run_test to be ignored + ( __bash_unit_current_test__="$test" run_test ) + status=$? exit $status ) failure=$(( $? || failure)) @@ -284,9 +293,18 @@ run_tests() { } run_test() { - set -e notify_test_starting "$__bash_unit_current_test__" - "$__bash_unit_current_test__" && notify_test_succeeded "$__bash_unit_current_test__" + ( + set -e + "$__bash_unit_current_test__" + ) + local status=$? + if (( $status != 0 )); then + # notify_test_failed "$__bash_unit_current_test__" + exit $status + else + notify_test_succeeded "$__bash_unit_current_test__" + fi } run_teardown_suite() {
will use a version with this patch for a while and test
EDIT: not quite. will tinker and report backEDIT: a couple more changes and now it works.
Hello @lczyk and thank you for opening this issue. I must admit that I'm struggling to figure out what's really happening with this
set -eand what the impact, as is, or when changed.I'm trying to find a test we can add to
test_core.shto exhibit the exact behavior we want.yeah, the
set -ebehavior is weird. have a look at the following example:#!/bin/bash # +e is the default, but let's be unambiguous set +e ( echo "subshell 1" set -e false echo "subshell 1 should not reach this point" ) [ $? -eq 0 ] || echo "subshell 1 failed" ( echo "subshell 2" set -e false echo "subshell 2 should not reach this point" ) || echo "subshell 2 failed" function f() { echo "function $1" set -e false echo "function $1 should not reach this point" } f 1 [ $? -eq 0 ] || echo "function 1 failed" f 2 || echo "function 2 failed"
which prints
subshell 1 subshell 1 failed subshell 2 subshell 2 should not reach this point function 1For explanation, see this post on unix stackexchange. tldr; subshell 2 is the unintuitive one. from man bash: "The shell does not exit if the command that fails is... part of any command executed in a && or || list". this includes subshells. whereas function 1 just runs in top-level scope and so it exits that immediately after its set -e
so my proposed patches just treat that case a bit more carefully, swapping some
||'s forlocal status=$?'s, and then usingtrap ... EXITto ensure the same behavior around teardown.these are the tests i've added to check for this:
diff --git i/tests/test_cli.sh w/tests/test_cli.sh index 3e237cc..b023a90 100644 --- i/tests/test_cli.sh +++ w/tests/test_cli.sh @@ -31,6 +31,27 @@ EOF )" } +test_exit_code_not_0_in_case_of_non_zero_exit_code() { + assert_fails "$BASH_UNIT <($CAT << EOF +function test_fails() { false ; } +EOF +)" +} + +test_exit_code_0_in_case_of_zero_exit_code() { + assert "$BASH_UNIT <($CAT << EOF +function test_succeeds() { true ; } +EOF +)" +} + +test_exit_code_not_0_in_case_of_non_zero_exit_code_followed_by_zero_exit_code() { + assert_fails "$BASH_UNIT <($CAT << EOF +function test_fails() { false ; true ; } +EOF +)" +} + test_run_all_file_parameters() { bash_unit_output=$($BASH_UNIT \ <(echo "test_one() { echo -n ; }") \
specifically the, artfully named,
test_exit_code_not_0_in_case_of_non_zero_exit_code_followed_by_zero_exit_codeposting things here as diffs for now, but if you see my case and agree with the general direction of changes i'm more than happy to just make it a PR. i just wanted to make sure that the behavior that i'm expecting from the tests (aka for the
test_exit_code_not_0_in_case_of_non_zero_exit_code_followed_by_zero_exit_codeabove to fail) is, in fact, the behavior whichbash_unitought to have. if so, i think my patches are warrantedSure, feel free to make a PR, I definitely see your case.
set -eon line 287 is, currently, ineffective, due to how set -x works in subshells.prints
because
run_testis run in a subshell (inrun_testson line 276).this also causes unexpected behavior when users try to use
set -ein tests.prints