On Mon, Aug 30, 2021 at 09:42:03AM -0700, Junio C Hamano wrote:
> I wonder how close our test suite is for being "set -u" clean.
> Running our tests under "set -u" may not be a bad endpoint, but
> only if we can get there without too much pain.
I wondered, too, but I don't think we're very close.
Just throwing "set -u" at the top of test-lib.sh shows many issues:
- we don't initialize some known variables, like say, verbose_only. It
might be reasonable to have a big list of:
verbose_only=
verbose=
etc. That would probably be an improvement, though a slight
maintenance burden.
- reading optional environment variables needs to be careful. You
can't just say:
if test -z "$GIT_TEST_CMP"
but need:
if test -z "${GIT_TEST_CMP:-}"
It's doubly annoying when you're passing it to "test -z", of course,
since the whole point is to see if it's set. In this case you can't
just initialize to empty, since we might get the variable from the
environment. I guess you could do:
: ${GIT_TEST_CMP:=}
near the top of the script to pre-declare all such variables.
- optional arguments in functions need to be annotated. Like:
foo=${2:-}
Kind of annoying, but at least it communicates the intent in some
ways.
Below is a diff that gets "./t0001-init.sh" running. But:
- I have no illusions that there aren't tons of problems still
lurking. All of these spots were found by running repeatedly and
fixing up any errors. So whatever code paths I didn't run are
probably full of similar bugs.
- Most of the fixes were in test-lib.sh, so they'd apply to the other
tests. If that's the extent, then maybe it's not so bad. But it's
not clear how many individual scripts will need to be fixed, too
(and likewise, for script authors to remain aware as they write new
scripts). t0000, for example, fails half of its tests.
So I dunno. It could help find some bugs, but it seems like a
non-trivial burden.
t/test-lib-functions.sh | 2 +-
t/test-lib.sh | 75 ++++++++++++++++++++++-------------
2 files changed, 49 insertions(+), 28 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e28411bb75..c1449fb91c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1518,7 +1518,7 @@ test_detect_hash () {
# Load common hash metadata and common placeholder object IDs for use with
# test_oid.
test_oid_init () {
- test -n "$test_hash_algo" || test_detect_hash &&
+ test -n "${test_hash_algo:-}" || test_detect_hash &&
test_oid_cache <"$TEST_DIRECTORY/oid-info/hash-info" &&
test_oid_cache <"$TEST_DIRECTORY/oid-info/oid"
}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d6..72344ddf78 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -15,9 +15,11 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see http://www.gnu.org/licenses/ .
+set -u
+
# Test the binaries we have just built. The tests are kept in
# t/ subdirectory and are run in 'trash directory' subdirectory.
-if test -z "$TEST_DIRECTORY"
+if test -z "${TEST_DIRECTORY:-}"
then
# We allow tests to override this, in case they want to run tests
# outside of t/, e.g. for running tests on the test library
@@ -28,7 +30,7 @@ else
# is valid even if the current working directory is changed
TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
fi
-if test -z "$TEST_OUTPUT_DIRECTORY"
+if test -z "${TEST_OUTPUT_DIRECTORY:-}"
then
# Similarly, override this to store the test-results subdir
# elsewhere
@@ -61,13 +63,13 @@ export PERL_PATH SHELL_PATH
# the developer has TEST_OUTPUT_DIRECTORY part of his build options, then we'd
# reset this value to instead contain what the developer has specified. We thus
# have this knob to allow overriding the directory.
-if test -n "${TEST_OUTPUT_DIRECTORY_OVERRIDE}"
+if test -n "${TEST_OUTPUT_DIRECTORY_OVERRIDE:-}"
then
TEST_OUTPUT_DIRECTORY="${TEST_OUTPUT_DIRECTORY_OVERRIDE}"
fi
# Disallow the use of abbreviated options in the test suite by default
-if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}"
+if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:-}"
then
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
@@ -204,6 +206,24 @@ parse_option () {
esac
}
+debug=
+help=
+immediate=
+quiet=
+run_list=
+root=
+stress=
+tee=
+trace=
+valgrind=
+valgrind_only=
+verbose=
+verbose_log=
+verbose_only=
+write_junit_xml=
+
+skip_all=
+
# Parse options while taking care to leave $@ intact, so we will still
# have all the original command line options when executing the test
# script again for '--tee' and '--verbose-log' later.
@@ -271,7 +291,7 @@ case "$TRASH_DIRECTORY" in
esac
# If --stress was passed, run this test repeatedly in several parallel loops.
-if test "$GIT_TEST_STRESS_STARTED" = "done"
+if test "${GIT_TEST_STRESS_STARTED:-}" = "done"
then
: # Don't stress test again.
elif test -n "$stress"
@@ -359,7 +379,7 @@ fi
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
-if test "$GIT_TEST_TEE_STARTED" = "done"
+if test "${GIT_TEST_TEE_STARTED:-}" = "done"
then
: # do not redirect again
elif test -n "$tee"
@@ -391,7 +411,7 @@ then
# potentially re-executed with $TEST_SHELL_PATH for '--tee' or
# '--verbose-log', so the right shell is checked and the
# warning is issued only once.
- if test -n "$BASH_VERSION" && eval '
+ if test -n "${BASH_VERSION:-}" && eval '
test ${BASH_VERSINFO[0]} -gt 4 || {
test ${BASH_VERSINFO[0]} -eq 4 &&
test ${BASH_VERSINFO[1]} -ge 1
@@ -413,7 +433,7 @@ fi
# update the COLUMNS variable every time a non-builtin command
# completes, even for non-interactive shells.
# Disable that since we are aiming for repeatability.
-test -n "$BASH_VERSION" && shopt -u checkwinsize 2>/dev/null
+test -n "${BASH_VERSION:-}" && shopt -u checkwinsize 2>/dev/null
# For repeatability, reset the environment to known value.
# TERM is sanitized below, after saving color control sequences.
@@ -483,7 +503,7 @@ then
export GIT_INDEX_VERSION
fi
-if test -n "$GIT_TEST_PERL_FATAL_WARNINGS"
+if test -n "${GIT_TEST_PERL_FATAL_WARNINGS:-}"
then
GIT_PERL_FATAL_WARNINGS=1
export GIT_PERL_FATAL_WARNINGS
@@ -492,7 +512,7 @@ fi
# Add libc MALLOC and MALLOC_PERTURB test
# only if we are not executing the test with valgrind
if test -n "$valgrind" ||
- test -n "$TEST_NO_MALLOC_CHECK"
+ test -n "${TEST_NO_MALLOC_CHECK:-}"
then
setup_malloc_check () {
: nothing
@@ -517,7 +537,7 @@ unset CDPATH
unset GREP_OPTIONS
unset UNZIP
-case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in
+case $(echo ${GIT_TRACE:-} |tr "[A-Z]" "[a-z]") in
1|2|true)
GIT_TRACE=4
;;
@@ -603,7 +623,7 @@ say () {
say_color info "$*"
}
-if test -n "$HARNESS_ACTIVE"
+if test -n "${HARNESS_ACTIVE:-}"
then
if test "$verbose" = t || test -n "$verbose_only"
then
@@ -892,12 +912,12 @@ maybe_setup_verbose () {
}
maybe_teardown_valgrind () {
- test -z "$GIT_VALGRIND" && return
+ test -z "${GIT_VALGRIND:-}" && return
GIT_VALGRIND_ENABLED=
}
maybe_setup_valgrind () {
- test -z "$GIT_VALGRIND" && return
+ test -z "${GIT_VALGRIND:-}" && return
if test -z "$valgrind_only"
then
GIT_VALGRIND_ENABLED=t
@@ -969,12 +989,12 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
- expecting_failure=$2
+ expecting_failure=${2:-}
if test "${GIT_TEST_CHAIN_LINT:-1}" != 0; then
# turn off tracing for this test-eval, as it simply creates
# confusing noise in the "-x" output
- trace_tmp=$trace
+ trace_tmp=${trace:-}
trace=
# 117 is magic because it is unlikely to match the exit
# code of other programs
@@ -1022,7 +1042,7 @@ test_finish_ () {
echo >&3 ""
maybe_teardown_valgrind
maybe_teardown_verbose
- if test -n "$GIT_TEST_TEE_OFFSET"
+ if test -n "${GIT_TEST_TEE_OFFSET:-}"
then
GIT_TEST_TEE_OFFSET=$(test-tool path-utils file-size \
"$GIT_TEST_TEE_OUTPUT_FILE")
@@ -1032,7 +1052,7 @@ test_finish_ () {
test_skip () {
to_skip=
skipped_reason=
- if match_pattern_list $this_test.$test_count "$GIT_SKIP_TESTS"
+ if match_pattern_list $this_test.$test_count "${GIT_SKIP_TESTS:-}"
then
to_skip=t
skipped_reason="GIT_SKIP_TESTS"
@@ -1150,7 +1170,7 @@ test_done () {
finalize_junit_xml
- if test -z "$HARNESS_ACTIVE"
+ if test -z "${HARNESS_ACTIVE:-}"
then
mkdir -p "$TEST_RESULTS_DIR"
@@ -1312,17 +1332,18 @@ then
GIT_VALGRIND_ENABLED=t
test -n "$valgrind_only" && GIT_VALGRIND_ENABLED=
export GIT_VALGRIND_ENABLED
-elif test -n "$GIT_TEST_INSTALLED"
+elif test -n "${GIT_TEST_INSTALLED:-}"
then
GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path) ||
error "Cannot run git from $GIT_TEST_INSTALLED."
PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
else # normal case, use ../bin-wrappers only unless $with_dashes:
- if test -n "$no_bin_wrappers"
+ if test -n "${no_bin_wrappers:-}"
then
with_dashes=t
else
+ with_dashes=
git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
if ! test -x "$git_bin_dir/git"
then
@@ -1345,9 +1366,9 @@ GIT_CONFIG_NOSYSTEM=1
GIT_ATTR_NOSYSTEM=1
export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM
-if test -z "$GIT_TEST_CMP"
+if test -z "${GIT_TEST_CMP:-}"
then
- if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
+ if test -n "${GIT_TEST_CMP_USE_COPIED_CONTEXT:-}"
then
GIT_TEST_CMP="$DIFF -c"
else
@@ -1372,7 +1393,7 @@ fi
remove_trash=
this_test=${0##*/}
this_test=${this_test%%-*}
-if match_pattern_list "$this_test" "$GIT_SKIP_TESTS"
+if match_pattern_list "$this_test" "${GIT_SKIP_TESTS:-}"
then
say_color info >&3 "skipping test $this_test altogether"
skip_all="skip all tests in $this_test"
@@ -1392,7 +1413,7 @@ rm -fr "$TRASH_DIRECTORY" || {
}
remove_trash=t
-if test -z "$TEST_NO_CREATE_REPO"
+if test -z "${TEST_NO_CREATE_REPO:-}"
then
git init "$TRASH_DIRECTORY" >&3 2>&4 ||
error "cannot run git init"
@@ -1463,7 +1484,7 @@ yes () {
# to call "git env--helper" (via test_bool_env). Only do that work
# if needed by seeing if GIT_TEST_FAIL_PREREQS is set at all.
GIT_TEST_FAIL_PREREQS_INTERNAL=
-if test -n "$GIT_TEST_FAIL_PREREQS"
+if test -n "${GIT_TEST_FAIL_PREREQS:-}"
then
if test_bool_env GIT_TEST_FAIL_PREREQS false
then
@@ -1534,7 +1555,7 @@ test -n "$USE_LIBPCRE2" && test_set_prereq PCRE
test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
-if test -z "$GIT_TEST_CHECK_CACHE_TREE"
+if test -z "${GIT_TEST_CHECK_CACHE_TREE:-}"
then
GIT_TEST_CHECK_CACHE_TREE=true
export GIT_TEST_CHECK_CACHE_TREE
