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
src/coverage.sh
Outdated
| local file_data | ||
| local file_data_count=0 |
There was a problem hiding this comment.
This pattern is used multiple times in this PR, but this isn't safe. When no elements are added, "${file_data[@]}" still generates a single empty string in Bash 3.0. One needs to explicitly initialize an empty array as
| local file_data | |
| local file_data_count=0 | |
| local -a file_data=() | |
| local file_data_count=0 |
Note that -a is required here. local file_data=() (without -a) results in a scalar variable local file_data='()' containing '()' as a string. It should also be noted that local -a arr=(...) causes problems with custom IFS in Bash 3.0 in the general case, but an empty case local -a arr=() is the only exception that we can safely use in Bash 3.0.
Or if you decided not to use local -a arr=(...) at all even if ... is empty, you can do
| local file_data | |
| local file_data_count=0 | |
| local file_data | |
| file_data=() | |
| local file_data_count=0 |
The same applies to other occurrences of this pattern. Although whether the resulting array is empty is checked in some branches, I think one should maintain the safe style.
There was a problem hiding this comment.
I use now local -a file_data=(), thanks
src/runner.sh
Outdated
|
|
||
| local provider_data=() | ||
| # Bash 3.0 compatible: unset before redeclaring to clear previous iteration's data | ||
| unset provider_data |
There was a problem hiding this comment.
One should declare provider_data before the loop. Otherwise, this may remove user's global variable provider_data or a shell function provider_data if any.
There was a problem hiding this comment.
Also, to selectively remove the shell variable rather than a shell function, it is generally recommended to always specify the type of the unset (-v or -f).
| unset provider_data | |
| unset -v provider_data |
There was a problem hiding this comment.
Changed to local -a provider_data=() so no need to have unset at all
| @@ -253,7 +253,7 @@ function bashunit::state::calculate_total_assertions() { | |||
| numbers=$(echo "$input" | grep -oE '##ASSERTIONS_\w+=[0-9]+' | grep -oE '[0-9]+') | |||
|
|
|||
| for number in $numbers; do | |||
There was a problem hiding this comment.
This isn't an issue with this PR, but the variable number leaks here.
There was a problem hiding this comment.
thanks! I added local number
|
@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. |
2 participants
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+)