From 6b2039d13cff7b8a52e30b16ede6691cf0bfd181 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Sat, 25 Jul 2026 14:54:25 +0200 Subject: [PATCH 1/2] fix(env): an empty .env entry no longer clobbers the caller's value .env is sourced under 'set -o allexport' so it can hold arbitrary shell, which made every KEY= line an unconditional assignment. Merely listing a name blanked it, so 'BASHUNIT_OUTPUT_FORMAT=tap ./bashunit' stopped working in any project whose .env mentioned that setting -- and a project could not document a setting without breaking it. Snapshot the caller's non-empty BASHUNIT_* values and restore any the file blanked. An entry with a value still wins, which is what a committed project config is for; only an empty one is treated as 'not configured here', matching how .bashunitrc already applies values. This had been concealing defects rather than merely annoying: two bugs fixed in #879 were not reproducible from inside a repo checkout because .env blanked the very variable under test. The two tests that needed --skip-env-file as a workaround now exercise the real path. Verified on real bash 3.0 (compgen -v accepts a prefix there) and under CI conditions with 'cp .env.example .env'. Closes #865 --- .env.example | 12 ++-- CHANGELOG.md | 1 + docs/configuration.md | 20 ++++-- src/env.sh | 39 +++++++++-- .../bashunit_env_file_precedence_test.sh | 68 +++++++++++++++++++ .../bashunit_invalid_option_value_test.sh | 13 ++-- 6 files changed, 130 insertions(+), 23 deletions(-) create mode 100644 tests/acceptance/bashunit_env_file_precedence_test.sh diff --git a/.env.example b/.env.example index 7958e79c..8c51ab19 100644 --- a/.env.example +++ b/.env.example @@ -3,12 +3,12 @@ # Copy this file to .env and customize as needed # All values shown are defaults (leave empty to use default) # -# Do NOT add a variable here just to document it. .env is loaded with -# `set -o allexport; source .env`, so every line is an unconditional assignment: -# listing a name makes an empty value OVERRIDE one the caller exported or set on -# the command line (`BASHUNIT_OUTPUT_FORMAT=tap ./bashunit` stops working). -# Settings that must stay overridable are documented in docs/configuration.md -# instead. `.bashunitrc` does not have this problem — it applies only when unset. +# Precedence: an entry left EMPTY here means "not configured", so a value the +# caller exported or set on the command line still wins +# (`BASHUNIT_OUTPUT_FORMAT=tap ./bashunit` keeps working). Give an entry a value +# and it takes effect for the project, overriding the ambient environment. +# Listing a name with no value is therefore safe and is how a setting is +# documented here. ################################################################################ #─────────────────────────────────────────────────────────────────────────────── diff --git a/CHANGELOG.md b/CHANGELOG.md index c8f5626c..290b431c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,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 +- An empty entry in `.env` no longer overrides a value the caller exported or set on the command line. `.env` is sourced under `set -o allexport`, so every line was an unconditional assignment: merely *listing* a name blanked it, and `BASHUNIT_OUTPUT_FORMAT=tap ./bashunit` silently stopped working in any project whose `.env` mentioned that setting. An empty entry now means "not configured here"; an entry with a value still takes effect for the project. This had been actively concealing defects — two of the bugs fixed in #879 were not reproducible from inside a repo checkout for exactly this reason (#865) - `BASHUNIT_COVERAGE_THRESHOLD_LOW`/`BASHUNIT_COVERAGE_THRESHOLD_HIGH` (env-only, no CLI flag) now validate as non-negative integers like the other numeric settings. They are compared with `[ -ge ]` in the coverage class lookup, which errors instead of returning false on a non-integer value: a bad threshold leaked a raw `integer expression expected` into the coverage report and silently mis-bucketed every file's high/medium/low class - `@max_ms` benchmark annotations with a decimal value (e.g. `@max_ms=100.5`) no longer leak a raw `integer expression expected` into the results table and always render as failing (`>`) regardless of the actual average; the threshold comparison now tolerates fractional operands like the average itself already could - `bashunit assert ` with no arguments now reports a clear error and exits non-zero. It used to compute a `-1` array index, which Bash 3.x rejects, so it leaked a raw `bad array subscript` and still exited `0` — a malformed standalone assertion in a deploy script reported success (#877) diff --git a/docs/configuration.md b/docs/configuration.md index 80903163..9df2fa6c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -27,13 +27,19 @@ It is meant for committing sensible project defaults. Precedence, from highest to lowest: 1. CLI flags (e.g. `--simple`) -2. Environment variables and the `.env` file -3. `.bashunitrc` -4. Built-in defaults - -`.bashunitrc` only fills values that are not already set, so an exported -environment variable or a `.env` entry always wins. `--skip-env-file` skips -`.bashunitrc` as well. +2. `.env` entries that have a value +3. Environment variables +4. `.bashunitrc` +5. Built-in defaults + +An entry left **empty** in `.env` means "not configured here" and does not +override the environment, so `BASHUNIT_OUTPUT_FORMAT=tap ./bashunit` keeps +working even when `.env` lists that name. Give the entry a value and it takes +effect for the whole project, overriding the ambient environment — that is what a +committed project config is for. + +`.bashunitrc` only fills values that are not already set, so anything above it +always wins. `--skip-env-file` skips both `.env` and `.bashunitrc`. ## Default path diff --git a/src/env.sh b/src/env.sh index e5195e6a..f4ddf985 100644 --- a/src/env.sh +++ b/src/env.sh @@ -62,10 +62,41 @@ function bashunit::env::positive_int_or_default() { # Load .env file (skip if --skip-env-file is used to keep shell environment intact) if [ "${BASHUNIT_SKIP_ENV_FILE:-false}" != "true" ]; then bashunit::env::load_config_file ".bashunitrc" - set -o allexport - # shellcheck source=/dev/null - [ -f ".env" ] && source .env - set +o allexport + + if [ -f ".env" ]; then + # `.env` is sourced under allexport so it can hold arbitrary shell, which + # means every `KEY=` line is an unconditional assignment: an empty entry used + # to wipe a value the caller exported, so a project could not document a + # setting without breaking it on the command line (#865). + # + # Snapshot the caller's non-empty BASHUNIT_* values and put back any the file + # blanked. A non-empty entry still wins -- that is what a project config is + # for -- and only an empty one is treated as "not configured here", matching + # how .bashunitrc already applies values. + _bashunit_env_preserved="" + for _bashunit_env_name in $(compgen -v BASHUNIT_ 2>/dev/null || true); do + eval "_bashunit_env_value=\${$_bashunit_env_name}" + if [ -n "$_bashunit_env_value" ]; then + _bashunit_env_preserved="$_bashunit_env_preserved $_bashunit_env_name" + eval "_bashunit_env_saved_$_bashunit_env_name=\$_bashunit_env_value" + fi + done + + set -o allexport + # shellcheck source=/dev/null + source .env + set +o allexport + + for _bashunit_env_name in $_bashunit_env_preserved; do + eval "_bashunit_env_value=\${$_bashunit_env_name}" + if [ -z "$_bashunit_env_value" ]; then + eval "export $_bashunit_env_name=\$_bashunit_env_saved_$_bashunit_env_name" + fi + eval "unset _bashunit_env_saved_$_bashunit_env_name" + done + + unset _bashunit_env_preserved _bashunit_env_name _bashunit_env_value + fi fi _BASHUNIT_DEFAULT_DEFAULT_PATH="tests" diff --git a/tests/acceptance/bashunit_env_file_precedence_test.sh b/tests/acceptance/bashunit_env_file_precedence_test.sh new file mode 100644 index 00000000..c156ddf0 --- /dev/null +++ b/tests/acceptance/bashunit_env_file_precedence_test.sh @@ -0,0 +1,68 @@ +#!/usr/bin/env bash + +# `.env` is loaded with `set -o allexport; source .env`, so every line is an +# unconditional assignment. Listing a name with an empty value therefore +# OVERRODE a value the caller exported, which meant a project could not document +# a setting in .env/.env.example without breaking it on the command line (#865). +# +# An empty entry now means "not configured here" and leaves the caller's value +# alone, matching how .bashunitrc already applies values (`${key:-val}`). +# A non-empty entry still wins, which is what a project config is for. + +BASHUNIT_PATH="$PWD/bashunit" + +function set_up() { + WORK_DIR=$(mktemp -d) + printf '#!/usr/bin/env bash\n\nfunction test_ok() {\n assert_same "1" "1"\n}\n' \ + >"$WORK_DIR/probe_test.sh" +} + +function tear_down() { + rm -rf "$WORK_DIR" +} + +function test_an_empty_env_entry_does_not_override_an_exported_value() { + printf 'BASHUNIT_OUTPUT_FORMAT=\n' >"$WORK_DIR/.env" + + local output + pushd "$WORK_DIR" >/dev/null + output=$(BASHUNIT_OUTPUT_FORMAT=tap "$BASHUNIT_PATH" . 2>&1) || true + popd >/dev/null + + assert_contains "TAP version 13" "$output" +} + +function test_a_non_empty_env_entry_still_applies() { + printf 'BASHUNIT_OUTPUT_FORMAT=tap\n' >"$WORK_DIR/.env" + + local output + pushd "$WORK_DIR" >/dev/null + output=$("$BASHUNIT_PATH" . 2>&1) || true + popd >/dev/null + + assert_contains "TAP version 13" "$output" +} + +# A project that pins a value must still win over the ambient environment: an +# empty caller value is not a deliberate choice, so the .env entry applies. +function test_a_non_empty_env_entry_applies_over_an_empty_exported_value() { + printf 'BASHUNIT_OUTPUT_FORMAT=tap\n' >"$WORK_DIR/.env" + + local output + pushd "$WORK_DIR" >/dev/null + output=$(BASHUNIT_OUTPUT_FORMAT= "$BASHUNIT_PATH" . 2>&1) || true + popd >/dev/null + + assert_contains "TAP version 13" "$output" +} + +function test_env_file_still_sets_a_value_the_caller_did_not_provide() { + printf 'BASHUNIT_OUTPUT_FORMAT=tap\n' >"$WORK_DIR/.env" + + local output + pushd "$WORK_DIR" >/dev/null + output=$("$BASHUNIT_PATH" . 2>&1) || true + popd >/dev/null + + assert_contains "ok 1 - Ok" "$output" +} diff --git a/tests/acceptance/bashunit_invalid_option_value_test.sh b/tests/acceptance/bashunit_invalid_option_value_test.sh index ed45d650..e1c4cd46 100644 --- a/tests/acceptance/bashunit_invalid_option_value_test.sh +++ b/tests/acceptance/bashunit_invalid_option_value_test.sh @@ -80,10 +80,11 @@ function test_bashunit_fails_when_parallel_jobs_env_var_is_not_a_number() { function test_bashunit_fails_when_coverage_threshold_high_env_var_is_not_a_number() { local ec=0 local output - # --skip-env-file is required: .env / .env.example both list these two names, - # and an allexport `source .env` turns an empty listing into an unconditional - # assignment that overrides the exported value under test (#865). - output=$(BASHUNIT_COVERAGE_THRESHOLD_HIGH=abc ./bashunit --skip-env-file --coverage "$TEST_FILE" 2>&1) || ec=$? + # .env / .env.example both list this name with an empty value. That used to + # override the exported value and this test needed --skip-env-file to work; + # since #865 an empty entry no longer clobbers the caller, so the real path is + # exercised here. + output=$(BASHUNIT_COVERAGE_THRESHOLD_HIGH=abc ./bashunit --coverage "$TEST_FILE" 2>&1) || ec=$? assert_general_error "" "" "$ec" assert_contains "BASHUNIT_COVERAGE_THRESHOLD_HIGH" "$output" @@ -92,8 +93,8 @@ function test_bashunit_fails_when_coverage_threshold_high_env_var_is_not_a_numbe function test_bashunit_fails_when_coverage_threshold_low_env_var_is_not_a_number() { local ec=0 local output - # See the note above: .env would otherwise override the value under test. - output=$(BASHUNIT_COVERAGE_THRESHOLD_LOW=abc ./bashunit --skip-env-file --coverage "$TEST_FILE" 2>&1) || ec=$? + # See the note above. + output=$(BASHUNIT_COVERAGE_THRESHOLD_LOW=abc ./bashunit --coverage "$TEST_FILE" 2>&1) || ec=$? assert_general_error "" "" "$ec" assert_contains "BASHUNIT_COVERAGE_THRESHOLD_LOW" "$output" From 33c00bf3dc2cffae7039120b7e5279b6c8b4fa16 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Sat, 25 Jul 2026 14:59:04 +0200 Subject: [PATCH 2/2] fix(make): lint untracked scripts too, and satisfy shellcheck in the new test CI caught SC2164/SC1007 in the new test file that 'make sa' had passed. Cause: since #863 the file list comes from 'git ls-files', which only lists TRACKED files, so a brand-new script was invisible locally and first failed in CI -- exactly backwards. Add --others --exclude-standard. Verified by dropping an untracked offending script in the tree: make sa now fails on it. --- Makefile | 7 ++++++- .../bashunit_env_file_precedence_test.sh | 18 +++++++++--------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index a7e48bcf..ebf767fa 100644 --- a/Makefile +++ b/Makefile @@ -100,12 +100,17 @@ test/parallel: $(TEST_SCRIPTS) # the target lints the repo N+1 times and appears to hang), and its "*.sh" glob # never sees the extensionless entrypoint or the bin/ scripts. `find` remains # the fallback outside a git checkout. +# +# --others --exclude-standard includes files that are new and not yet staged: +# without it a brand-new script is invisible to `make sa` and only fails once CI +# lints it, which is exactly backwards. sa: ifndef STATIC_ANALYSIS_CHECKER @printf "\e[1m\e[31m%s\e[0m\n" "Shellcheck not installed: Static analysis not performed!" && exit 1 else @{ if git rev-parse --is-inside-work-tree >/dev/null 2>&1; then \ - git ls-files -z "*.sh" bashunit bin/pre-commit bin/create-pr; \ + git ls-files -z --cached --others --exclude-standard \ + "*.sh" bashunit bin/pre-commit bin/create-pr; \ else \ find . -name "*.sh" -not -path "./local/*" -not -path "./.claude/worktrees/*" -print0; \ fi; } \ diff --git a/tests/acceptance/bashunit_env_file_precedence_test.sh b/tests/acceptance/bashunit_env_file_precedence_test.sh index c156ddf0..703b4fd7 100644 --- a/tests/acceptance/bashunit_env_file_precedence_test.sh +++ b/tests/acceptance/bashunit_env_file_precedence_test.sh @@ -25,9 +25,9 @@ function test_an_empty_env_entry_does_not_override_an_exported_value() { printf 'BASHUNIT_OUTPUT_FORMAT=\n' >"$WORK_DIR/.env" local output - pushd "$WORK_DIR" >/dev/null + pushd "$WORK_DIR" >/dev/null || return 1 output=$(BASHUNIT_OUTPUT_FORMAT=tap "$BASHUNIT_PATH" . 2>&1) || true - popd >/dev/null + popd >/dev/null || return 1 assert_contains "TAP version 13" "$output" } @@ -36,9 +36,9 @@ function test_a_non_empty_env_entry_still_applies() { printf 'BASHUNIT_OUTPUT_FORMAT=tap\n' >"$WORK_DIR/.env" local output - pushd "$WORK_DIR" >/dev/null + pushd "$WORK_DIR" >/dev/null || return 1 output=$("$BASHUNIT_PATH" . 2>&1) || true - popd >/dev/null + popd >/dev/null || return 1 assert_contains "TAP version 13" "$output" } @@ -49,9 +49,9 @@ function test_a_non_empty_env_entry_applies_over_an_empty_exported_value() { printf 'BASHUNIT_OUTPUT_FORMAT=tap\n' >"$WORK_DIR/.env" local output - pushd "$WORK_DIR" >/dev/null - output=$(BASHUNIT_OUTPUT_FORMAT= "$BASHUNIT_PATH" . 2>&1) || true - popd >/dev/null + pushd "$WORK_DIR" >/dev/null || return 1 + output=$(BASHUNIT_OUTPUT_FORMAT='' "$BASHUNIT_PATH" . 2>&1) || true + popd >/dev/null || return 1 assert_contains "TAP version 13" "$output" } @@ -60,9 +60,9 @@ function test_env_file_still_sets_a_value_the_caller_did_not_provide() { printf 'BASHUNIT_OUTPUT_FORMAT=tap\n' >"$WORK_DIR/.env" local output - pushd "$WORK_DIR" >/dev/null + pushd "$WORK_DIR" >/dev/null || return 1 output=$("$BASHUNIT_PATH" . 2>&1) || true - popd >/dev/null + popd >/dev/null || return 1 assert_contains "ok 1 - Ok" "$output" }