From 6dbbc6bad1245075a7771c8ae7a67f62d025667e Mon Sep 17 00:00:00 2001 From: Martin Schulze Date: Sat, 16 Jul 2022 12:41:21 +0200 Subject: [PATCH 1/4] Fix unset variables --- lib/bats-core/common.bash | 4 ++-- lib/bats-core/tracing.bash | 24 +++++++++++++++++-- libexec/bats-core/bats | 9 +++---- libexec/bats-core/bats-exec-suite | 7 +++--- libexec/bats-core/bats-exec-test | 21 ---------------- libexec/bats-core/bats-preprocess | 2 +- test/bats.bats | 24 +++++++++---------- test/file_setup_teardown.bats | 2 +- test/fixtures/bats/environment.bats | 2 +- test/fixtures/bats/retry.bats | 2 +- .../teardown_file_does_not_leak2.bats | 2 +- test/fixtures/load/bats_load_library.bats | 2 +- test/fixtures/load/load.bats | 2 +- .../must_not_parallelize_within_file.bats | 8 +++---- test/fixtures/suite/multiple/b.bats | 2 +- test/formatter.bats | 2 +- 16 files changed, 58 insertions(+), 57 deletions(-) diff --git a/lib/bats-core/common.bash b/lib/bats-core/common.bash index fa45be0a98..0acbf959f0 100644 --- a/lib/bats-core/common.bash +++ b/lib/bats-core/common.bash @@ -65,13 +65,13 @@ bats_require_minimum_version() { # } bats_binary_search() { # - local -r search_value=$1 array_name=$2 - if [[ $# -ne 2 ]]; then printf "ERROR: bats_binary_search requires exactly 2 arguments: \n" >&2 return 2 fi + local -r search_value=$1 array_name=$2 + # we'd like to test if array is set but we cannot distinguish unset from empty arrays, so we need to skip that local start=0 mid end mid_value diff --git a/lib/bats-core/tracing.bash b/lib/bats-core/tracing.bash index 603d108d4d..39f083ca2a 100644 --- a/lib/bats-core/tracing.bash +++ b/lib/bats-core/tracing.bash @@ -30,7 +30,7 @@ bats_capture_stack_trace() { bats_get_failure_stack_trace() { local stack_trace_var # See bats_debug_trap for details. - if [[ -n "${BATS_DEBUG_LAST_STACK_TRACE_IS_VALID}" ]]; then + if [[ -n "${BATS_DEBUG_LAST_STACK_TRACE_IS_VALID:-}" ]]; then stack_trace_var=BATS_DEBUG_LAST_STACK_TRACE else stack_trace_var=BATS_DEBUG_LASTLAST_STACK_TRACE @@ -275,7 +275,7 @@ bats_debug_trap() { # isn't set properly during `teardown()` errors. bats_check_status_from_trap() { local status="$?" - if [[ -z "$BATS_TEST_COMPLETED" ]]; then + if [[ -z "${BATS_TEST_COMPLETED:-}" ]]; then BATS_ERROR_STATUS="${BATS_ERROR_STATUS:-$status}" if [[ "$BATS_ERROR_STATUS" -eq 0 ]]; then BATS_ERROR_STATUS=1 @@ -299,6 +299,26 @@ bats_add_debug_exclude_path() { # } bats_setup_tracing() { + # Variables for capturing accurate stack traces. See bats_debug_trap for + # details. + # + # BATS_DEBUG_LAST_LINENO, BATS_DEBUG_LAST_SOURCE, and + # BATS_DEBUG_LAST_STACK_TRACE hold data from the most recent call to + # bats_debug_trap. + # + # BATS_DEBUG_LASTLAST_STACK_TRACE holds data from two bats_debug_trap calls + # ago. + # + # BATS_DEBUG_LAST_STACK_TRACE_IS_VALID indicates that + # BATS_DEBUG_LAST_STACK_TRACE contains the stack trace of the test's error. If + # unset, BATS_DEBUG_LAST_STACK_TRACE is unreliable and + # BATS_DEBUG_LASTLAST_STACK_TRACE should be used instead. + BATS_DEBUG_LASTLAST_STACK_TRACE=() + BATS_DEBUG_LAST_LINENO=() + BATS_DEBUG_LAST_SOURCE=() + BATS_DEBUG_LAST_STACK_TRACE=() + BATS_DEBUG_LAST_STACK_TRACE_IS_VALID= + BATS_ERROR_SUFFIX= BATS_DEBUG_EXCLUDE_PATHS=() # exclude some paths by default bats_add_debug_exclude_path "$BATS_ROOT/lib/" diff --git a/libexec/bats-core/bats b/libexec/bats-core/bats index 77b81684b5..372209046a 100755 --- a/libexec/bats-core/bats +++ b/libexec/bats-core/bats @@ -282,13 +282,14 @@ export BATS_WARNING_FILE="${BATS_RUN_TMPDIR}/warnings.log" bats_exit_trap() { if [[ -s "$BATS_WARNING_FILE" ]]; then + local pre_cat='' post_cat='' if [[ $formatter == pretty ]]; then - PRE_CAT=$'\x1B[31m' - POST_CAT=$'\x1B[0m' + pre_cat=$'\x1B[31m' + post_cat=$'\x1B[0m' fi - printf "\nThe following warnings were encountered during tests:\n%s" "$PRE_CAT" + printf "\nThe following warnings were encountered during tests:\n%s" "$pre_cat" cat "$BATS_WARNING_FILE" - printf "%s" "$POST_CAT" + printf "%s" "$post_cat" fi >&2 if [[ -n "$BATS_TEMPDIR_CLEANUP" ]]; then diff --git a/libexec/bats-core/bats-exec-suite b/libexec/bats-core/bats-exec-suite index 7c87657781..e23c30eaa2 100755 --- a/libexec/bats-core/bats-exec-suite +++ b/libexec/bats-core/bats-exec-suite @@ -124,7 +124,8 @@ bats_gather_tests() { test_count="${#all_tests[@]}" } -TEST_ROOT=${1%/*} +TEST_ROOT=${1-} +TEST_ROOT=${TEST_ROOT%/*} BATS_RUN_LOGS_DIRECTORY="$TEST_ROOT/.bats/run-logs" if [[ ! -d "$BATS_RUN_LOGS_DIRECTORY" ]]; then if [[ -n "$filter_status" ]]; then @@ -218,7 +219,7 @@ if [[ -n "$filter_status" ]]; then done >> "$BATS_RUNLOG_FILE" test_count="${#filtered_tests[@]}" - if [[ ${#failed_tests} -eq 0 && ${#filtered_tests[@]} -eq 0 ]]; then + if [[ ${#failed_tests[@]} -eq 0 && ${#filtered_tests[@]} -eq 0 ]]; then printf "There where no failed tests in the last recorded run.\n" >&2 fi else @@ -287,7 +288,7 @@ bats_suite_exit_trap() { printf "\n# Received SIGINT, aborting ...\n\n" fi - if [[ -d "$BATS_RUN_LOGS_DIRECTORY" && -n "${BATS_INTERRUPTED}" ]]; then + if [[ -d "$BATS_RUN_LOGS_DIRECTORY" && -n "${BATS_INTERRUPTED:-}" ]]; then # aborting a test run with CTRL+C does not save the runlog file rm "$BATS_RUNLOG_FILE" fi diff --git a/libexec/bats-core/bats-exec-test b/libexec/bats-core/bats-exec-test index b733757003..9fbf9ac20b 100755 --- a/libexec/bats-core/bats-exec-test +++ b/libexec/bats-core/bats-exec-test @@ -202,31 +202,10 @@ bats_perform_test() { exit 1 fi - # Variables for capturing accurate stack traces. See bats_debug_trap for - # details. - # - # BATS_DEBUG_LAST_LINENO, BATS_DEBUG_LAST_SOURCE, and - # BATS_DEBUG_LAST_STACK_TRACE hold data from the most recent call to - # bats_debug_trap. - # - # BATS_DEBUG_LASTLAST_STACK_TRACE holds data from two bats_debug_trap calls - # ago. - # - # BATS_DEBUG_LAST_STACK_TRACE_IS_VALID indicates that - # BATS_DEBUG_LAST_STACK_TRACE contains the stack trace of the test's error. If - # unset, BATS_DEBUG_LAST_STACK_TRACE is unreliable and - # BATS_DEBUG_LASTLAST_STACK_TRACE should be used instead. - BATS_DEBUG_LASTLAST_STACK_TRACE=() - BATS_DEBUG_LAST_LINENO=() - BATS_DEBUG_LAST_SOURCE=() - BATS_DEBUG_LAST_STACK_TRACE=() - BATS_DEBUG_LAST_STACK_TRACE_IS_VALID= - BATS_TEST_COMPLETED= BATS_TEST_SKIPPED= BATS_TEARDOWN_COMPLETED= BATS_ERROR_STATUS= - BATS_ERROR_SUFFIX= bats_setup_tracing # mark this call as trap call trap 'bats_teardown_trap as-exit-trap' EXIT diff --git a/libexec/bats-core/bats-preprocess b/libexec/bats-core/bats-preprocess index 0391298f4b..93098d3e7f 100755 --- a/libexec/bats-core/bats-preprocess +++ b/libexec/bats-core/bats-preprocess @@ -42,7 +42,7 @@ tests=() if [[ "$line" =~ $BATS_TEST_PATTERN ]] || [[ "$line" =~ $BATS_TEST_PATTERN_COMMENT ]]; then name="${BASH_REMATCH[1]#[\'\"]}" name="${name%[\'\"]}" - body="${BASH_REMATCH[2]}" + body="${BASH_REMATCH[2]:-}" bats_encode_test_name "$name" 'encoded_name' printf '%s() { bats_test_begin "%s"; %s\n' "${encoded_name:?}" "$name" "$body" || : diff --git a/test/bats.bats b/test/bats.bats index 774cc40369..35f51868d6 100755 --- a/test/bats.bats +++ b/test/bats.bats @@ -326,8 +326,8 @@ setup() { @test 'ensure compatibility with unofficial Bash strict mode' { local expected='ok 1 unofficial Bash strict mode conditions met' - if [[ -n "$BATS_NUMBER_OF_PARALLEL_JOBS" ]]; then - if [[ -z "$BATS_NO_PARALLELIZE_ACROSS_FILES" ]]; then + if [[ -n "${BATS_NUMBER_OF_PARALLEL_JOBS:-}" ]]; then + if [[ -z "${BATS_NO_PARALLELIZE_ACROSS_FILES:-}" ]]; then type -p parallel &>/dev/null || skip "Don't check file parallelized without GNU parallel" fi (type -p flock &>/dev/null || type -p shlock &>/dev/null) || skip "Don't check parallelized without flock/shlock " @@ -340,8 +340,8 @@ setup() { run env - \ "PATH=$PATH" \ "HOME=$HOME" \ - "BATS_NO_PARALLELIZE_ACROSS_FILES=$BATS_NO_PARALLELIZE_ACROSS_FILES" \ - "BATS_NUMBER_OF_PARALLEL_JOBS=$BATS_NUMBER_OF_PARALLEL_JOBS" \ + "BATS_NO_PARALLELIZE_ACROSS_FILES=${BATS_NO_PARALLELIZE_ACROSS_FILES:-}" \ + "BATS_NUMBER_OF_PARALLEL_JOBS=${BATS_NUMBER_OF_PARALLEL_JOBS:-}" \ SHELLOPTS=nounset \ "${BATS_ROOT}/bin/bats" "$FIXTURE_ROOT/unofficial_bash_strict_mode.bats" if [[ "$status" -ne 0 || "${lines[1]}" != "$expected" ]]; then @@ -602,7 +602,7 @@ END_OF_ERR_MSG } @test "Don't hang on CTRL-C (issue #353)" { - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi load 'concurrent-coordination' @@ -731,7 +731,7 @@ END_OF_ERR_MSG } @test "CTRL-C aborts and fails the current test" { - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi @@ -767,7 +767,7 @@ END_OF_ERR_MSG } @test "CTRL-C aborts and fails the current run" { - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi @@ -804,7 +804,7 @@ END_OF_ERR_MSG @test "CTRL-C aborts and fails after run" { # shellcheck disable=SC2034 BATS_TEST_RETRIES=2 - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi @@ -839,7 +839,7 @@ END_OF_ERR_MSG } @test "CTRL-C aborts and fails the current teardown" { - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi @@ -875,7 +875,7 @@ END_OF_ERR_MSG } @test "CTRL-C aborts and fails the current setup_file" { - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi @@ -911,7 +911,7 @@ END_OF_ERR_MSG } @test "CTRL-C aborts and fails the current teardown_file" { - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi # shellcheck disable=SC2031 @@ -1338,7 +1338,7 @@ enforce_own_process_group() { } @test "--filter-status failed does not update list when run is aborted" { - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi diff --git a/test/file_setup_teardown.bats b/test/file_setup_teardown.bats index 938f1a5112..4a212dd1f4 100644 --- a/test/file_setup_teardown.bats +++ b/test/file_setup_teardown.bats @@ -96,7 +96,7 @@ not ok 1 failing test } @test "teardown_file should run even after user abort via CTRL-C" { - if [[ "$BATS_NUMBER_OF_PARALLEL_JOBS" -gt 1 ]]; then + if [[ "${BATS_NUMBER_OF_PARALLEL_JOBS:-1}" -gt 1 ]]; then skip "Aborts don't work in parallel mode" fi # shellcheck disable=SC2031,SC2030 diff --git a/test/fixtures/bats/environment.bats b/test/fixtures/bats/environment.bats index f643e1bd16..0d045da81b 100644 --- a/test/fixtures/bats/environment.bats +++ b/test/fixtures/bats/environment.bats @@ -6,5 +6,5 @@ @test "variables do not persist across tests" { # shellcheck disable=SC2031 - [ -z "$variable" ] + [ -z "${variable:-}" ] } diff --git a/test/fixtures/bats/retry.bats b/test/fixtures/bats/retry.bats index 1981e1d59b..fe8d47d44b 100644 --- a/test/fixtures/bats/retry.bats +++ b/test/fixtures/bats/retry.bats @@ -1,7 +1,7 @@ BATS_TEST_RETRIES=2 # means three tries per test log_caller() { - printf "%s %s %s\n" "$BATS_TEST_NAME" "${FUNCNAME[1]}" "$BATS_TEST_TRY_NUMBER" >> "${LOG?}" + printf "%s %s %s\n" "${BATS_TEST_NAME:-}" "${FUNCNAME[1]}" "${BATS_TEST_TRY_NUMBER:-}" >> "${LOG?}" } setup_file() { diff --git a/test/fixtures/file_setup_teardown/teardown_file_does_not_leak2.bats b/test/fixtures/file_setup_teardown/teardown_file_does_not_leak2.bats index 798bc8cc1f..ed85ac934a 100644 --- a/test/fixtures/file_setup_teardown/teardown_file_does_not_leak2.bats +++ b/test/fixtures/file_setup_teardown/teardown_file_does_not_leak2.bats @@ -1,3 +1,3 @@ @test "must not see variable from first run" { - [[ -z "$POTENTIALLY_LEAKING_VARIABLE" ]] + [[ -z "${POTENTIALLY_LEAKING_VARIABLE:-}" ]] } \ No newline at end of file diff --git a/test/fixtures/load/bats_load_library.bats b/test/fixtures/load/bats_load_library.bats index 5f9b566915..69bd1f723e 100644 --- a/test/fixtures/load/bats_load_library.bats +++ b/test/fixtures/load/bats_load_library.bats @@ -1,4 +1,4 @@ -[ -n "$HELPER_NAME" ] || HELPER_NAME="test_helper" +[ -n "${HELPER_NAME:-}" ] || HELPER_NAME="test_helper" bats_load_library "$HELPER_NAME" @test "calling a loaded helper" { diff --git a/test/fixtures/load/load.bats b/test/fixtures/load/load.bats index 975b6b8c1b..ba656c68bc 100644 --- a/test/fixtures/load/load.bats +++ b/test/fixtures/load/load.bats @@ -1,4 +1,4 @@ -[ -n "$HELPER_NAME" ] || HELPER_NAME="test_helper" +[ -n "${HELPER_NAME:-}" ] || HELPER_NAME="test_helper" load "$HELPER_NAME" @test "calling a loaded helper" { diff --git a/test/fixtures/parallel/must_not_parallelize_within_file.bats b/test/fixtures/parallel/must_not_parallelize_within_file.bats index 15aeffb430..22f5e2d5c9 100644 --- a/test/fixtures/parallel/must_not_parallelize_within_file.bats +++ b/test/fixtures/parallel/must_not_parallelize_within_file.bats @@ -1,13 +1,13 @@ setup_file() { export FILE_MARKER FILE_MARKER=$(mktemp "${BATS_RUN_TMPDIR}/file_marker.XXXXXX") - if [[ -n "${DISABLE_IN_SETUP_FILE_FUNCTION}" ]]; then + if [[ -n "${DISABLE_IN_SETUP_FILE_FUNCTION:-}" ]]; then export BATS_NO_PARALLELIZE_WITHIN_FILE=true echo "setup_file() sets BATS_NO_PARALLELIZE_WITHIN_FILE=true" >&2 fi } -if [[ -n "${DISABLE_OUTSIDE_ALL_FUNCTIONS}" ]]; then +if [[ -n "${DISABLE_OUTSIDE_ALL_FUNCTIONS:-}" ]]; then export BATS_NO_PARALLELIZE_WITHIN_FILE=true echo "File sets BATS_NO_PARALLELIZE_WITHIN_FILE=true" >&2 fi @@ -17,7 +17,7 @@ teardown_file() { } setup() { - if [[ -n "${DISABLE_IN_SETUP_FUNCTION}" ]]; then + if [[ -n "${DISABLE_IN_SETUP_FUNCTION:-}" ]]; then export BATS_NO_PARALLELIZE_WITHIN_FILE=true echo "setup() sets BATS_NO_PARALLELIZE_WITHIN_FILE=true" >&3 fi @@ -29,7 +29,7 @@ teardown() { } @test "test 1" { - if [[ -n "${DISABLE_IN_TEST_FUNCTION}" ]]; then + if [[ -n "${DISABLE_IN_TEST_FUNCTION:-}" ]]; then export BATS_NO_PARALLELIZE_WITHIN_FILE=true echo "Test function sets BATS_NO_PARALLELIZE_WITHIN_FILE=true" >&3 fi diff --git a/test/fixtures/suite/multiple/b.bats b/test/fixtures/suite/multiple/b.bats index bb965a4f86..327ec8dfe5 100644 --- a/test/fixtures/suite/multiple/b.bats +++ b/test/fixtures/suite/multiple/b.bats @@ -3,5 +3,5 @@ } @test "quasi-truth" { - [ -z "$FLUNK" ] + [ -z "${FLUNK:-}" ] } diff --git a/test/formatter.bats b/test/formatter.bats index 0e111ecec9..77c4ecd498 100644 --- a/test/formatter.bats +++ b/test/formatter.bats @@ -64,7 +64,7 @@ setup() { # the replay should be possible without errors bash -u "$formatter" >/dev/null < Date: Wed, 20 Jul 2022 12:23:16 +0200 Subject: [PATCH 2/4] Fix return state under pipefail --- test/test_helper.bash | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/test_helper.bash b/test/test_helper.bash index dcb16d9ebe..eaa7b08135 100644 --- a/test/test_helper.bash +++ b/test/test_helper.bash @@ -13,7 +13,9 @@ fixtures() { } filter_control_sequences() { - "$@" | sed $'s,\x1b\\[[0-9;]*[a-zA-Z],,g' + local status=0 + "$@" | sed $'s,\x1b\\[[0-9;]*[a-zA-Z],,g' || status=$? + return "$status" } if ! command -v tput >/dev/null; then From f56906cc2cb3575847bd00ae1eb857c595748a8f Mon Sep 17 00:00:00 2001 From: Martin Schulze Date: Wed, 20 Jul 2022 12:25:08 +0200 Subject: [PATCH 3/4] Test set -u recursively on whole suite --- .github/workflows/set_nounset.bash | 1 + .github/workflows/tests.yml | 1 + 2 files changed, 2 insertions(+) create mode 100644 .github/workflows/set_nounset.bash diff --git a/.github/workflows/set_nounset.bash b/.github/workflows/set_nounset.bash new file mode 100644 index 0000000000..8925c2d815 --- /dev/null +++ b/.github/workflows/set_nounset.bash @@ -0,0 +1 @@ +set -u diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index cf4ca8e30e..3874fa3b7e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -32,6 +32,7 @@ jobs: - '' # allow for some parallelity without GNU parallel, since it is not installed by default - 'BATS_NO_PARALLELIZE_ACROSS_FILES=1 BATS_NUMBER_OF_PARALLEL_JOBS=2' + - 'BASH_ENV=${GITHUB_WORKSPACE}/.github/workflows/set_nounset.bash' runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v2 From e2cf761de2138b5551482c34e6e9aed2d709c9fa Mon Sep 17 00:00:00 2001 From: Martin Schulze Date: Sat, 23 Jul 2022 14:31:31 +0200 Subject: [PATCH 4/4] Add changelog entry for #621 The test goes deeper than the unofficial strict mode test but still cannot see all possible codepaths. --- docs/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9d3f1ad831..20bd8107cb 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -33,6 +33,7 @@ The format is based on [Keep a Changelog][kac] and this project adheres to * also add `--clean-and-gather-test-outputs-in ` for improved UX * double slashes in paths derived from TMPDIR on MacOS (#607) * fix `load` in `teardown` marking failed tests as not run (#612) +* fix unset variable errors (with set -u) and add regression test (#621) #### Documentation