-
-
Notifications
You must be signed in to change notification settings - Fork 43
Support Bash 3.0 #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Support Bash 3.0 #584
Conversation
- Replace printf -v with command substitution (Bash 3.1 feature) - Replace += operators with explicit concatenation (Bash 3.1 feature) - Store regex patterns in variables for [[ =~ ]] compatibility - Fix empty array access with set -u by using conditional initialization - Update version check, tests, and documentation
- Replace shopt nocasematch (Bash 3.1+) with tr for case conversion - Skip release_test.sh on Bash 3.0 (release.sh uses += array syntax) - Fix shellcheck warnings with proper directive placement
- Build Docker image once and share via artifact - Run 4 test modes in parallel: sequential, parallel, simple, simple+parallel - Use matrix strategy for parallel job execution - Add git to Docker image for tests that need it
- Remove unused minor variable from version check - Split long lines in learn.sh
|
@akinomyoga thank you for the review! I addressed all your feedback, please let me know if you find anything else |
|
I haven't actually gone through the PR. I can later check it in detail. |
e1804db to
526f6ec
Compare
akinomyoga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regular expressions used in the following lines do not work in an intended way in Bash < 3.2.
src/coverage.sh:516: if [[ $brace_count -eq 0 && "$line" =~ \{ && "$line" =~ \} ]]; then
install.sh:61: [[ "$1" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ || "$1" == "latest" || "$1" == "beta" ]]
release.sh:532: if [[ ! "$version" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
src/learn.sh:861: [[ $1 =~ ^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$ ]]It seems you assign the regular expression to a variable and use it as [[ ... =~ $variable ]], but I recommend you to define a function like bashunit::regex_match() { [[ $1 =~ $2 ]]; } so that you can call bashunit::regex_match "$line" '\{', etc.
src/runner.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bash-3.0 has a bug that the replacements of line="${line//\[failed\]/$'\n'}" becomes '<newline>' instead of <newline>. See this example:
$ script='line="${line//2/$'\''\n'\''}"; echo "$line"'
$ echo "$script"
line="${line//2/$'\n'}"; echo "$line"
$ line=123 bash-3.0 -c "$script"
1'
'3
$ line=123 bash-3.1 -c "$script"
1
3The solution is not to quote the right-hand side (RHS) of the assignment:
$ script='line=${line//2/$'\''\n'\''}; echo "$line"'
$ echo "$script"
line=${line//2/$'\n'}; echo "$line"
$ line=123 bash-3.0 -c "$script"
1
3
$ line=123 bash-3.1 -c "$script"
1
3In the first place, the quoting of the RHSs of assignments is redundant because the RHSs are not subject to word splitting or pathname expansions (aka glob expansions). The only cases where quoting of the RHSs of assignments is needed are the case where the RHS contains shell special characters (such as whitespace or delimiters like ;&|<> and the case where the RHS contains expansions like "$*" or "${arr[*]}" with a custom IFS.
- Add regex_match helper function for Bash 3.0+ compatibility - Replace literal regex patterns with helper function calls - Fix Bash 3.0 assignment quoting bug with $'\n' replacements - Update learn tutorial example to use variable-based regex pattern Addresses reviewer feedback on PR #584
Add local declarations for loop variables to prevent them from leaking into the global scope. This addresses all variable leakage issues identified in PR review. Files updated: - src/benchmark.sh: Loop vars i, r, d, val - src/colors.sh: Loop var c - src/console_results.sh: Loop var arg - src/coverage.sh: Loop vars test_file, found_file, pid_file, line, data, display_file, hit, executable, pct, safe_filename, test_file, test_fn - src/runner.sh: Loop vars line, data - src/main.sh: Loop var arg
Complete comprehensive fix for variable leakage across entire codebase. Add local declarations for loop variables in for/while loops to prevent them from leaking into global scope. Files updated (32 locations total): - src/console_results.sh: 1 loop variable - src/coverage.sh: 2 loop variables - src/doc.sh: 1 loop variable - src/env.sh: 1 loop variable - src/globals.sh: 1 loop variable - src/helpers.sh: 4 loop variables - src/learn.sh: 1 loop variable - src/main.sh: 2 loop variables - src/parallel.sh: 2 loop variables - src/reports.sh: 3 loop variables - src/runner.sh: 14 loop variables - src/test_doubles.sh: 1 loop variable All tests pass: 530 passed, 2 snapshot, 532 total
When bashunit runs with BASHUNIT_STRICT_MODE=true (set -u), iterating
over arrays could trigger "unbound variable" errors. This fix implements
safe array iteration patterns:
- For value iteration: Use "${array[@]+"${array[@]}"}" pattern which
expands to nothing when array is empty, preventing unbound errors
- For indexed iteration: Check array size before loop with
[ "${#array[@]}" -gt 0 ] to avoid iterating over empty arrays
- Add default values (:-) when accessing array elements to handle
unset elements gracefully
Changes affect:
- src/benchmark.sh: Safe iteration over benchmark result arrays
- src/env.sh: Safe iteration over environment variable keys
- src/helpers.sh: Safe iteration over function names
- src/main.sh: Safe iteration over command-line arguments
- src/parallel.sh: Safe iteration over result files
- src/reports.sh: Safe iteration over test results
- src/runner.sh: Safe iteration over test files and functions
- src/test_doubles.sh: Safe iteration over mocked functions
All tests passing locally (738 passed, 3 skipped, 7 incomplete).
Fixes strict mode compatibility without breaking existing functionality.
- Remove duplicate local declaration in doc.sh - Fix uninitialized array variables in assert.sh by using safe initialization pattern - Clarify IFS usage in coverage.sh by moving to function start - Remove unnecessary count initialization in coverage.sh - Format all files with shfmt
Store all regex patterns in variables before use to ensure Bash 3.0 compatibility. In Bash < 3.2, regex matching with literal patterns doesn't work properly. Fixed patterns in: - src/doc.sh: code fence pattern - src/main.sh: digit validation pattern - src/helpers.sh: test name patterns and variable name validation - src/runner.sh: metacharacter detection patterns - src/clock.sh: digit validation pattern - src/assert.sh: eval and alias detection patterns All patterns now follow the safe approach: local _pattern='...' [[ $var =~ $_pattern ]]
Move _code_fence_pattern declaration outside the loop to prevent potential issues with local variable redeclaration in Bash 3.0. This fixes the assert documentation display tests.
2989328 to
f77aa5f
Compare
Replace regex matching with glob pattern matching for code fence detection to avoid backtick escaping issues in Bash 3.0. Changed from: [[ "$line" =~ ^\`\`\` ]] To: [[ "$line" == '```'* ]] This is more reliable across all Bash versions.
f77aa5f to
bc993e1
Compare
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
Fixes remaining literal regex patterns that don't work in Bash < 3.2: - build.sh: absolute path check - coverage.sh: comment and braces-only line patterns All regex patterns now stored in variables before use in =~ operator.
akinomyoga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are intentional because those functions are ensured to be called in a context where IFS is properly set, please ignore the following comments.
4f9e543 to
172026f
Compare
ef627e1 to
197f4b2
Compare
akinomyoga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Closes: #468 by @akinomyoga
Lower the minimum Bash version requirement from 3.2 to 3.0.
Changes
assert_contains_ignore_caseto usetrinstead ofshopt nocasematchprintf '%q'and data providersrelease_test.shon Bash 3.0 (release.sh requires 3.1+)