ref: codebase quality sweep — dedup, dead code, cycles and defensive-code audit#862
Merged
Conversation
print_successful_test carried a verbatim 14-line copy of format_duration. Extract format_duration_to_slot following the house return-slot idiom so the per-test path formats fork-free, and keep format_duration as a thin echoing wrapper for the cold call sites. Output verified identical over the full ms range (all three branches).
Comment-only cleanup. The substantive fix is in the extract_branches header,
which still documented kind as {if, case} after loop constructs became branch
points (#858); the code emits |loop| and the unit tests already spec three
kinds, so the header was the stale side.
Also documents two previously undocumented load-bearing constructs: the
readonly re-source guard on the result-parsing regex, and the embed markers
build::embed_docs aborts on. Drops comments that only record when something was
written ("Existing test", "had no tests at all", "First unit tests for"),
and fixes an "internationally blank" typo in three discovery fixtures.
No behavior change; no shellcheck directive removed.
Four places where bashunit's own plumbing (not user test code) hid a failure and carried on with a silently wrong answer: - env.sh: the run scratch dir and shared temp dir were created with `mkdir -p ... 2>/dev/null || true`. Every deferred-output collector (failures, skipped, incomplete, risky, profile, rerun) is created lazily by `>>` under that dir and read behind `[ -s ]`, so a failed mkdir turned all of them into no-ops: a red suite rendered as green on a read-only or full TMPDIR. Extracted bashunit::env::create_scratch_dirs, which asserts the postcondition (both dirs exist) rather than trusting mkdir's exit code, and aborts with an actionable message. Checking the postcondition keeps a benign mkdir -p race on the shared temp dir harmless. - runner.sh: restore_workdir swallowed a failed `cd` back to BASHUNIT_WORKING_DIR. Since every later test file is discovered and sourced relative to that directory, the loop kept going from wherever the user's set_up_before_script left it and quietly dropped the remaining files from the run. It now aborts with a clear error, and takes an optional target so it is testable. - release.sh: rollback::auto printed "Rollback complete. Files restored to pre-release state." even when restore_files had failed, sending the operator away from a half-released working tree. Now reports the failure. Also asserts that the sandbox actually received a copy of the project: neither the tar pipe nor the cp fallback gives a conclusive exit code, so a rehearsal could "pass" every step it never ran. Kept as load-bearing (unchanged): `grep -c || true` under errexit, `((var++)) || true`, optional-tool probes, cross-platform fallbacks, and the set +euo pipefail / trap - ERR hook-failure contract from #836.
Bash has no type system; the equivalent shared contracts are the global env-var table, the named constants, and the delimited records passed between processes. Three of them had more than one definition site. - runner: drop _BASHUNIT_RUNNER_PARSE_RESULT_REGEX. It is a second, already-divergent statement of the "##KEY=value##" result-record layout (it stops at TEST_EXIT_CODE and never gained the four trailing fields), left behind by #610 when the [[ =~ ]] parse was replaced with parameter expansion. It has zero consumers, so a future reader would "update the contract" there and change nothing. - runner: pin build_timeout_result to state::export_subshell_context. The record has two writers in two files; a field added to the canonical one silently shortens the timeout line and every reader then mis-parses timed-out tests. Folding them into one function would put a call on the per-test hot path that #762/#764 deliberately stripped, so the layout is pinned by a test instead: zero runtime cost, fails on divergence. - env: register BASHUNIT_RERUN_FAILED, BASHUNIT_COVERAGE_SHOW_LINE_HITS and BASHUNIT_PARALLEL_JOBS in the central defaults table. They were the only public variables whose default lived inline at the read site (rerun.sh, coverage.sh) or as a bare literal. Purely additive: the :- guards stay so callers that unset them still work, and no bare aliases are introduced, matching the reasoning already recorded for RETRY/SEED. - .env.example: BASHUNIT_SHOW_EXECUTION_TIME has defaulted to "auto" since #765, not "true". Adds the 15 supported variables the file was missing.
The 2026 performance campaign left behind five internal helpers with no production callers, plus one branch that can no longer execute: - runner::call_test_functions kept a pre-cache fallback that recomputed the function list. Both call sites pass the cached list, and load_test_files skips a file whose list is empty, so the branch was unreachable since #598. Its filter/tag arguments were only read there, so the signature drops them. - state::calculate_total_assertions: superseded by runner::compute_total_assertions (#598), which does the same sum fork-free. - coverage::get_line_hits: superseded by coverage::get_all_line_hits (#644), which also propagates hits across backslash-continuation chains. Its two tests are ported to the live function rather than deleted. - helper::trim: never called from src/ since it was added in 2023. - dependencies::has_adjtimex: dead from birth, the adjtimex clock branch it was written for never shipped. Also: env::find_terminal_width guarded its first probe with a condition that is always true (leftover from the 2024 env.sh unification), and the --parallel unsupported-OS warning still claimed Alpine was excluded even though parallel runs there have been supported since the race conditions were fixed. All removals are sub-namespaced internal helpers, outside the documented public API (assert_* plus single-level bashunit::<name>).
The src/ call graph had two 2-node strongly connected components. Both were
design coupling rather than runtime bugs (every module is sourced before the
run starts, and build.sh concatenates them into one file, so Bash resolves the
mutual calls fine) - but each hid a helper living in the wrong module.
env.sh <-> globals.sh: env.sh owns BASHUNIT_DEV_LOG and its predicates yet had
to call up into globals.sh to log, while globals.sh's log writers called back
down for those predicates. env.sh also called bashunit::random_str at source
time, which only worked because the entrypoint happens to source globals.sh
first - an undocumented ordering constraint.
- bashunit::random_str -> str.sh (pure-bash, zero deps, genuine leaf)
- bashunit::current_timestamp, bashunit::log, bashunit::internal_log -> env.sh,
next to is_dev_mode_enabled / is_internal_log_enabled
parallel.sh <-> state.sh: parallel::aggregate_test_results hand-parsed the
'##ASSERTIONS_FAILED=...##TEST_EXIT_CODE=...' payload that
state::export_subshell_context writes, so the decoder for state's own format
lived in another module and had to reach up for all six counters.
- renamed to bashunit::state::aggregate_parallel_results and moved to
state.sh, next to the encoder; body unchanged
No behaviour, signature or public API change - all four functions keep their
names and stay available from the same entrypoint. The call graph is now an
acyclic 12-layer DAG.
bashunit::dependencies::has_adjtimex had no production caller: the whole has_* family was cross-checked and every other member is used, while the clock.sh time-source chain (perl > python > node > powershell > date) has no adjtimex step. Its last real use died in ab451be. bashunit::coverage::get_line_hits ran one grep -c per line and was superseded by get_all_line_hits in 6afbded; all seven production call sites use the single-pass variant. Both survived only on tests that assert the function does the one thing it does, with no other caller in the tree. Those tautological tests go with them, plus a now-inert has_adjtimex mock in console_results_test. tests/unit/fixtures/release/mock_checksum is referenced by no tracked file; test_update_checksum_updates_package_json writes its own.
When an assertion fails with no test_* frame on the stack and no custom title, the label is derived from FUNCNAME[$fallback_depth] -- the assertion's own name. That makes it sensitive to stack depth, and nothing covered it: the -a standalone path sets a custom title, which short-circuits the fallback. The fixtures are assembled with printf rather than a heredoc so bashunit's own duplicate-test-function scan does not read them as tests defined twice here.
…with Every assertion repeated the same four lines to report a failure: resolve the label into a slot, copy it to a local, mark the assertion failed, then call print_failed_test. That block appeared 57 times across eight files. bashunit::assert::fail_with does all four. It passes an explicit fallback depth of 3 to label_to_slot so the extra stack frame is invisible: when no test_* frame is on the stack the label still resolves to the assertion, not to the helper. label_to_slot grew an optional depth argument for that; its default is unchanged, so every other caller behaves exactly as before. assert_array_contains, assert_array_length, assert_array_not_contains and assert_file_permissions also stop resolving a label on their success paths, where it was computed and discarded.
… name assert_arrays_equal resolved its label through the echoing bashunit::assert::label wrapper. That wrapper adds a stack frame, so when the assertion failed outside a test function (e.g. from a set_up hook) the fallback named the wrapper: 'Bashunit::assert::label'. It now reports 'Assert arrays equal' like every other assertion, and no longer forks a subshell to build the label.
find_files_recursive built a find command string and eval'd it whenever the
path contained a literal '*'. eval word-splits the already-interpolated
string, so a quoted glob over a directory with a space in its name ('my
dir/*') became the two roots 'my' and 'dir/*' and matched nothing. It also
expanded $-substitutions and ran ';'-separated commands present in a path.
Expand the glob into an array with IFS='' instead: that disables field
splitting while leaving pathname expansion intact (verified on Bash 3.2), so
the roots survive spaces and the path is never evaluated as shell syntax. A
non-matching glob still stays literal, preserving the previous behaviour of
handing the raw pattern to find.
[ x -ne y ] exits with status 2, not 1, when an operand is not an integer. The exit-code assertions tested 'if [ actual -ne expected ]' to decide the failure branch, so status 2 was read as 'condition false' and fell through to add_assertions_passed: assert_exit_code 'not-a-number' was counted as a PASSED assertion. The test only went red incidentally, because the runner treats the [ builtin's stderr as an execution error; in 'bashunit assert' mode the verdict was simply wrong. State the pass condition inside [ ] and negate it. For two integers this is exactly equivalent to the old form; for an unparseable operand status 2 negates to true, so the assertion fails closed. This is the '! [ -lt ]' shape assert_less_than and friends already use, which is why that family was never affected. Applies to assert_exit_code, assert_successful_code, assert_unsuccessful_code, assert_general_error and assert_command_not_found.
Seven assertions take a variadic actual via actual_arr=("${@:2}"), so
calling one with the actual omitted leaves the array empty. Expanding an
empty array with a bare "${arr[@]}" is an unbound-variable error under
set -u on Bash < 4.4, and set -u is live inside the test subshell whenever
--strict is on. The same call therefore aborted with an internal error on
macOS system bash (3.2) while reporting a clean assertion failure on Bash 5.
Use the "${arr[@]+"${arr[@]}"}" guard that assert_line_count in this same
file already uses, so an omitted actual joins to "" on every supported Bash.
Covers assert_contains, assert_not_contains, assert_matches,
assert_not_matches, assert_string_starts_with, assert_string_ends_with and
assert_string_not_ends_with. The new acceptance test is only non-vacuous on
Bash < 4.4; it guards the Bash 3.0 CI leg.
compute_total_assertions and the parallel aggregator extract counters with
${result##*##ASSERTIONS_FAILED=}. When the marker is absent the strip is a
no-op and the variable holds the entire input string. The existing ${x:-0}
guard only defends against empty, not against non-numeric, and arbitrary text
in $(( )) is not 0 -- it is a fatal arithmetic syntax error that aborts the
run. In the parallel path the same text also reached [ "$exit_code" -ne 0 ],
which exits 2 and was therefore read as 'equal to 0', counting an unreadable
result file as a passing test.
Validate with a single case over the concatenated fields: digits (or empty,
which $(( )) already reads as 0) pass through untouched, anything else
degrades to zeros. One case per test, no fork, so the per-test fork budget is
unchanged. An unparseable result additionally forces exit_code=1 so it counts
as failed rather than silently passing, and is logged.
The only [[ ]] left in src/. It is prohibited by the project's Bash 3.0+ rules, and the codebase notes elsewhere that [[ =~ ]] is not available on the supported floor. A case glob is provably the same domain: a string is all-digits iff it is non-empty and contains no non-digit character. It also makes the eval'd 'return $exit_code_or_impl' interpolation provably numeric, so a spy exit code can no longer carry shell syntax into the generated function body. No behaviour change; covered by the existing spy tests.
The same seven-line nest appeared three times in run_test: check the flag, then either raise the parallel stop file or exit with EXIT_CODE_STOP_ON_FAILURE. Extracted as halt_if_stop_on_failure, which flattens the three deepest branches in the file and gives the policy a name. exit inside a function still exits the same shell, so behaviour is unchanged.
Chemaclass
added
bug
Listing a variable in .env.example makes it an unconditional assignment once copied to .env, because .env is sourced under set -o allexport. Documenting BASHUNIT_OUTPUT_FORMAT and BASHUNIT_SHOW_OUTPUT_ON_FAILURE there therefore overrode values the caller had exported, breaking their env-var forms. Also assert the tests-failed delta rather than the cumulative total in the parallel aggregation test, which otherwise counts unrelated earlier failures.
This was referenced Jul 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤔 Background
A full-tree quality pass over
src/,tests/and the top-level scripts: deduplication, dead-code removal, dependency-cycle untangling, weak-typing and error-hiding audits, and comment cleanup. Every removal was proved unreferenced against agit ls-filescorpus rather than inferred from a green suite.💡 Changes
assert_arrays_equalreported an internal name as its label, a globbed path containing a space silently discovered zero tests, and an unreadable parallel result aborted aggregation.release.shrollback no longer report success on failure.env/globalsandparallel/statecall cycles, leaving an acyclic module graph.