diff --git a/CHANGELOG.md b/CHANGELOG.md index 897d90f47..d8fabf9d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased ### Changed +- Data providers pass arguments without per-argument base64 processes, preserving quoting, empty values and parser isolation (#1370) - JSON reports write ordinary filenames, test names and empty messages without per-field processes, preserving existing escaping (#1369) ### Fixed diff --git a/src/runner/exec.sh b/src/runner/exec.sh index 72c313a16..4f2026cdf 100644 --- a/src/runner/exec.sh +++ b/src/runner/exec.sh @@ -52,13 +52,6 @@ function bashunit::runner::order_functions_for_script() { _BASHUNIT_RUNNER_ORDERED_FNS_OUT="${ordered[*]+${ordered[*]}}" } -## -# Runs the given test functions of a script (sequentially, or one background -# worker per test under --parallel). -# Arguments: $1 script path, $2 space-separated test function names, already -# filter/tag/rerun-filtered by load_test_files (never empty: the caller skips -# the file when no function survives filtering). -## function bashunit::runner::report_unusable_provider() { local test_file="$1" local fn_name="$2" @@ -73,6 +66,14 @@ function bashunit::runner::report_unusable_provider() { reason="data provider '$provider' is not defined, so the test never ran" fi + bashunit::runner::report_provider_error "$test_file" "$fn_name" "$reason" +} + +function bashunit::runner::report_provider_error() { + local test_file="$1" + local fn_name="$2" + local reason="$3" + bashunit::state::add_tests_failed bashunit::console_results::print_error_test "$fn_name" "$reason" local _normalized_fn @@ -95,7 +96,13 @@ function bashunit::runner::report_unusable_provider() { fi } - +## +# Runs the given test functions of a script (sequentially, or one background +# worker per test under --parallel). +# Arguments: $1 script path, $2 space-separated test function names, already +# filter/tag/rerun-filtered by load_test_files (never empty: the caller skips +# the file when no function survives filtering). +## function bashunit::runner::call_test_functions() { local script="$1" local cached_functions="${2:-}" @@ -122,6 +129,7 @@ function bashunit::runner::call_test_functions() { local provider_data_count=0 local -a parsed_data=() local parsed_data_count=0 + local provider_arg_file="" # Monotonic within this file; names each parallel worker's .result file. local _test_ordinal=0 @@ -201,17 +209,48 @@ function bashunit::runner::call_test_functions() { continue fi - # Execute the test function for each line of data local data for data in "${provider_data[@]+"${provider_data[@]}"}"; do parsed_data=() parsed_data_count=0 + local transport_error="" + if ! bashunit::env::ensure_run_output_dir; then + transport_error="argument storage could not be created" + elif [ -z "$provider_arg_file" ] || [ ! -f "$provider_arg_file" ]; then + local provider_arg_dir="$_BASHUNIT_RUN_OUTPUT_DIR" + case "$provider_arg_dir" in + /*) ;; + *) provider_arg_dir="$BASHUNIT_WORKING_DIR/$provider_arg_dir" ;; + esac + provider_arg_file="$("$MKTEMP" "$provider_arg_dir/provider-args.XXXXXXX")" || + transport_error="argument storage could not be created" + fi local line - while IFS= read -r line; do - [ -z "$line" ] && continue - parsed_data[parsed_data_count]="$(bashunit::helper::decode_base64 "${line}")" - parsed_data_count=$((parsed_data_count + 1)) - done <<<"$(bashunit::runner::parse_data_provider_args "$data")" + # The parser's eval must stay in a subshell so expansions cannot change runner state. + if [ -z "$transport_error" ]; then + # Bash 3 does not invert a compound command's redirect failure with `!`. + if (bashunit::runner::parse_data_provider_args "$data" nul) >"$provider_arg_file"; then + if { + while IFS= read -r -d '' line; do + parsed_data[parsed_data_count]="$line" + parsed_data_count=$((parsed_data_count + 1)) + done + } <"$provider_arg_file"; then + : + else + transport_error="arguments could not be read" + fi + else + transport_error="arguments could not be written" + fi + fi + if [ -n "$transport_error" ]; then + _test_ordinal=$((_test_ordinal + 1)) + _BASHUNIT_RUNNER_RESULT_ORDINAL=$_test_ordinal + bashunit::runner::report_provider_error \ + "$script" "$fn_name" "data provider '$_BASHUNIT_PROVIDER_FN_OUT' $transport_error" + break + fi if bashunit::parallel::is_enabled && [ "$allow_test_parallel" = true ]; then bashunit::runner::wait_for_job_slot _test_ordinal=$((_test_ordinal + 1)) diff --git a/src/runner/provider.sh b/src/runner/provider.sh index a6fb440fe..4f91b1557 100644 --- a/src/runner/provider.sh +++ b/src/runner/provider.sh @@ -11,6 +11,7 @@ function bashunit::runner::parse_data_provider_args() { local i=0 local arg="" local encoded_arg + local output_format="${2:-base64}" local -a args=() local args_count=0 @@ -46,8 +47,12 @@ function bashunit::runner::parse_data_provider_args() { fi # Print args and return early for arg in "${args[@]+"${args[@]}"}"; do - encoded_arg="$(bashunit::helper::encode_base64 "${arg}")" - printf '%s\n' "$encoded_arg" + if [ "$output_format" = nul ]; then + printf '%s\0' "$arg" + else + encoded_arg="$(bashunit::helper::encode_base64 "${arg}")" + printf '%s\n' "$encoded_arg" + fi done return fi @@ -117,10 +122,14 @@ function bashunit::runner::parse_data_provider_args() { break fi done - # Print one arg per line to stdout, base64-encoded to preserve newlines in the data + # Keep the original base64 stdout format for callers without a requested transport. local arg for arg in ${args+"${args[@]}"}; do - encoded_arg="$(bashunit::helper::encode_base64 "${arg}")" - printf '%s\n' "$encoded_arg" + if [ "$output_format" = nul ]; then + printf '%s\0' "$arg" + else + encoded_arg="$(bashunit::helper::encode_base64 "${arg}")" + printf '%s\n' "$encoded_arg" + fi done } diff --git a/tests/acceptance/bashunit_provider_transport_test.sh b/tests/acceptance/bashunit_provider_transport_test.sh new file mode 100644 index 000000000..a997dcbf3 --- /dev/null +++ b/tests/acceptance/bashunit_provider_transport_test.sh @@ -0,0 +1,137 @@ +#!/usr/bin/env bash +set -euo pipefail + +function set_up_before_script() { + PROVIDER_TRANSPORT_FIXTURES="$(bashunit::temp_dir provider_transport)" +} + +function provide_transport_setup_failures() { + bashunit::data_set --no-parallel mktemp + bashunit::data_set --parallel mktemp + bashunit::data_set --no-parallel directory + bashunit::data_set --parallel directory +} + +# @data_provider provide_transport_setup_failures +function test_provider_storage_setup_failure_is_reported_once() { + local mode="$1" failure="$2" + local dir="$PROVIDER_TRANSPORT_FIXTURES/setup_${mode#--}_$failure" + mkdir -p "$dir" + { + if [ "$failure" = mktemp ]; then + printf '%s\n' 'function fail_provider_mktemp() { return 1; } +function set_up_before_script() { MKTEMP=fail_provider_mktemp; }' + else + printf '%s\n' 'function set_up_before_script() { + function bashunit::env::ensure_run_output_dir() { return 1; } +}' + fi + printf '%s\n' 'function test_before_provider_storage_failure() { assert_same 1 1; } +function provide_setup_failure_rows() { bashunit::data_set first; bashunit::data_set second; } +# @data_provider provide_setup_failure_rows +function test_row_with_unavailable_storage() { + printf executed >"$PROVIDER_BODY_MARKER" + assert_same first "${1-}" +}' + } >"$dir/setup_failure_test.sh" + + local output code=0 + output="$(PROVIDER_BODY_MARKER="$dir/body" ./bashunit "$mode" \ + --env tests/acceptance/fixtures/.env.default --report-json "$dir/report.json" \ + "$dir/setup_failure_test.sh" 2>&1)" || code=$? + output="$(printf '%s' "$output" | strip_ansi)" + + assert_same 1 "$code" + assert_contains "data provider 'provide_setup_failure_rows'" "$output" + assert_contains "1 passed" "$output" + assert_contains "1 failed" "$output" + assert_contains "2 total" "$output" + assert_contains '"total": 2, "passed": 1, "failed": 1' "$(<"$dir/report.json")" + assert_file_not_exists "$dir/body" +} + +function provide_transport_io_failures() { + bashunit::data_set --no-parallel write + bashunit::data_set --parallel write + bashunit::data_set --no-parallel read + bashunit::data_set --parallel read +} + +# @data_provider provide_transport_io_failures +function test_provider_storage_io_failure_does_not_execute_the_test_body() { + local mode="$1" failure="$2" + local dir="$PROVIDER_TRANSPORT_FIXTURES/io_${mode#--}_$failure" + mkdir -p "$dir" + { + if [ "$failure" = write ]; then + printf '%s\n' 'function unusable_provider_storage() { printf "%s\n" "$_BASHUNIT_RUN_OUTPUT_DIR"; } +function set_up_before_script() { MKTEMP=unusable_provider_storage; }' + else + printf '%s\n' 'function set_up_before_script() { + function bashunit::runner::parse_data_provider_args() { + rm -f "$provider_arg_file" + printf "%s\0" row + } +}' + fi + printf '%s\n' 'function test_before_provider_io_failure() { assert_same 1 1; } +function provide_io_failure_rows() { bashunit::data_set first; bashunit::data_set second; } +# @data_provider provide_io_failure_rows +function test_row_with_unreadable_storage() { + printf executed >"$PROVIDER_BODY_MARKER" + assert_same 0 "$#" +}' + } >"$dir/io_failure_test.sh" + + local output code=0 + output="$(PROVIDER_BODY_MARKER="$dir/body" ./bashunit "$mode" \ + --env tests/acceptance/fixtures/.env.default --report-json "$dir/report.json" \ + "$dir/io_failure_test.sh" 2>&1)" || code=$? + output="$(printf '%s' "$output" | strip_ansi)" + + assert_same 1 "$code" + assert_contains "data provider 'provide_io_failure_rows'" "$output" + assert_contains "1 passed" "$output" + assert_contains "1 failed" "$output" + assert_contains "2 total" "$output" + assert_contains '"total": 2, "passed": 1, "failed": 1' "$(<"$dir/report.json")" + assert_file_not_exists "$dir/body" +} + +function provide_transport_modes() { + printf '%s\n' --no-parallel --parallel +} + +# @data_provider provide_transport_modes +function test_provider_rows_restore_a_scratch_directory_removed_by_the_previous_row() { + local mode="$1" + local dir="$PROVIDER_TRANSPORT_FIXTURES/restore_${mode#--}" + mkdir -p "$dir" + : >"$dir/public_files" + printf '%s\n' '# bashunit: no-parallel-tests +function provide_rows_across_scratch_loss() { bashunit::data_set first; bashunit::data_set second; } +# @data_provider provide_rows_across_scratch_loss +function test_row_after_scratch_loss() { + assert_same 1 "$#" + assert_not_empty "${1-}" + printf "%s\n" "$1" >>"$PROVIDER_ROWS_MARKER" + if ! bashunit::check_os::is_windows; then + find "$_BASHUNIT_RUN_OUTPUT_DIR" -name "provider-args.*" -perm -004 -print >>"$PROVIDER_PUBLIC_FILES" + fi + if [ "${1-}" = first ]; then + printf "Removing scratch directory: %s\n" "$_BASHUNIT_RUN_OUTPUT_DIR" + bashunit::env::cleanup_run_output_dir + fi +}' >"$dir/scratch_loss_test.sh" + + local output code=0 + output="$(PROVIDER_ROWS_MARKER="$dir/rows" PROVIDER_PUBLIC_FILES="$dir/public_files" \ + ./bashunit "$mode" --env tests/acceptance/fixtures/.env.default \ + "$dir/scratch_loss_test.sh" 2>&1)" || code=$? + output="$(printf '%s' "$output" | strip_ansi)" + + assert_same 0 "$code" + assert_same $'first\nsecond' "$(<"$dir/rows")" + assert_empty "$(<"$dir/public_files")" + assert_not_contains "No such file or directory" "$output" +} diff --git a/tests/acceptance/bashunit_run_forks_test.sh b/tests/acceptance/bashunit_run_forks_test.sh index 67d7fc2b1..0d4856dfe 100644 --- a/tests/acceptance/bashunit_run_forks_test.sh +++ b/tests/acceptance/bashunit_run_forks_test.sh @@ -328,6 +328,45 @@ function test_reports_do_not_fork_base64_per_field() { assert_less_or_equal_than 16 "$calls" } +function test_provider_arguments_do_not_fork_base64_per_value() { + if bashunit::check_os::is_windows; then + bashunit::skip "PATH shims are unreliable under Git Bash" && return + fi + + local dir + dir="$(bashunit::temp_dir)" + local count_file="$dir/base64_calls" + local real_base64 + real_base64="$(command -v base64)" + { + echo '#!/usr/bin/env bash' + echo "case \"\$*\" in --help) ;; *) echo x >> \"$count_file\" ;; esac" + echo "exec \"$real_base64\" \"\$@\"" + } >"$dir/base64" + chmod +x "$dir/base64" + + local fixture="$dir/provider_forks_test.sh" + { + echo 'function provide_rows() {' + echo ' bashunit::data_set a b' + echo ' bashunit::data_set c d' + echo ' bashunit::data_set e f' + echo '}' + echo '# @data_provider provide_rows' + echo 'function test_row() { assert_not_empty "$1"; assert_not_empty "$2"; }' + } >"$fixture" + + local code=0 + PATH="$dir:$PATH" ./bashunit --no-parallel "$fixture" >/dev/null 2>&1 || code=$? + assert_same 0 "$code" + + local calls=0 + if [ -f "$count_file" ]; then + calls="$(grep -c . "$count_file" || true)" + fi + assert_equals 0 "$calls" +} + # Regression guard for the per-test hook path. A test in a file that defines # `set_up` or `tear_down` used to cost five process forks: each hook minted its # output file with `mktemp` and removed it with `rm -f`, and the temp-owner diff --git a/tests/functional/provider_test.sh b/tests/functional/provider_test.sh index 8e95d0264..424f98f79 100644 --- a/tests/functional/provider_test.sh +++ b/tests/functional/provider_test.sh @@ -153,10 +153,6 @@ function provide_two_args_with_spaces() { bashunit::data_set "first test" "second test" } -# A value ending in an odd run of backslashes escaped the `)` of the parser's -# `eval "args=($input)"` fast path, making it a syntax error -- which kills the -# command substitution the runner calls the parser inside, so the argument -# reached the test unset rather than as `C:` (#1134). function provide_trailing_backslash() { # shellcheck disable=SC1003 # a lone trailing backslash is the case under test echo 'C:\' @@ -167,3 +163,28 @@ function provide_trailing_backslash() { function test_a_value_ending_in_a_backslash_still_arrives() { assert_not_empty "${1-}" } + +function provide_lossless_values() { + bashunit::data_set "" $'unit\037separator' $'line\none\n' $'trailing \t ' "C:\\path\\" '*;|&' +} + +# @data_provider provide_lossless_values +function test_provider_values_arrive_without_record_delimiter_collisions() { + assert_same 6 "$#" + assert_same "" "$1" + assert_same $'unit\037separator' "$2" + assert_same $'line\none\n' "$3" + assert_same $'trailing \t ' "$4" + assert_same "C:\\path\\" "$5" + assert_same '*;|&' "$6" +} + +function provide_eval_assignment() { + printf '%s\n' '${PROVIDER_EVAL_SIDE_EFFECT:=changed}' +} + +# @data_provider provide_eval_assignment +function test_provider_eval_does_not_assign_in_runner() { + assert_same changed "$1" + assert_same unset "${PROVIDER_EVAL_SIDE_EFFECT-unset}" +}