From 17ea6bb693cca45c7eac5c43ed7fd2430849583d Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Sat, 1 Aug 2026 22:56:34 +0200 Subject: [PATCH] fix(parallel): key per-test result dirs on the path, not the basename Under --parallel, each test file's results were bucketed into a directory named by the file's basename: local test_suite_base="${test_file##*/}" local test_suite_dir="${TEMP_DIR_PARALLEL_TEST_SUITE}/${test_suite_base%.sh}" Two files sharing a basename in different directories therefore shared one bucket, and the per-suite ordinal is unique per suite rather than globally, so `/foo_test/1.result` was written twice and the first write was lost. The run stayed green and simply reported fewer tests. Same defect as #923, which fixed exactly this in build.sh: a basename is not an identity when directories can repeat one. Found while mirroring tests/unit onto the src/ module layout (#957/#960), which produced three duplicate basenames and cost 52 tests under --parallel. That layout is what the docs encourage, so any user project doing it is exposed. The derivation now folds the whole path, and both sites that computed it -- runner/result.sh and the pre-create in runner/exec.sh (#813) -- call one shared helper, so they cannot drift apart. It stays pure parameter expansion: this runs once per test in every parallel worker and the path must remain fork-free. The fork-budget acceptance tests pass unchanged. Nothing parses the directory name; aggregate_parallel_results globs whatever subdirectories exist. Closes #959 --- CHANGELOG.md | 1 + src/runner/exec.sh | 4 +-- src/runner/result.sh | 28 +++++++++++++++++-- .../bashunit_parallel_consistency_test.sh | 28 +++++++++++++++++++ .../fixtures/dup_basename/one/test_dup.sh | 5 ++++ .../fixtures/dup_basename/two/test_dup.sh | 5 ++++ 6 files changed, 67 insertions(+), 4 deletions(-) create mode 100644 tests/acceptance/fixtures/dup_basename/one/test_dup.sh create mode 100644 tests/acceptance/fixtures/dup_basename/two/test_dup.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 208f9d3b..905cde45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Internal: `src/runner.sh` and `src/coverage.sh` are split into `src/runner/` and `src/coverage/` modules of single-responsibility files, each behind a `source`-only `index.sh` aggregator. A pure relocation, no behavior change; see [ADR-010](adrs/adr-010-src-module-directories.md) (#924, #925) ### Fixed +- Parallel runs no longer lose results when two test files in different directories share a filename. Per-test results were bucketed by basename, so the second file overwrote the first — silently, with the run still green. A tests/ tree mirroring a src/ tree makes that layout ordinary (#959) - Coverage no longer counts variable assignments as functions. A line like `URL="https://${host}/api"` was reported as a function, inflating `FNF`/`FNH` in the LCOV report (17 phantom entries in bashunit's own run); when the value also contained a `|`, the malformed record aborted the LCOV writer with a raw bash arithmetic error (#936) - `build.sh` dedupes embedded files by repo-relative path. The previous basename key compared the top-level loop's relative paths against the recursion's absolute ones, so a file reached from two places could be bundled twice in the released binary; it also collided for same-named files in different directories (#923) - `bashunit doc` no longer errors when the default bootstrap file is missing (#929) diff --git a/src/runner/exec.sh b/src/runner/exec.sh index cbd12235..3ebcd458 100644 --- a/src/runner/exec.sh +++ b/src/runner/exec.sh @@ -64,8 +64,8 @@ function bashunit::runner::call_test_functions() { # publish into it, and checking `[ -d ]` inside a worker races its siblings # (every worker would still pay the mkdir fork). if bashunit::parallel::is_enabled && [ "$allow_test_parallel" = true ]; then - local _suite_base="${script##*/}" - mkdir -p "${TEMP_DIR_PARALLEL_TEST_SUITE}/${_suite_base%.sh}" 2>/dev/null || true + bashunit::runner::parallel_suite_dir_to_slot "$script" + mkdir -p "$_BASHUNIT_RUNNER_SUITE_DIR_OUT" 2>/dev/null || true fi for fn_name in "${functions_to_run[@]+"${functions_to_run[@]}"}"; do diff --git a/src/runner/result.sh b/src/runner/result.sh index cac00c0b..2474adf2 100644 --- a/src/runner/result.sh +++ b/src/runner/result.sh @@ -16,6 +16,30 @@ function bashunit::runner::parse_result() { fi } +_BASHUNIT_RUNNER_SUITE_DIR_OUT="" + +## +# Writes the parallel result directory for a test file into +# _BASHUNIT_RUNNER_SUITE_DIR_OUT. +# +# Keyed on the whole path with separators folded, never on the basename: two +# files sharing a basename in different directories used to land in one +# directory, where their per-suite ordinals collided and the second file's +# results overwrote the first's -- silently, with the run still green (#959). +# That is the same defect #923 fixed in build.sh. Mirroring a source tree in +# tests/ makes duplicate basenames ordinary, so this is a normal layout. +# +# Pure parameter expansion: this runs once per test in every parallel worker and +# the path must stay fork-free (.claude/rules/perf-fork-budget.md). +# Arguments: $1 - the test file path +## +function bashunit::runner::parallel_suite_dir_to_slot() { + local key="${1#./}" + key="${key%.sh}" + key="${key//\//_}" + _BASHUNIT_RUNNER_SUITE_DIR_OUT="${TEMP_DIR_PARALLEL_TEST_SUITE}/${key}" +} + function bashunit::runner::parse_result_parallel() { local fn_name=$1 shift @@ -25,8 +49,8 @@ function bashunit::runner::parse_result_parallel() { # mkdir when the dir is missing (first test of the file wins the race, # `-p` makes the losers no-ops), and name the result file by the per-suite # ordinal the dispatcher assigned — unique without forking mktemp or mv. - local test_suite_base="${test_file##*/}" - local test_suite_dir="${TEMP_DIR_PARALLEL_TEST_SUITE}/${test_suite_base%.sh}" + bashunit::runner::parallel_suite_dir_to_slot "$test_file" + local test_suite_dir=$_BASHUNIT_RUNNER_SUITE_DIR_OUT [ -d "$test_suite_dir" ] || mkdir -p "$test_suite_dir" local unique_test_result_file="${test_suite_dir}/${_BASHUNIT_RUNNER_RESULT_ORDINAL}.result" diff --git a/tests/acceptance/bashunit_parallel_consistency_test.sh b/tests/acceptance/bashunit_parallel_consistency_test.sh index a30f4208..39e296aa 100644 --- a/tests/acceptance/bashunit_parallel_consistency_test.sh +++ b/tests/acceptance/bashunit_parallel_consistency_test.sh @@ -42,3 +42,31 @@ function test_jobs_auto_caps_at_detected_cores_and_matches_sequential() { assert_equals "$sequential_summary" "$auto_summary" } + +# Per-test results used to be bucketed by the test file's BASENAME, so two files +# sharing one in different directories wrote into the same bucket and their +# per-suite ordinals collided -- the second file's results overwrote the first's, +# silently, with the run still green (#959). Mirroring a source tree in tests/ +# makes duplicate basenames normal, so this is a realistic layout, not a corner. +function test_parallel_does_not_lose_same_named_files_in_different_dirs() { + local one=tests/acceptance/fixtures/dup_basename/one/test_dup.sh + local two=tests/acceptance/fixtures/dup_basename/two/test_dup.sh + + local sequential_output + sequential_output=$(./bashunit --no-parallel --env "$TEST_ENV_FILE" "$one" "$two" 2>&1) || true + + local parallel_output + parallel_output=$(./bashunit --parallel --env "$TEST_ENV_FILE" "$one" "$two" 2>&1) || true + + # Counts only, not the rendered line: the parallel spinner leaves control bytes + # and indentation on the summary, which is cosmetic and not what this asserts. + local sequential_counts + sequential_counts=$(echo "$sequential_output" | grep -oE '[0-9]+ (passed|total)' | tr '\n' ' ') || true + + local parallel_counts + parallel_counts=$(echo "$parallel_output" | grep -oE '[0-9]+ (passed|total)' | tr '\n' ' ') || true + + assert_same "$sequential_counts" "$parallel_counts" + # Guard against both sides collapsing to nothing and matching vacuously. + assert_contains "2 total" "$sequential_counts" +} diff --git a/tests/acceptance/fixtures/dup_basename/one/test_dup.sh b/tests/acceptance/fixtures/dup_basename/one/test_dup.sh new file mode 100644 index 00000000..55672e0c --- /dev/null +++ b/tests/acceptance/fixtures/dup_basename/one/test_dup.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +function test_dup_in_dir_one() { + assert_same "one" "one" +} diff --git a/tests/acceptance/fixtures/dup_basename/two/test_dup.sh b/tests/acceptance/fixtures/dup_basename/two/test_dup.sh new file mode 100644 index 00000000..6e7137e7 --- /dev/null +++ b/tests/acceptance/fixtures/dup_basename/two/test_dup.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +function test_dup_in_dir_two() { + assert_same "two" "two" +}