diff --git a/CHANGELOG.md b/CHANGELOG.md index fb5bb696..2fdb66b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - The `--parallel` unsupported-OS warning no longer claims Alpine is excluded: Alpine has been a supported parallel platform since the race conditions were fixed, the message was simply never updated ### Fixed +- A missing `--env`/`--boot` bootstrap file is now an error instead of a green run that tested nothing: the file used to be sourced unchecked, so a typo'd path leaked a raw `No such file or directory`, skipped the entire suite and still exited `0`. The report options (`--report-json`, `--log-junit`, `--report-tap`, `--report-html`, `--log-gha`) are likewise checked before the run instead of failing silently after it — an unwritable destination produced a passing run, no report on disk, and exit `0`. `--seed` now validates as a non-negative integer like the other numeric options (#875) - The numeric options (`--jobs`, `--retry`, `--test-timeout`, `--coverage-min`) and `--output` now reject an invalid value with a clear error and a non-zero exit, instead of dropping the setting. `--jobs abc` was the worst case: `[ n -lt abc ]` errors rather than returning false, so the Bash 3.x job-slot poll never reached its `break` and the run **hung**, while Bash 4.3+ silently ignored the concurrency limit. `--coverage-min abc` leaked a raw `[: abc: integer expression expected` and still exited `0`, so a CI job never enforced its threshold. Validation runs on the resolved configuration, so it covers the `BASHUNIT_*` environment variables too (#873) - An unknown option is now rejected with a clear error and a non-zero exit instead of being silently filed under test paths. A mistyped flag used to degrade the run while still reporting success: `bashunit --parralel tests/` ran sequentially and exited `0`, and `bashunit --filterr foo tests/` swallowed both the flag and its value and ran the whole suite. Applies to `test` and `bench`; the `assert` subcommand parses its own arguments and is unchanged (#871) - The quickstart's sample output showed durations as `16 ms` / `90 ms`; bashunit prints `16ms` / `90ms` diff --git a/src/main.sh b/src/main.sh index b0f570d7..e005b260 100644 --- a/src/main.sh +++ b/src/main.sh @@ -24,6 +24,30 @@ function bashunit::main::require_non_negative_int_or_exit() { esac } +## +# Exits non-zero unless the path can be written: either it already exists and is +# writable, or its parent directory exists and is writable. Reports are produced +# after the suite has finished, so an unwritable destination used to surface as a +# raw redirect error on a run that had already reported success (#875). +# Arguments: $1 - path, $2 - the setting name to quote in the error +## +function bashunit::main::require_writable_path_or_exit() { + local path=$1 + local parent=${1%/*} + [ "$parent" = "$1" ] && parent="." + [ -z "$parent" ] && parent="/" + + if [ -e "$path" ]; then + [ -w "$path" ] && return 0 + elif [ -d "$parent" ] && [ -w "$parent" ]; then + return 0 + fi + + printf "%sError: %s cannot be written: '%s'.%s\n" \ + "${_BASHUNIT_COLOR_FAILED}" "$2" "$path" "${_BASHUNIT_COLOR_DEFAULT}" >&2 + exit 1 +} + ## # Validates the resolved configuration and exits non-zero on a bad value. # Runs after flag parsing so it covers both the flags and the BASHUNIT_* env @@ -46,6 +70,19 @@ function bashunit::main::validate_config_or_exit() { "${BASHUNIT_COVERAGE_MIN}" "BASHUNIT_COVERAGE_MIN (--coverage-min)" fi + if [ -n "${BASHUNIT_SEED:-}" ]; then + bashunit::main::require_non_negative_int_or_exit "${BASHUNIT_SEED}" "BASHUNIT_SEED (--seed)" + fi + + local _report_var _report_path + for _report_var in BASHUNIT_LOG_JUNIT BASHUNIT_LOG_GHA BASHUNIT_REPORT_HTML \ + BASHUNIT_REPORT_TAP BASHUNIT_REPORT_JSON; do + _report_path=${!_report_var:-} + if [ -n "$_report_path" ]; then + bashunit::main::require_writable_path_or_exit "$_report_path" "$_report_var" + fi + done + # Only TAP is implemented; an unrecognised name used to fall back to the # default renderer without a word, so `--output tpa` looked like it worked. case "${BASHUNIT_OUTPUT_FORMAT:-}" in @@ -235,6 +272,13 @@ function bashunit::main::cmd_test() { BASHUNIT_BOOTSTRAP_ARGS="$boot_args" export -n BASHUNIT_BOOTSTRAP_ARGS fi + # A missing bootstrap used to leak a raw `source` error, abort the rest of + # the parse and exit 0 with no tests run at all (#875). + if [ ! -f "$boot_file" ] || [ ! -r "$boot_file" ]; then + printf "%sError: cannot read the bootstrap file: '%s'.%s\n" \ + "${_BASHUNIT_COLOR_FAILED}" "$boot_file" "${_BASHUNIT_COLOR_DEFAULT}" >&2 + exit 1 + fi # Export all variables from the env file so they're available in subshells # (e.g., process substitution used in load_test_files) set -o allexport @@ -568,6 +612,13 @@ function bashunit::main::cmd_bench() { BASHUNIT_BOOTSTRAP_ARGS="$boot_args" export -n BASHUNIT_BOOTSTRAP_ARGS fi + # A missing bootstrap used to leak a raw `source` error, abort the rest of + # the parse and exit 0 with no tests run at all (#875). + if [ ! -f "$boot_file" ] || [ ! -r "$boot_file" ]; then + printf "%sError: cannot read the bootstrap file: '%s'.%s\n" \ + "${_BASHUNIT_COLOR_FAILED}" "$boot_file" "${_BASHUNIT_COLOR_DEFAULT}" >&2 + exit 1 + fi # Export all variables from the env file so they're available in subshells # (e.g., process substitution used in load_test_files) set -o allexport diff --git a/tests/acceptance/bashunit_invalid_path_test.sh b/tests/acceptance/bashunit_invalid_path_test.sh new file mode 100644 index 00000000..6889add0 --- /dev/null +++ b/tests/acceptance/bashunit_invalid_path_test.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +set -euo pipefail + +function set_up_before_script() { + TEST_ENV_FILE="tests/acceptance/fixtures/.env.default" + TEST_FILE="tests/acceptance/fixtures/tests_path/a_test.sh" +} + +# A missing bootstrap used to leak a raw `source` error, run no tests at all and +# still exit 0 -- a green build that tested nothing (#875). +function test_bashunit_fails_when_the_bootstrap_file_is_missing() { + local ec=0 + local output + output=$(./bashunit --env definitely_missing_bootstrap.sh "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "definitely_missing_bootstrap.sh" "$output" +} + +function test_bashunit_fails_when_the_bootstrap_file_is_missing_via_boot_alias() { + local ec=0 + local output + output=$(./bashunit --boot definitely_missing_bootstrap.sh "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" +} + +# The report was silently not written while the suite still reported success, so +# a CI job uploaded an empty artifact from a green build. +function test_bashunit_fails_when_a_report_path_is_not_writable() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --report-json /nope/dir/out.json "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "/nope/dir/out.json" "$output" +} + +function test_bashunit_fails_when_a_junit_report_path_is_not_writable() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --log-junit /nope/dir/out.xml "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" +} + +function test_bashunit_fails_when_seed_is_not_a_number() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --seed abc "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "--seed" "$output" +} + +function test_bashunit_still_accepts_an_existing_bootstrap_file() { + local boot_file + boot_file="$(bashunit::temp_file)" + printf 'export BASHUNIT_TEST_BOOTSTRAP_MARKER=loaded\n' >"$boot_file" + + local ec=0 + local output + output=$(./bashunit --env "$boot_file" "$TEST_FILE" 2>&1) || ec=$? + + assert_successful_code "" "" "$ec" + assert_contains "All tests passed" "$output" +} + +function test_bashunit_still_writes_a_report_to_a_writable_path() { + local report_dir + report_dir="$(bashunit::temp_dir)" + + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --report-json "$report_dir/out.json" "$TEST_FILE" 2>&1) || ec=$? + + assert_successful_code "" "" "$ec" + assert_file_exists "$report_dir/out.json" +}