bug: ineffective set -e

[lczyk]

set -e on line 287 is, currently, ineffective, due to how set -x works in subshells.

git clone https://github.com/bash-unit/bash_unit && cd bash_unit
git apply - << 'EOF'
diff --git i/bash_unit w/bash_unit
index 1afc3b7..5b1faed 100755
--- i/bash_unit
+++ w/bash_unit
@@ -287,6 +287,8 @@ run_test() {
 set -e
 notify_test_starting "$__bash_unit_current_test__"
 "$__bash_unit_current_test__" && notify_test_succeeded "$__bash_unit_current_test__"
+ false
+ echo "THIS SHOULD NOT BE PRINTED"
 }
 
 run_teardown_suite() {

EOF
./bash_unit -p test_assert_fails_succeeds ./tests/test_core.sh # pick just one test

prints

Running tests in ./tests/test_core.sh
 Running test_assert_fails_succeeds ... SUCCESS ✓ 
THIS SHOULD NOT BE PRINTED
Overall result: SUCCESS ✓ 

because run_test is run in a subshell (in run_tests on line 276).

this also causes unexpected behavior when users try to use set -e in tests.

git clone https://github.com/bash-unit/bash_unit && cd bash_unit
git apply - << 'EOF'
diff --git i/tests/test_core.sh w/tests/test_core.sh
index cc3bd27..dce75d7 100644
--- i/tests/test_core.sh
+++ w/tests/test_core.sh
@@ -15,6 +15,8 @@ test_fail_fails() {
 
 test_assert_fails_succeeds() {
 (assert_fails false) || fail 'assert_fails should succeed'
+ false
+ echo "THIS SHOULD NOT BE PRINTED"
 }
 
 test_assert_fails_fails() {

EOF
./bash_unit -p test_assert_fails_succeeds ./tests/test_core.sh # pick just one test

prints

Running tests in ./tests/test_core.sh
 Running test_assert_fails_succeeds ... THIS SHOULD NOT BE PRINTED
SUCCESS ✓ 
Overall result: SUCCESS ✓ 

[lczyk]

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 back

EDIT: a couple more changes and now it works.

[pgrange]

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 -e and what the impact, as is, or when changed.

I'm trying to find a test we can add to test_core.sh to exhibit the exact behavior we want.

[pgrange]

Related to this issue, I've noticed that, when setup_suite fails, bash_unit terminates in success and that's not cool.

So I fixed that simple case and added a test for that.

[lczyk]

yeah, the set -e behavior 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 1

For 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

[lczyk]

so my proposed patches just treat that case a bit more carefully, swapping some ||'s for local status=$?'s, and then using trap ... EXIT to ensure the same behavior around teardown.

[lczyk]

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_code

[lczyk]

posting 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_code above to fail) is, in fact, the behavior which bash_unit ought to have. if so, i think my patches are warranted

[pgrange]

Sure, feel free to make a PR, I definitely see your case.

[lczyk]

@pgrange #138 is up 👍

[pgrange]

Fixed in #138 , Thanks to @lczyk