Skip to content

Errors setup teardown still not caught correctly#513

Merged
Chemaclass merged 1 commit intomainfrom
bug/errors-setup-teardown-still-not-caught-correctly
Nov 23, 2025
Merged

Errors setup teardown still not caught correctly#513
Chemaclass merged 1 commit intomainfrom
bug/errors-setup-teardown-still-not-caught-correctly

Conversation

@Chemaclass
Copy link
Copy Markdown
Member

@Chemaclass Chemaclass commented Nov 22, 2025

📚 Description

Closes #512

Fixed lifecycle hooks not catching failing commands (non-zero exit codes) in set_up, tear_down, set_up_before_script, and tear_down_after_script.

Problem: Failing commands like git checkout non-existing-ref, ls /non/existing/path, or false were silently ignored in lifecycle hooks, allowing tests to run even when setup failed.

Root cause: Using if ! command; then status=$? - the $? inside the if block returns 0 (success of the conditional) instead of the command's actual exit code.

Solution: Changed to command || exit $? to capture the exit code immediately.

🔖 Changes

  • Fixed src/runner.sh:349 - Changed if ! runner::run_set_up to runner::run_set_up || exit $?
  • Added test coverage for all lifecycle hooks with failing commands
  • Updated CHANGELOG.md

The Real Fix

Both set_up and tear_down were broken for the same reason - they both call runner::execute_test_hook which was working correctly. The actual bug that affected BOTH was that runner::execute_test_hook wasn't catching failing commands.

But once runner::execute_test_hook returns the correct error code:

  • set_up needed the fix at line 349 to properly exit on error
  • tear_down was already correctly using || teardown_status=$? in the trap

So both are fixed now, but through different mechanisms:

  1. The common fix: runner::execute_test_hook now correctly detects all failing commands
  2. The set_up fix: || exit $? ensures we exit when setup fails
  3. The tear_down "fix": It was already correctly written with || teardown_status=$?

✅ To-do list

  • I updated the CHANGELOG.md to reflect the new feature or fix
  • I updated the documentation to reflect the changes

@Chemaclass Chemaclass added the bug Something isn't working label Nov 22, 2025
@Chemaclass Chemaclass self-assigned this Nov 22, 2025
@Chemaclass Chemaclass merged commit c063e7b into main Nov 23, 2025
18 checks passed
@Chemaclass Chemaclass deleted the bug/errors-setup-teardown-still-not-caught-correctly branch November 23, 2025 11:32
@carlfriedrich
Copy link
Copy Markdown
Contributor

@Chemaclass Thanks a lot for the super quick fix!

I can confirm that bashunit now correctly displays errors and does not execute the test function when the set_up function returns an error code. This is a big win! 👏

However, we still have to handle errors explicitly. The following set_up function will still silently pass:

function set_up() {
  false
  true
}

In order to detect the false statement, we have to:

function set_up() {
  false || return -1
  true
}

IMO this is not ideal, because it bears the risk of potential errors being overlooked and set_up functions becoming overloaded with error handling (which is usually not what you want in a test environment).

However, I am open for a discussion here, since at least it is possible now to detect errors in these functions. I think that we should detect any failing statement, though, in order to force robust and clean test implementations.

@Chemaclass
Copy link
Copy Markdown
Member Author

Chemaclass commented Nov 27, 2025

@carlfriedrich, thanks for the feedback, I implemented the fix here #515 | release 0.27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Errors in set_up and tear_down functions are still not caught correctly

3 participants