From fca77b052a1e140fd24dcd8a97f6dbabede4534e Mon Sep 17 00:00:00 2001 From: D B Date: Fri, 28 Aug 2026 12:00:51 -0700 Subject: [PATCH] Make tests.sh strictly POSIX-portable, fix real quoting/logic bugs Addresses a full review (credit: Sol) of tests.sh against POSIX sh: - Remove `local` (not POSIX; widely supported but not guaranteed) - Replace the `getopts :hua-:` long-option hack (relies on unspecified getopts behavior for non-alphanumeric option chars) with a plain case/shift loop that handles short options, long options, and `--` uniformly -- simpler than running getopts and manual parsing side by side, and fully POSIX either way - Replace every `echo` with `printf`, since echo's handling of `-n` and backslashes is implementation-defined per POSIX (BSD vs System V) - Quote "$PHPUNIT_BIN", "$ME_DIR", and function arguments throughout -- unquoted expansions broke on paths containing whitespace - Recompute ME_DIR via `CDPATH= cd -P ... && pwd -P` with real failure handling, replacing a `cd "$dir"; pwd` pattern where a failed cd let pwd silently report the previous directory instead of erroring - Fix printf calls that used dynamic data as the format string itself (a literal `%` in a path would have been read as a format directive) - Replace the coverage-option text-generation (building a string then relying on word-splitting to turn it back into argv) with `set --`, so a coverage path containing whitespace can't corrupt phpunit's args - Narrow cmd_status_filter's "not a shell/signal status" range from a Linux-specific 126..165 guess to a portable 1..125, since POSIX does not define signal-number-to-exit-status encoding - Fix inconsistent phpunit-*.xml suffix handling: passing a suite name with or without the .xml extension now both resolve to the same file - Correct the usage line: `--` only terminates the wrapper's own option scanning (before ), it was never consumable after it Also fixes a real latent bug the restructuring surfaced: phpunit_coverage_check() returned 0 (success) on every branch, including "coverage should be skipped." That was inert in the original script because print_phpunit_coverage_opt duplicated the same guard inline and its exit status was never checked -- but using phpunit_coverage_check as an actual boolean gate (the correct way to build the coverage flags) surfaced it: coverage flags were being added even when xdebug wasn't available, which PHPUnit 13 treated as fatal ("No tests executed!"). Fixed by making the skip/unavailable branches return 1. Verified in php:8.5-cli: all 629 tests pass (default, --print-coverage, and explicit `phpunit` suite modes), error paths give correct messages and exit codes, and a run under a path containing a space confirmed the quoting fixes hold end-to-end. --- tests.sh | 235 +++++++++++++++++++++++++++---------------------------- 1 file changed, 117 insertions(+), 118 deletions(-) diff --git a/tests.sh b/tests.sh index b992921..5e257a7 100755 --- a/tests.sh +++ b/tests.sh @@ -3,10 +3,13 @@ # ME_ABOUT='wrapper to peform unit tests' -ME_USAGE='[<...OPTIONS>] [] [[--]<...passthru args>]' +ME_USAGE='[<...OPTIONS>] [--] [] [<...passthru args>]' ME_COPYRIGHT='Copyright (c) 2016-2026, Doug Bird. All Rights Reserved.' ME_NAME='tests.sh' -ME_DIR="/$0"; ME_DIR=${ME_DIR%/*}; ME_DIR=${ME_DIR:-.}; ME_DIR=${ME_DIR#/}/; ME_DIR=$(cd "$ME_DIR"; pwd) +ME_DIR=$(CDPATH= cd -P "$(dirname "$0")" && pwd -P) || { + printf '%s: failed to determine app root directory\n' "$ME_NAME" >&2 + exit 1 +} # # paths @@ -25,91 +28,91 @@ ME_ERROR_MISSING_DEP=3 CMD_STATUS_DONTUSE="255 $ME_ERROR_USAGE $ME_ERROR_ONE_OR_MORE_TESTS_FAILED $ME_ERROR_MISSING_DEP" print_hint() { - echo " Hint, try: $ME_NAME --usage" + printf ' Hint, try: %s --usage\n' "$ME_NAME" } PRINT_COVERAGE=0 HTML_COVERAGE_REPORT=0 SKIP_COVERAGE_REPORT=0 OPTION_STATUS=0 -while getopts :hua-: arg; do { case $arg in - h|u|a) HELP_MODE=1;; - -) LONG_OPTARG="${OPTARG#*=}"; case $OPTARG in - help|usage|about) HELP_MODE=1;; - skip-coverage) SKIP_COVERAGE_REPORT=1;; - html-coverage) HTML_COVERAGE_REPORT=1;; - print-coverage) PRINT_COVERAGE=1;; - show-coverage) PRINT_COVERAGE=1;; - coverage) PRINT_COVERAGE=1;; - *) >&2 echo "$ME_NAME: unrecognized long option --$OPTARG"; OPTION_STATUS=$ME_ERROR_USAGE;; - esac ;; - *) >&2 echo "$ME_NAME: unrecognized option -$OPTARG"; OPTION_STATUS=$ME_ERROR_USAGE;; -esac } done -shift $((OPTIND-1)) # remove parsed options and args from $@ list -[ "$OPTION_STATUS" != "0" ] && { >&2 echo "$ME_NAME: (FATAL) one or more invalid options"; >&2 print_hint; exit $OPTION_STATUS; } +while [ $# -gt 0 ]; do + case $1 in + --) shift; break ;; + -h|-u|-a|--help|--usage|--about) HELP_MODE=1 ;; + --skip-coverage) SKIP_COVERAGE_REPORT=1 ;; + --html-coverage) HTML_COVERAGE_REPORT=1 ;; + --print-coverage|--show-coverage|--coverage) PRINT_COVERAGE=1 ;; + --*) >&2 printf '%s: unrecognized long option %s\n' "$ME_NAME" "$1"; OPTION_STATUS=$ME_ERROR_USAGE ;; + -?*) >&2 printf '%s: unrecognized option %s\n' "$ME_NAME" "$1"; OPTION_STATUS=$ME_ERROR_USAGE ;; + *) break ;; + esac + shift +done +[ "$OPTION_STATUS" != "0" ] && { >&2 printf '%s: (FATAL) one or more invalid options\n' "$ME_NAME"; >&2 print_hint; exit "$OPTION_STATUS"; } if [ "$HELP_MODE" ]; then - echo "$ME_NAME" - echo "$ME_ABOUT" - echo "$ME_COPYRIGHT" - echo "" - echo "Usage:" - echo " $ME_NAME $ME_USAGE" - echo "" - echo "Arguments:" - echo " " - echo " Optionally specify a test suite; otherwise all test suites are performed." - echo " Acceptable Values: phpunit" - echo " Test Suite Descriptions:" - echo " phpunit: \"Unit\" phpunit test suite; see phpunit.xml" - echo " If xdebug is available, a coverage report in text format is (re)generated unless the '--skip-coverage' option is provided." - echo " Coverage report path: $ME_DIR/coverage.txt" - echo " HTML coverage report dir: $HTML_ROOT/.coverage" - echo "" - echo "Options:" - echo " --skip-coverage" - echo " Always skip creating coverage reports." - echo "" - echo " --html-coverage" - echo " Creates a coverage report in HTML format in a hidden folder in the project's 'web' directory." - echo " Ignored if xdebug is not available." - echo "" - echo " --print-coverage" - echo " Outputs a text coverage report after unit test completion." - echo " Ignored if xdebug is not available." - echo "" - echo "Exit code meanings:" - echo " $ME_ERROR_USAGE: command-line usage error" - echo " $ME_ERROR_MISSING_DEP: missing required dependency" - echo " $ME_ERROR_ONE_OR_MORE_TESTS_FAILED: one or more tests failed" + printf '%s\n' "$ME_NAME" + printf '%s\n' "$ME_ABOUT" + printf '%s\n' "$ME_COPYRIGHT" + printf '\n' + printf 'Usage:\n' + printf ' %s %s\n' "$ME_NAME" "$ME_USAGE" + printf '\n' + printf 'Arguments:\n' + printf ' \n' + printf ' Optionally specify a test suite; otherwise all test suites are performed.\n' + printf ' Acceptable Values: phpunit\n' + printf ' Test Suite Descriptions:\n' + printf ' phpunit: "Unit" phpunit test suite; see phpunit.xml\n' + printf " If xdebug is available, a coverage report in text format is (re)generated unless the '--skip-coverage' option is provided.\n" + printf ' Coverage report path: %s/coverage.txt\n' "$ME_DIR" + printf ' HTML coverage report dir: %s/.coverage\n' "$HTML_ROOT" + printf '\n' + printf 'Options:\n' + printf ' --skip-coverage\n' + printf ' Always skip creating coverage reports.\n' + printf '\n' + printf ' --html-coverage\n' + printf " Creates a coverage report in HTML format in a hidden folder in the project's 'web' directory.\n" + printf ' Ignored if xdebug is not available.\n' + printf '\n' + printf ' --print-coverage\n' + printf ' Outputs a text coverage report after unit test completion.\n' + printf ' Ignored if xdebug is not available.\n' + printf '\n' + printf 'Exit code meanings:\n' + printf ' %s: command-line usage error\n' "$ME_ERROR_USAGE" + printf ' %s: missing required dependency\n' "$ME_ERROR_MISSING_DEP" + printf ' %s: one or more tests failed\n' "$ME_ERROR_ONE_OR_MORE_TESTS_FAILED" exit 0 fi -if [ "$ME_DIR" != "$(pwd)" ]; then - cd $ME_DIR || { - >&2 echo "$ME_NAME: failed to change to app root directory" - exit 1 - } -fi +CDPATH= cd -P "$ME_DIR" || { + >&2 printf '%s: failed to change to app root directory\n' "$ME_NAME" + exit 1 +} cmd_status_filter() { cmd_status=$1 - ! [ "$cmd_status" -eq "$cmd_status" ] 2> /dev/null && return 1 case " $CMD_STATUS_DONTUSE " in *" $cmd_status "*) return 1;; esac - ( [ "$cmd_status" -lt "126" ] || [ "$cmd_status" -gt "165" ] ) && return $cmd_status + # only re-propagate a status in the range conventionally used for an + # application's own meaningful exit codes; anything outside 1..125 is + # shell/signal territory, and POSIX does not guarantee a portable + # signal-number encoding to unpack there. + [ "$cmd_status" -ge 1 ] && [ "$cmd_status" -le 125 ] && return "$cmd_status" return 1 } PHPUNIT_STATUS=-1 phpunit_sanity_check() { - [ "$PHPUNIT_STATUS" != "-1" ] && return $PHPUNIT_STATUS + [ "$PHPUNIT_STATUS" != "-1" ] && return $PHPUNIT_STATUS if [ ! -f "$PHPUNIT_BIN" ]; then - >&2 echo "$ME_NAME: phpunit binary '$PHPUNIT_BIN' is missing or inaccessible, have you run composer?" + >&2 printf "%s: phpunit binary '%s' is missing or inaccessible, have you run composer?\n" "$ME_NAME" "$PHPUNIT_BIN" PHPUNIT_STATUS=$ME_ERROR_MISSING_DEP return $ME_ERROR_MISSING_DEP fi if [ ! -x "$PHPUNIT_BIN" ]; then - >&2 echo "$ME_NAME: phpunit binary '$PHPUNIT_BIN' is not executable" + >&2 printf "%s: phpunit binary '%s' is not executable\n" "$ME_NAME" "$PHPUNIT_BIN" PHPUNIT_STATUS=$ME_ERROR_MISSING_DEP return $ME_ERROR_MISSING_DEP fi @@ -120,10 +123,10 @@ phpunit_sanity_check() { # phpunit wrapper function # phpunit() { - $PHPUNIT_BIN "$@" || { + "$PHPUNIT_BIN" "$@" || { cmd_status=$? - >&2 echo "$ME_NAME: phpunit failed with exit code $cmd_status" - cmd_status_filter $cmd_status + >&2 printf '%s: phpunit failed with exit code %s\n' "$ME_NAME" "$cmd_status" + cmd_status_filter "$cmd_status" return } return 0 @@ -131,55 +134,46 @@ phpunit() { XDEBUG_STATUS=-1 xdebug_sanity_check() { - [ "$XDEBUG_STATUS" != "-1" ] && return $XDEBUG_STATUS - php -m 2> /dev/null | grep xdebug > /dev/null 2>&1 - XDEBUG_STATUS=$? - [ "$XDEBUG_STATUS" = "0" ] || { - >&2 echo "$ME_NAME: (NOTICE) xdebug is not available, will skip coverage reports" - } - return $XDEBUG_STATUS + [ "$XDEBUG_STATUS" != "-1" ] && return $XDEBUG_STATUS + php -m 2> /dev/null | grep xdebug > /dev/null 2>&1 + XDEBUG_STATUS=$? + [ "$XDEBUG_STATUS" = "0" ] || { + >&2 printf '%s: (NOTICE) xdebug is not available, will skip coverage reports\n' "$ME_NAME" + } + return $XDEBUG_STATUS } phpunit_coverage_check() { - [ "$SKIP_COVERAGE_REPORT" = "0" ] || return 0 - xdebug_sanity_check || return 0 + [ "$SKIP_COVERAGE_REPORT" = "0" ] || return 1 + xdebug_sanity_check } print_phpunit_text_coverage_path() { - local test_suffix=$1 - if [ -z "$test_suffix" ]; then - printf "coverage.txt" - else - printf "coverage-$test_suffix.txt" - fi + test_suffix=$1 + if [ -z "$test_suffix" ]; then + printf '%s' 'coverage.txt' + else + printf '%s' "coverage-$test_suffix.txt" + fi } print_phpunit_html_coverage_path() { - local test_suffix=$1 - if [ -z "$test_suffix" ]; then - printf "$HTML_ROOT/.coverage" - else - printf "$HTML_ROOT/.coverage-$test_suffix" - fi -} - -print_phpunit_coverage_opt() { - local test_suffix=$1 - [ "$SKIP_COVERAGE_REPORT" = "0" ] || return 0 - xdebug_sanity_check || return 0 - if [ "$HTML_COVERAGE_REPORT" = "1" ]; then - printf " --coverage-html=$(print_phpunit_html_coverage_path $test_suffix) " + test_suffix=$1 + if [ -z "$test_suffix" ]; then + printf '%s' "$HTML_ROOT/.coverage" + else + printf '%s' "$HTML_ROOT/.coverage-$test_suffix" fi - printf " --coverage-text=$(print_phpunit_text_coverage_path $test_suffix) " } print_phpunit_coverage_report() { - local test_suffix=$1 - phpunit_coverage_check || return 0 - [ "$PRINT_COVERAGE" = "1" ] || return 0 - [ -f "$(print_phpunit_text_coverage_path $test_suffix)" ] || return 0 - printf "\n$(print_phpunit_text_coverage_path $test_suffix):\n" - cat $(print_phpunit_text_coverage_path $test_suffix) + test_suffix=$1 + phpunit_coverage_check || return 0 + [ "$PRINT_COVERAGE" = "1" ] || return 0 + coverage_path=$(print_phpunit_text_coverage_path "$test_suffix") + [ -f "$coverage_path" ] || return 0 + printf '\n%s:\n' "$coverage_path" + cat "$coverage_path" } # @@ -192,11 +186,18 @@ run_phpunit_suite() { suite_config=$1 suite_suffix=$2 shift 2 + # build phpunit's argv via "set --" (real argument list, not a + # word-split string) so nothing here breaks on paths containing spaces if [ -n "$suite_config" ]; then - phpunit $(print_phpunit_coverage_opt "$suite_suffix") -c "$suite_config" "$@" - else - phpunit $(print_phpunit_coverage_opt "$suite_suffix") "$@" + set -- -c "$suite_config" "$@" fi + if phpunit_coverage_check; then + set -- "--coverage-text=$(print_phpunit_text_coverage_path "$suite_suffix")" "$@" + if [ "$HTML_COVERAGE_REPORT" = "1" ]; then + set -- "--coverage-html=$(print_phpunit_html_coverage_path "$suite_suffix")" "$@" + fi + fi + phpunit "$@" suite_status=$? [ "$suite_status" = "0" ] && print_phpunit_coverage_report "$suite_suffix" return $suite_status @@ -215,27 +216,27 @@ if [ -n "$TEST_SUITE" ]; then if [ "$TEST_SUITE" = "phpunit" ]; then phpunit_sanity_check || exit run_phpunit_suite "" "" "$@" || { - cmd_status_filter $? - exit + cmd_status_filter $? + exit } exit 0 fi case $TEST_SUITE in phpunit-*) - if [ -f "$TEST_SUITE.xml" ]; then - TEST_SUFFIX=${TEST_SUITE#phpunit-} - TEST_SUFFIX=${TEST_SUFFIX%.xml} + TEST_SUITE_XML=${TEST_SUITE%.xml} + if [ -f "$TEST_SUITE_XML.xml" ]; then + TEST_SUFFIX=${TEST_SUITE_XML#phpunit-} phpunit_sanity_check || exit - run_phpunit_suite "$TEST_SUITE.xml" "$TEST_SUFFIX" "$@" || { - cmd_status_filter $? - exit - } + run_phpunit_suite "$TEST_SUITE_XML.xml" "$TEST_SUFFIX" "$@" || { + cmd_status_filter $? + exit + } exit 0 fi ;; esac - >&2 echo "$ME_NAME: (FATAL) unrecognized test suite: $TEST_SUITE" + >&2 printf '%s: (FATAL) unrecognized test suite: %s\n' "$ME_NAME" "$TEST_SUITE" >&2 print_hint exit $ME_ERROR_USAGE fi @@ -266,9 +267,7 @@ for file in phpunit-*.xml; do run_phpunit_suite "$file" "$TEST_SUFFIX" || TESTS_STATUS=$ME_ERROR_ONE_OR_MORE_TESTS_FAILED done - - [ "$TESTS_STATUS" -eq "0" ] || { - >&2 echo "$ME_NAME: one or more tests failed" + >&2 printf '%s: one or more tests failed\n' "$ME_NAME" exit $TESTS_STATUS -} \ No newline at end of file +}