fix(tests): guard remaining bash 3.2 hazards from PR #95 review
Review findings #5 and #7 on PR #95 flagged two bash-3.2-incompatible patterns despite the surrounding scripts claiming 3.2 safety: - tests/run-bats.sh used `mapfile` (bash 4.0+), which fails immediately under macOS's stock bash 3.2 before any batching logic runs. Replaced with the `while read` loop already established in tests/run-tests.sh, and guarded the two downstream `${TEST_FILES[@]}` expansions with `${arr[@]+"${arr[@]}"}` to match that file's convention. - `trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT` was unguarded in tests/test-sync-marketplace-mirror.sh and tests/test-sync-plugin-content.sh: under `set -u`, if `mktemp -d` fails before the array is populated, the trap itself throws an unbound-variable error that masks the real test failure. A repo-wide grep for the same pattern turned up a third, unreviewed instance in tests/test-check-release-needed.sh. Fixed all three with the guarded idiom already used elsewhere in the repo. Extended the existing bash-3.2-hazard static check (test 16 in tests/test-vale-wrap.sh) to scan all four fixed files going forward, so a regression of either pattern fails the suite instead of only surfacing on a real bash 3.2 host. Refs: PR #95 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT
This commit is contained in:
@@ -16,7 +16,15 @@ if [[ ! -x "$BATS" ]]; then
|
|||||||
exit 1
|
exit 1
|
||||||
fi
|
fi
|
||||||
|
|
||||||
mapfile -t TEST_FILES < <(
|
# Collected with a `while read` loop rather than `mapfile` — macOS ships
|
||||||
|
# /bin/bash 3.2, which has no `mapfile`. Process substitution (not a pipe)
|
||||||
|
# keeps the loop in this shell so the appends survive. `sort` is still fed
|
||||||
|
# newline-delimited output, exactly as before. Same convention as
|
||||||
|
# tests/run-tests.sh.
|
||||||
|
TEST_FILES=()
|
||||||
|
while IFS= read -r f; do
|
||||||
|
TEST_FILES+=("$f")
|
||||||
|
done < <(
|
||||||
find "$REPO_ROOT" -name "*.bats" \
|
find "$REPO_ROOT" -name "*.bats" \
|
||||||
-not -path "*/tests/bats/*" \
|
-not -path "*/tests/bats/*" \
|
||||||
-not -path "*/test_helper/*" \
|
-not -path "*/test_helper/*" \
|
||||||
@@ -48,7 +56,7 @@ source "$REPO_ROOT/scripts/lib/batch-run.sh"
|
|||||||
|
|
||||||
declare -a batch_args=()
|
declare -a batch_args=()
|
||||||
i=0
|
i=0
|
||||||
for f in "${TEST_FILES[@]}"; do
|
for f in ${TEST_FILES[@]+"${TEST_FILES[@]}"}; do
|
||||||
i=$((i + 1))
|
i=$((i + 1))
|
||||||
cmd="$(printf '%q %q; echo $? >%q' "$BATS" "$f" "$SCRATCH_ROOT/$i.status")"
|
cmd="$(printf '%q %q; echo $? >%q' "$BATS" "$f" "$SCRATCH_ROOT/$i.status")"
|
||||||
batch_args+=("$i" "$cmd")
|
batch_args+=("$i" "$cmd")
|
||||||
@@ -59,7 +67,7 @@ FAIL=0
|
|||||||
TOTAL_OK=0
|
TOTAL_OK=0
|
||||||
TOTAL_NOT_OK=0
|
TOTAL_NOT_OK=0
|
||||||
i=0
|
i=0
|
||||||
for f in "${TEST_FILES[@]}"; do
|
for f in ${TEST_FILES[@]+"${TEST_FILES[@]}"}; do
|
||||||
i=$((i + 1))
|
i=$((i + 1))
|
||||||
rel="${f#"$REPO_ROOT"/}"
|
rel="${f#"$REPO_ROOT"/}"
|
||||||
echo "=== $rel ==="
|
echo "=== $rel ==="
|
||||||
|
|||||||
@@ -95,7 +95,7 @@ run_check() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
CLEANUP_DIRS=()
|
CLEANUP_DIRS=()
|
||||||
trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT
|
trap 'rm -rf ${CLEANUP_DIRS[@]+"${CLEANUP_DIRS[@]}"}' EXIT
|
||||||
track() { CLEANUP_DIRS+=("$1"); }
|
track() { CLEANUP_DIRS+=("$1"); }
|
||||||
|
|
||||||
# --- 1. Not targeting main: silent no-op regardless of state ---
|
# --- 1. Not targeting main: silent no-op regardless of state ---
|
||||||
|
|||||||
@@ -44,7 +44,7 @@ run_script() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
CLEANUP_DIRS=()
|
CLEANUP_DIRS=()
|
||||||
trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT
|
trap 'rm -rf ${CLEANUP_DIRS[@]+"${CLEANUP_DIRS[@]}"}' EXIT
|
||||||
track() { CLEANUP_DIRS+=("$1"); }
|
track() { CLEANUP_DIRS+=("$1"); }
|
||||||
|
|
||||||
# --- 1. No .claude-plugin/marketplace.json at all: real-sync mode is a no-op, exit 0 ---
|
# --- 1. No .claude-plugin/marketplace.json at all: real-sync mode is a no-op, exit 0 ---
|
||||||
|
|||||||
@@ -104,7 +104,7 @@ EOF
|
|||||||
}
|
}
|
||||||
|
|
||||||
CLEANUP_DIRS=()
|
CLEANUP_DIRS=()
|
||||||
trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT
|
trap 'rm -rf ${CLEANUP_DIRS[@]+"${CLEANUP_DIRS[@]}"}' EXIT
|
||||||
track() { CLEANUP_DIRS+=("$1"); }
|
track() { CLEANUP_DIRS+=("$1"); }
|
||||||
|
|
||||||
# --- 1. --check reports drift before any sync has run ---
|
# --- 1. --check reports drift before any sync has run ---
|
||||||
|
|||||||
@@ -444,7 +444,12 @@ fi
|
|||||||
# site, so the construct is not a hazard there and demanding the guarded form
|
# site, so the construct is not a hazard there and demanding the guarded form
|
||||||
# would be a wrong test. The file list covers every script this repo ships or
|
# would be a wrong test. The file list covers every script this repo ships or
|
||||||
# runs that a macOS user reaches: the wrapper itself, the two pre-commit hook
|
# runs that a macOS user reaches: the wrapper itself, the two pre-commit hook
|
||||||
# scripts, and the test runner AGENTS.md tells contributors to run by hand.
|
# scripts, the test runner AGENTS.md tells contributors to run by hand, its
|
||||||
|
# bats-dispatch companion, and the three test-*.sh scripts whose
|
||||||
|
# `trap 'rm -rf "${CLEANUP_DIRS[@]}"' EXIT` cleanup traps were unguarded (PR
|
||||||
|
# #95 review finding #7 named two of them; a repo-wide grep for the same
|
||||||
|
# pattern turned up test-check-release-needed.sh as a third) until they were
|
||||||
|
# switched to the guarded form.
|
||||||
# `mapfile` is checked alongside, because it is bash 4.0+ and the expansion scan
|
# `mapfile` is checked alongside, because it is bash 4.0+ and the expansion scan
|
||||||
# cannot see it — run-tests.sh carried one until it was replaced with a
|
# cannot see it — run-tests.sh carried one until it was replaced with a
|
||||||
# `while read` loop, and nothing would have caught its return. `declare -A`
|
# `while read` loop, and nothing would have caught its return. `declare -A`
|
||||||
@@ -478,7 +483,11 @@ for BASH32_SCRIPT in \
|
|||||||
"$REPO_ROOT/scripts/skill-size-check.sh" \
|
"$REPO_ROOT/scripts/skill-size-check.sh" \
|
||||||
"$REPO_ROOT/scripts/check-release-needed.sh" \
|
"$REPO_ROOT/scripts/check-release-needed.sh" \
|
||||||
"$REPO_ROOT/scripts/check-vale-style-sync.sh" \
|
"$REPO_ROOT/scripts/check-vale-style-sync.sh" \
|
||||||
"$REPO_ROOT/tests/run-tests.sh"; do
|
"$REPO_ROOT/tests/run-tests.sh" \
|
||||||
|
"$REPO_ROOT/tests/run-bats.sh" \
|
||||||
|
"$REPO_ROOT/tests/test-sync-marketplace-mirror.sh" \
|
||||||
|
"$REPO_ROOT/tests/test-sync-plugin-content.sh" \
|
||||||
|
"$REPO_ROOT/tests/test-check-release-needed.sh"; do
|
||||||
FOUND16="$(unguarded_expansions "$BASH32_SCRIPT")"
|
FOUND16="$(unguarded_expansions "$BASH32_SCRIPT")"
|
||||||
if [[ -n "$FOUND16" ]]; then
|
if [[ -n "$FOUND16" ]]; then
|
||||||
HAZARDS16+="$FOUND16 "
|
HAZARDS16+="$FOUND16 "
|
||||||
|
|||||||
Reference in New Issue
Block a user