From 89eb2034883a912382706f8f6fc98b3457ee4f33 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Sat, 25 Jul 2026 11:42:23 +0200 Subject: [PATCH] fix(cli): validate option values instead of dropping bad ones The numeric settings are compared with [ -lt ], which errors instead of returning false on a non-integer operand. Nothing validated them, so: --jobs abc hung on Bash 3.x (the job-slot poll never reached its break) and was ignored on Bash 4.3+ --coverage-min abc leaked '[: abc: integer expression expected' and still exited 0, so the threshold went unenforced --retry/-timeout silently dropped --output nonsense silently fell back to the default renderer Validation runs once on the resolved configuration, after flag parsing, so it covers the BASHUNIT_* environment variables that bypass the parser as well as the flags. Closes #873 --- CHANGELOG.md | 1 + src/main.sh | 50 +++++++++ .../bashunit_invalid_option_value_test.sh | 100 ++++++++++++++++++ 3 files changed, 151 insertions(+) create mode 100644 tests/acceptance/bashunit_invalid_option_value_test.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index c203cdf5..fb5bb696 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 +- 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` - `.env.example` documented `BASHUNIT_SHOW_EXECUTION_TIME` as defaulting to `true`; the actual default has been `auto` since #765 diff --git a/src/main.sh b/src/main.sh index c8cd4bc1..b0f570d7 100644 --- a/src/main.sh +++ b/src/main.sh @@ -10,6 +10,54 @@ function bashunit::main::abort_unknown_option() { exit 1 } +## +# Exits non-zero unless the value is a non-negative integer. +# Arguments: $1 - value, $2 - the setting name to quote in the error +## +function bashunit::main::require_non_negative_int_or_exit() { + case "$1" in + '' | *[!0-9]*) + printf "%sError: %s must be a non-negative integer, got '%s'.%s\n" \ + "${_BASHUNIT_COLOR_FAILED}" "$2" "$1" "${_BASHUNIT_COLOR_DEFAULT}" >&2 + exit 1 + ;; + esac +} + +## +# 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 +# vars, which bypass the parser entirely. The numeric settings are compared with +# `[ -lt ]`, which errors instead of returning false on a non-integer operand: the +# job-slot poll looped forever and the rest silently dropped the setting (#873). +## +function bashunit::main::validate_config_or_exit() { + if [ "${BASHUNIT_PARALLEL_JOBS:-0}" != "0" ]; then + bashunit::main::require_non_negative_int_or_exit \ + "${BASHUNIT_PARALLEL_JOBS}" "BASHUNIT_PARALLEL_JOBS (--jobs)" + fi + bashunit::main::require_non_negative_int_or_exit \ + "${BASHUNIT_RETRY:-0}" "BASHUNIT_RETRY (--retry)" + bashunit::main::require_non_negative_int_or_exit \ + "${BASHUNIT_TEST_TIMEOUT:-0}" "BASHUNIT_TEST_TIMEOUT (--test-timeout)" + # Empty is the documented "no minimum" default, so only a set value is checked. + if [ -n "${BASHUNIT_COVERAGE_MIN:-}" ]; then + bashunit::main::require_non_negative_int_or_exit \ + "${BASHUNIT_COVERAGE_MIN}" "BASHUNIT_COVERAGE_MIN (--coverage-min)" + fi + + # 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 + '' | tap) ;; + *) + printf "%sError: unsupported output format '%s' for --output. Supported: tap.%s\n" \ + "${_BASHUNIT_COLOR_FAILED}" "${BASHUNIT_OUTPUT_FORMAT}" "${_BASHUNIT_COLOR_DEFAULT}" >&2 + exit 1 + ;; + esac +} + ## # Validates a `--shard /` spec and exports the parts, or prints an # error and exits non-zero. Requires numeric index/total with 1 <= index <= total. @@ -354,6 +402,8 @@ function bashunit::main::cmd_test() { shift done + bashunit::main::validate_config_or_exit + # Auto-enable coverage when any coverage output option is specified if [ "$_bashunit_coverage_opt_set" = true ]; then # shellcheck disable=SC2034 diff --git a/tests/acceptance/bashunit_invalid_option_value_test.sh b/tests/acceptance/bashunit_invalid_option_value_test.sh new file mode 100644 index 00000000..582f9332 --- /dev/null +++ b/tests/acceptance/bashunit_invalid_option_value_test.sh @@ -0,0 +1,100 @@ +#!/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" +} + +# The numeric options used to accept any string. `[ n -lt abc ]` errors rather +# than returning false, so the value was either ignored or -- on the Bash 3.x +# job-slot poll -- looped forever (#873). +function test_bashunit_fails_when_jobs_is_not_a_number() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --jobs abc "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "--jobs" "$output" +} + +function test_bashunit_fails_when_jobs_is_negative() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --jobs -5 "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" +} + +function test_bashunit_fails_when_retry_is_not_a_number() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --retry abc "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "--retry" "$output" +} + +function test_bashunit_fails_when_test_timeout_is_not_a_number() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --test-timeout abc "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "--test-timeout" "$output" +} + +function test_bashunit_fails_when_coverage_min_is_not_a_number() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --coverage-min abc "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "--coverage-min" "$output" +} + +function test_bashunit_fails_when_output_format_is_unsupported() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --output nonsense "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "--output" "$output" +} + +# The environment path bypasses flag parsing entirely, so it needs the same gate. +function test_bashunit_fails_when_parallel_jobs_env_var_is_not_a_number() { + local ec=0 + local output + output=$(BASHUNIT_PARALLEL_JOBS=abc ./bashunit --env "$TEST_ENV_FILE" --parallel "$TEST_FILE" 2>&1) || ec=$? + + assert_general_error "" "" "$ec" + assert_contains "BASHUNIT_PARALLEL_JOBS" "$output" +} + +function test_bashunit_still_accepts_valid_option_values() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --jobs 2 --retry 0 --test-timeout 0 "$TEST_FILE" 2>&1) || ec=$? + + assert_successful_code "" "" "$ec" + assert_contains "All tests passed" "$output" +} + +function test_bashunit_still_accepts_jobs_auto() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --jobs auto "$TEST_FILE" 2>&1) || ec=$? + + assert_successful_code "" "" "$ec" + assert_contains "All tests passed" "$output" +} + +function test_bashunit_still_accepts_tap_output() { + local ec=0 + local output + output=$(./bashunit --env "$TEST_ENV_FILE" --output tap "$TEST_FILE" 2>&1) || ec=$? + + assert_successful_code "" "" "$ec" + assert_contains "TAP version 13" "$output" +}