ref: second cleanup sweep — coverage/benchmark numeric bugs and duplication#880
Merged
Conversation
exec_assert/exec_multi_assert in src/main.sh had a comment restating
almost every line ("Check if the function exists", "Set test title
for this assertion", etc.) added when multi-assertion mode landed.
Remove them along with matching Arrange-Act-Assert narration in
bashunit_init_test.sh and a redundant section header in
bashunit_assert_multi_test.sh. Load-bearing comments (Bash 3 array
guard, #877 reference, exit-code vs output assertion branches) are
left untouched.
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. Reuses bashunit::main::require_non_negative_int_or_exit, the same gate already covering the other numeric BASHUNIT_* settings (#873).
`@max_ms` accepts a decimal (its parse_annotations regex is
`[0-9.][0-9.]*`, and the average itself can already be fractional),
but the status column used plain `[ "$avg" -le "$max_ms" ]`. That
errors instead of comparing on a fractional operand, so a well
under-budget row printed a stray "integer expression expected" line
and always rendered as failing (">") regardless of the real average.
Adds bashunit::math::is_le, mirroring bashunit::math::calculate's
bc > awk > integer fallback chain.
…them get_coverage_class and the HTML legend independently hardcoded 80/50 as the BASHUNIT_COVERAGE_THRESHOLD_HIGH/LOW fallback in six places, so a future change to _BASHUNIT_DEFAULT_COVERAGE_THRESHOLD_HIGH/LOW in env.sh (the single source of truth for every other BASHUNIT_* default) would silently drift from these copies. Reference the canonical globals instead, and pin the fallback behavior with a regression test.
Seven functions independently re-parsed get_all_line_hits into their own local -a hits_by_line with an identical 5-line while-loop. Bash 3.0 can't return an array from a function or copy a sparse array without losing its line-number keys, so extract the parse into bashunit::coverage::load_hits_by_line, which writes the shared _BASHUNIT_COVERAGE_HITS_BY_LINE global instead (same return-slot pattern already used elsewhere in this file). No behavior change; existing coverage unit/acceptance tests cover every call site touched.
assert_have_been_called, assert_have_been_called_times and assert_have_been_called_nth_with each repeated the same "resolve the spy's times file, cat it, default to 0" sequence. Extract bashunit::spy::times_to_slot (return-slot pattern, no added forks) and call it from all three. Adds direct unit coverage for the new helper alongside the existing spy assertion tests.
The two threshold tests passed in isolation but failed once integrated: .env is gitignored, so it exists in the main checkout but not in the worktrees they were written in. Both .env and .env.example list BASHUNIT_COVERAGE_THRESHOLD_LOW/HIGH with an empty value, and an allexport 'source .env' turns that listing into an unconditional assignment that overrides the exported value under test (#865). CI does 'cp .env.example .env', so this would have gone red there too. Run them with --skip-env-file, and correct an issue reference that pointed at a number that did not exist.
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
Related #879
A second 8-agent pass over the codebase. Four of the eight dimensions came back
genuinely clean (circular dependencies, unused code, defensive programming, legacy
code) — the earlier sweep in #862 and the CLI hardening in #871–#878 had already taken
that ground. The remaining four found two real bugs and two duplication clusters.
💡 Changes
BASHUNIT_COVERAGE_THRESHOLD_LOW/HIGH: a non-integer leaked a rawinteger expression expectedinto coverage reports and mis-bucketed every file's class (90% reported asmedium)@max_msbenchmark thresholds as floats: a decimal limit errored and rendered every row as failing regardless of the real averagehits_by_lineloader, copy-pasted into 7 coverage report writers, and the spy call-count lookup repeated across 3 assertionsenv.sh's canonical threshold defaults instead of hardcoding80/50in 6 places, and strip 18 lines of step-narrating comments from the assert CLI pathBoth bugs are not reproducible from inside a repo checkout:
.env/.env.examplelist the threshold names with empty values, and an allexportsource .envoverrides an exported value (#865). The new tests use--skip-env-filefor that reason.