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