Skip to content

bug: ineffective set -e #135

Closed
Closed
bug: ineffective set -e#135

Description

@lczyk
lczyk
opened on Aug 22, 2025

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 ✓

Activity

  1. lczyk commented on Aug 22, 2025

    @lczyk
    lczyk
    on Aug 22, 2025
    ContributorAuthor

    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.

  2. pgrange commented on Aug 29, 2025

    @pgrange
    pgrange
    on Aug 29, 2025
    Member

    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.

  3. pgrange commented on Aug 29, 2025

    @pgrange
    pgrange
    on Aug 29, 2025
    Member

    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.

  4. lczyk commented on Sep 1, 2025

    @lczyk
    lczyk
    on Sep 1, 2025
    ContributorAuthor

    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

  5. lczyk commented on Sep 1, 2025

    @lczyk
    lczyk
    on Sep 1, 2025
    ContributorAuthor

    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.

  6. lczyk commented on Sep 1, 2025

    @lczyk
    lczyk
    on Sep 1, 2025
    ContributorAuthor

    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

  7. lczyk commented on Sep 1, 2025

    @lczyk
    lczyk
    on Sep 1, 2025
    ContributorAuthor

    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

  8. pgrange commented on Sep 3, 2025

    @pgrange
    pgrange
    on Sep 3, 2025
    Member

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

  9. lczyk commented on Sep 13, 2025

    @lczyk
    lczyk
    on Sep 13, 2025
    ContributorAuthor

    @pgrange #138 is up 👍

  10. pgrange commented on Sep 28, 2025

    @pgrange
    pgrange
    on Sep 28, 2025
    Member

    Fixed in #138 , Thanks to @lczyk

  11. pgrange
    closed this as completedon Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions