From 6d00680de2c8a517f2b6b5853588786ed92fae93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:29 +0200 Subject: [PATCH 01/15] test-lib: use $1, not $@ in test_known_broken_{ok,failure}_ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clarify that these two functions never take N arguments, they'll only ever receive one. They've needlessly used $@ over $1 since 41ac414ea2b (Sane use of test_expect_failure, 2008-02-01). In the future we might want to pass the test source to these, but now that's not the case. This preparatory change helps to clarify a follow-up change. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/test-lib.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 7726d1da88..3f11ce3511 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -804,14 +804,14 @@ test_failure_ () { test_known_broken_ok_ () { test_fixed=$(($test_fixed+1)) - say_color error "ok $test_count - $@ # TODO known breakage vanished" - finalize_test_case_output fixed "$@" + say_color error "ok $test_count - $1 # TODO known breakage vanished" + finalize_test_case_output fixed "$1" } test_known_broken_failure_ () { test_broken=$(($test_broken+1)) - say_color warn "not ok $test_count - $@ # TODO known breakage" - finalize_test_case_output broken "$@" + say_color warn "not ok $test_count - $1 # TODO known breakage" + finalize_test_case_output broken "$1" } test_debug () { From e0258f15cbe1412659a0a68ac17b42dd470a49fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:30 +0200 Subject: [PATCH 02/15] test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the control flow in test_done so that we'll set GIT_EXIT_OK=t after we call test_atexit_handler(). This seems to have been a mistake in 900721e15c4 (test-lib: introduce 'test_atexit', 2019-03-13). It doesn't make sense to allow our "atexit" handling to call "exit" without us emitting the errors we'll emit without GIT_EXIT_OK=t being set. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/test-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 3f11ce3511..c8c84ef9b1 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1169,12 +1169,12 @@ test_atexit_handler () { } test_done () { - GIT_EXIT_OK=t - # Run the atexit commands _before_ the trash directory is # removed, so the commands can access pidfiles and socket files. test_atexit_handler + GIT_EXIT_OK=t + finalize_test_output if test -z "$HARNESS_ACTIVE" From 25c2351d85bf19cfd0b19e2de8468c3604897dcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:31 +0200 Subject: [PATCH 03/15] test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change various "exit 1" checks that happened after our "die" handler had been set up to use BAIL_OUT instead. See 234383cd401 (test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use, 2021-10-14) for the benefits of the BAIL_OUT function. The previous use of "error" here was not a logic error, but the "exit" without "GIT_EXIT_OK" would emit the "FATAL: Unexpected exit with code $code" message on top of the error we wanted to emit. Since we'd also like to stop "prove" in its tracks here, the right thing to do is to emit a "Bail out!" message. Let's also move the "GIT_EXIT_OK=t" assignments to just above the "exit [01]" in "test_done". It's not OK if we exit in e.g. finalize_test_output. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/test-lib.sh | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index c8c84ef9b1..118720493b 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1173,8 +1173,6 @@ test_done () { # removed, so the commands can access pidfiles and socket files. test_atexit_handler - GIT_EXIT_OK=t - finalize_test_output if test -z "$HARNESS_ACTIVE" @@ -1246,6 +1244,7 @@ test_done () { fi test_at_end_hook_ + GIT_EXIT_OK=t exit 0 ;; *) @@ -1255,6 +1254,7 @@ test_done () { say "1..$test_count" fi + GIT_EXIT_OK=t exit 1 ;; esac @@ -1387,14 +1387,12 @@ fi GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib export GITPERLLIB test -d "$GIT_BUILD_DIR"/templates/blt || { - error "You haven't built things yet, have you?" + BAIL_OUT "You haven't built things yet, have you?" } if ! test -x "$GIT_BUILD_DIR"/t/helper/test-tool$X then - echo >&2 'You need to build test-tool:' - echo >&2 'Run "make t/helper/test-tool" in the source (toplevel) directory' - exit 1 + BAIL_OUT 'You need to build test-tool; Run "make t/helper/test-tool" in the source (toplevel) directory' fi # Are we running this test at all? @@ -1448,9 +1446,7 @@ remove_trash_directory () { # Test repository remove_trash_directory "$TRASH_DIRECTORY" || { - GIT_EXIT_OK=t - echo >&5 "FATAL: Cannot prepare test area" - exit 1 + BAIL_OUT 'cannot prepare test area' } remove_trash=t @@ -1466,7 +1462,7 @@ fi # Use -P to resolve symlinks in our working directory so that the cwd # in subprocesses like git equals our $PWD (for pathname comparisons). -cd -P "$TRASH_DIRECTORY" || exit 1 +cd -P "$TRASH_DIRECTORY" || BAIL_OUT "cannot cd -P to \"$TRASH_DIRECTORY\"" start_test_output "$0" From 46fb057aaad2cfcbf6cdf409fcf241a362ad77b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:32 +0200 Subject: [PATCH 04/15] test-lib: add a --invert-exit-code switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the ability to have those tests that fail return 0, and those tests that succeed return 1. This is useful e.g. to run "--stress" tests on tests that fail 99% of the time on some setup, i.e. to smoke out the flaky run which yielded success. In a subsequent commit a new SANITIZE=leak mode will make use of this. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/t0000-basic.sh | 72 ++++++++++++++++++++++++++++++++++++++++++++++++ t/test-lib.sh | 45 ++++++++++++++++++++++++++++-- 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 17a268ccd1..502b4bcf9e 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -578,6 +578,78 @@ test_expect_success 'subtest: --run invalid range end' ' EOF_ERR ' +test_expect_success 'subtest: --invert-exit-code without --immediate' ' + run_sub_test_lib_test_err full-pass \ + --invert-exit-code && + check_sub_test_lib_test_err full-pass \ + <<-\EOF_OUT 3<<-EOF_ERR + ok 1 - passing test #1 + ok 2 - passing test #2 + ok 3 - passing test #3 + # passed all 3 test(s) + 1..3 + # faking up non-zero exit with --invert-exit-code + EOF_OUT + EOF_ERR +' + +test_expect_success 'subtest: --invert-exit-code with --immediate: all passed' ' + run_sub_test_lib_test_err full-pass \ + --invert-exit-code --immediate && + check_sub_test_lib_test_err full-pass \ + <<-\EOF_OUT 3<<-EOF_ERR + ok 1 - passing test #1 + ok 2 - passing test #2 + ok 3 - passing test #3 + # passed all 3 test(s) + 1..3 + # faking up non-zero exit with --invert-exit-code + EOF_OUT + EOF_ERR +' + +test_expect_success 'subtest: --invert-exit-code without --immediate: partial pass' ' + run_sub_test_lib_test partial-pass \ + --invert-exit-code && + check_sub_test_lib_test partial-pass <<-\EOF + ok 1 - passing test #1 + not ok 2 - # TODO induced breakage (--invert-exit-code): failing test #2 + # false + ok 3 - passing test #3 + # failed 1 among 3 test(s) + 1..3 + # faked up failures as TODO & now exiting with 0 due to --invert-exit-code + EOF +' + +test_expect_success 'subtest: --invert-exit-code with --immediate: partial pass' ' + run_sub_test_lib_test partial-pass \ + --invert-exit-code --immediate && + check_sub_test_lib_test partial-pass \ + <<-\EOF_OUT 3<<-EOF_ERR + ok 1 - passing test #1 + not ok 2 - # TODO induced breakage (--invert-exit-code): failing test #2 + # false + 1..2 + # faked up failures as TODO & now exiting with 0 due to --invert-exit-code + EOF_OUT + EOF_ERR +' + +test_expect_success 'subtest: --invert-exit-code --immediate: got a failure' ' + run_sub_test_lib_test partial-pass \ + --invert-exit-code --immediate && + check_sub_test_lib_test_err partial-pass \ + <<-\EOF_OUT 3<<-EOF_ERR + ok 1 - passing test #1 + not ok 2 - # TODO induced breakage (--invert-exit-code): failing test #2 + # false + 1..2 + # faked up failures as TODO & now exiting with 0 due to --invert-exit-code + EOF_OUT + EOF_ERR +' + test_expect_success 'subtest: tests respect prerequisites' ' write_and_run_sub_test_lib_test prereqs <<-\EOF && diff --git a/t/test-lib.sh b/t/test-lib.sh index 118720493b..31213b5f95 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -238,6 +238,9 @@ parse_option () { ;; esac ;; + --invert-exit-code) + invert_exit_code=t + ;; *) echo "error: unknown test option '$opt'" >&2; exit 1 ;; esac @@ -788,15 +791,31 @@ test_ok_ () { finalize_test_case_output ok "$@" } +_invert_exit_code_failure_end_blurb () { + say_color warn "# faked up failures as TODO & now exiting with 0 due to --invert-exit-code" +} + test_failure_ () { failure_label=$1 test_failure=$(($test_failure + 1)) - say_color error "not ok $test_count - $1" + local pfx="" + if test -n "$invert_exit_code" # && test -n "$HARNESS_ACTIVE" + then + pfx="# TODO induced breakage (--invert-exit-code):" + fi + say_color error "not ok $test_count - ${pfx:+$pfx }$1" shift printf '%s\n' "$*" | sed -e 's/^/# /' if test -n "$immediate" then say_color error "1..$test_count" + if test -n "$invert_exit_code" + then + finalize_test_output + _invert_exit_code_failure_end_blurb + GIT_EXIT_OK=t + exit 0 + fi _error_exit fi finalize_test_case_output failure "$failure_label" "$@" @@ -1229,7 +1248,14 @@ test_done () { esac fi - if test -z "$debug" && test -n "$remove_trash" + if test -n "$stress" && test -n "$invert_exit_code" + then + # We're about to move our "$TRASH_DIRECTORY" + # to "$TRASH_DIRECTORY.stress-failed" if + # --stress is combined with + # --invert-exit-code. + say "with --stress and --invert-exit-code we're not removing '$TRASH_DIRECTORY'" + elif test -z "$debug" && test -n "$remove_trash" then test -d "$TRASH_DIRECTORY" || error "Tests passed but trash directory already removed before test cleanup; aborting" @@ -1242,6 +1268,14 @@ test_done () { } || error "Tests passed but test cleanup failed; aborting" fi + + if test -z "$skip_all" && test -n "$invert_exit_code" + then + say_color warn "# faking up non-zero exit with --invert-exit-code" + GIT_EXIT_OK=t + exit 1 + fi + test_at_end_hook_ GIT_EXIT_OK=t @@ -1254,6 +1288,13 @@ test_done () { say "1..$test_count" fi + if test -n "$invert_exit_code" + then + _invert_exit_code_failure_end_blurb + GIT_EXIT_OK=t + exit 0 + fi + GIT_EXIT_OK=t exit 1 ;; From ac8e3e94e56eb13ca4bd023973501c6a3ad8fd98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:33 +0200 Subject: [PATCH 05/15] t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reword the documentation added in 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in CI, 2021-09-23) for brevity. The comment added in the same commit was also misleading: We skip certain tests if SANITIZE=leak and GIT_TEST_PASSING_SANITIZE_LEAK=true, not if we're compiled with SANITIZE=leak. Let's just remove the comment, the control flow here is obvious enough that the code can speak for itself. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/README | 10 ++++------ t/test-lib.sh | 1 - 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/t/README b/t/README index 4f9981cf5e..7f40939253 100644 --- a/t/README +++ b/t/README @@ -366,12 +366,10 @@ excluded as so much relies on it, but this might change in the future. GIT_TEST_SPLIT_INDEX= forces split-index mode on the whole test suite. Accept any boolean values that are accepted by git-config. -GIT_TEST_PASSING_SANITIZE_LEAK= when compiled with -SANITIZE=leak will run only those tests that have whitelisted -themselves as passing with no memory leaks. Tests can be whitelisted -by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing -"test-lib.sh" itself at the top of the test script. This test mode is -used by the "linux-leaks" CI target. +GIT_TEST_PASSING_SANITIZE_LEAK=true skips those tests that haven't +declared themselves as leak-free by setting +"TEST_PASSES_SANITIZE_LEAK=true" before sourcing "test-lib.sh". This +test mode is used by the "linux-leaks" CI target. GIT_TEST_PROTOCOL_VERSION=, when set, makes 'protocol.version' default to n. diff --git a/t/test-lib.sh b/t/test-lib.sh index 31213b5f95..f8adb92f02 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1447,7 +1447,6 @@ then test_done fi -# skip non-whitelisted tests when compiled with SANITIZE=leak if test -n "$SANITIZE_LEAK" then if test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false From 366bd129dc323d21fa830ca086162341a9b7c2d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:34 +0200 Subject: [PATCH 06/15] test-lib: add a SANITIZE=leak logging mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the ability to run the test suite under a new "GIT_TEST_SANITIZE_LEAK_LOG=true" mode, when true we'll log the leaks we find an a new "test-results/.leak" directory. That new path is consistent with the existing "test-results/." results, except that those are all files, not directories. We also set "log_exe_name=1" to include the name of the executable in the filename. This gives us files like "trace.git." instead of the default of "trace.". I.e. we'll be able to distinguish "git" leaks from "test-tool", "git-daemon" etc. We then set "dedup_token_length" to non-zero ("0" is the default) to succinctly log a token we can de-duplicate these stacktraces on. The string is simply a one-line stack-trace with only function names up to N frames, which we limit at "9999" as a shorthand for "infinite" (there appears to be no way to say "no limit"). With these combined we can now easily get e.g. the top 10 leaks in the test suite grouped by full stacktrace: grep -o -P -h '(?<=DEDUP_TOKEN: ).*' test-results/*.leak/trace.git.* | sort | uniq -c | sort -nr | head -n 10 Or add "grep -E -o '[^-]+'" to that to group by functions instead of stack traces: grep -o -P -h '(?<=DEDUP_TOKEN: ).*' test-results/*.leak/trace.git.* | grep -E -o '[^-]+' | sort | uniq -c | sort -nr | head -n 20 This new mode requires git to be compiled with SANITIZE=leak, rather than explaining that in the documentation let's make it self-documenting by bailing out if the user asks for this without git having been compiled with SANITIZE=leak, as we do with GIT_TEST_PASSING_SANITIZE_LEAK=true. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/README | 5 +++++ t/test-lib.sh | 30 +++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/t/README b/t/README index 7f40939253..0664aee7ed 100644 --- a/t/README +++ b/t/README @@ -371,6 +371,11 @@ declared themselves as leak-free by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing "test-lib.sh". This test mode is used by the "linux-leaks" CI target. +GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to +"test-results/$TEST_NAME.leak/trace.*" files. The logs include a +"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to +make logs +machine-readable. + GIT_TEST_PROTOCOL_VERSION=, when set, makes 'protocol.version' default to n. diff --git a/t/test-lib.sh b/t/test-lib.sh index f8adb92f02..557f77c971 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -305,6 +305,10 @@ TEST_NUMBER="${TEST_NAME%%-*}" TEST_NUMBER="${TEST_NUMBER#t}" TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results" TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX" +TEST_RESULTS_SAN_FILE_PFX=trace +TEST_RESULTS_SAN_DIR_SFX=leak +TEST_RESULTS_SAN_FILE= +TEST_RESULTS_SAN_DIR="$TEST_RESULTS_DIR/$TEST_NAME.$TEST_RESULTS_SAN_DIR_SFX" TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX" test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY" case "$TRASH_DIRECTORY" in @@ -1447,6 +1451,10 @@ then test_done fi +BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK () { + BAIL_OUT "$1 has no effect except when compiled with SANITIZE=leak" +} + if test -n "$SANITIZE_LEAK" then if test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false @@ -1461,9 +1469,29 @@ then test_done fi fi + + if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false + then + if ! mkdir -p "$TEST_RESULTS_SAN_DIR" + then + BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR" + fi && + TEST_RESULTS_SAN_FILE="$TEST_RESULTS_SAN_DIR/$TEST_RESULTS_SAN_FILE_PFX" + + # Don't litter *.leak dirs if there was nothing to report + test_atexit "rmdir \"$TEST_RESULTS_SAN_DIR\" 2>/dev/null || :" + + prepend_var LSAN_OPTIONS : dedup_token_length=9999 + prepend_var LSAN_OPTIONS : log_exe_name=1 + prepend_var LSAN_OPTIONS : log_path=\"$TEST_RESULTS_SAN_FILE\" + export LSAN_OPTIONS + fi elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false then - BAIL_OUT "GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak" + BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true" +elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false +then + BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true" fi # Last-minute variable setup From fee65b194d0a8cf31b991ea4b9310a70e55ec0b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:35 +0200 Subject: [PATCH 07/15] t/Makefile: don't remove test-results in "clean-except-prove-cache" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "make test" is run with the default of "DEFAULT_TEST_TARGET=test" we'll leave the "test-results" directory in-place, but don't do so for the "prove" target. The reason for this is that when 28d836c8158 (test: allow running the tests under "prove", 2010-10-14) allowed for running the tests under "prove" there was no point in leaving the "test-results" in place. The "prove" target provides its own summary, so we don't need to run "aggregate-results", which is the reason we have "test-results" in the first place. See 2d84e9fb6d2 (Modify test-lib.sh to output stats to t/test-results/*, 2008-06-08). But in a subsequent commit test-lib.sh will start emitting reports of memory leaks in test-results/*, and it will be useful to analyze these after the fact. This wouldn't be a problem as failing tests will halt the removal of the files (we'll never reach "clean-except-prove-cache" from the "prove" target), but will be subsequently as we'll want to report a successful run, but might still have e.g. logs of known memory leaks in test-results/*. So let's stop removing this, it's sufficient that "make clean" removes it, and that "pre-clean" (which both "test" and "prove" depend on) will remove it, i.e. we'll never have a stale "test-results" because of this change. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- contrib/scalar/t/Makefile | 2 +- contrib/subtree/t/Makefile | 2 +- t/Makefile | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile index 01e82e56d1..1ed174a8cf 100644 --- a/contrib/scalar/t/Makefile +++ b/contrib/scalar/t/Makefile @@ -42,7 +42,7 @@ $(T): @echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS) clean-except-prove-cache: - $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)' + $(RM) -r 'trash directory'.* $(RM) -r valgrind/bin clean: clean-except-prove-cache diff --git a/contrib/subtree/t/Makefile b/contrib/subtree/t/Makefile index 276898eb6b..3d278bb0ed 100644 --- a/contrib/subtree/t/Makefile +++ b/contrib/subtree/t/Makefile @@ -47,7 +47,7 @@ pre-clean: $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' clean-except-prove-cache: - $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)' + $(RM) -r 'trash directory'.* $(RM) -r valgrind/bin clean: clean-except-prove-cache diff --git a/t/Makefile b/t/Makefile index 7f56e52f76..1c80c0c79a 100644 --- a/t/Makefile +++ b/t/Makefile @@ -62,7 +62,7 @@ pre-clean: $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' clean-except-prove-cache: clean-chainlint - $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)' + $(RM) -r 'trash directory'.* $(RM) -r valgrind/bin clean: clean-except-prove-cache From 64f3f5a3f671e04150c900e447d8c858c25bad8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:36 +0200 Subject: [PATCH 08/15] tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the original "perl -MTest::More" prerequisite check was added in [1] it's been copy/pasted in [2], [3] and [4]. As we'll be changing these codepaths in a subsequent commit let's consolidate these. While we're at it let's move these to a lazy prereq, and make them conform to our usual coding style (e.g. "\nthen", not "; then"). 1. e46f9c8161a (t9700: skip when Test::More is not available, 2008-06-29) 2. 5e9637c6297 (i18n: add infrastructure for translating Git with gettext, 2011-11-18) 3. 8d314d7afec (send-email: reduce dependencies impact on parse_address_line, 2015-07-07) 4. f07eeed123b (git-credential-netrc: adapt to test framework for git, 2018-05-12) Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- .../netrc/t-git-credential-netrc.sh | 11 ++--------- t/lib-perl.sh | 19 +++++++++++++++++++ t/t0202-gettext-perl.sh | 12 ++---------- t/t9700-perl-git.sh | 11 ++--------- 4 files changed, 25 insertions(+), 28 deletions(-) create mode 100644 t/lib-perl.sh diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh b/contrib/credential/netrc/t-git-credential-netrc.sh index 07227d0228..ff17a9460c 100755 --- a/contrib/credential/netrc/t-git-credential-netrc.sh +++ b/contrib/credential/netrc/t-git-credential-netrc.sh @@ -3,16 +3,9 @@ cd ../../../t test_description='git-credential-netrc' . ./test-lib.sh + . "$TEST_DIRECTORY"/lib-perl.sh - if ! test_have_prereq PERL; then - skip_all='skipping perl interface tests, perl not available' - test_done - fi - - perl -MTest::More -e 0 2>/dev/null || { - skip_all="Perl Test::More unavailable, skipping test" - test_done - } + skip_all_if_no_Test_More # set up test repository diff --git a/t/lib-perl.sh b/t/lib-perl.sh new file mode 100644 index 0000000000..d0bf509a16 --- /dev/null +++ b/t/lib-perl.sh @@ -0,0 +1,19 @@ +# Copyright (c) 2022 Ævar Arnfjörð Bjarmason + +test_lazy_prereq PERL_TEST_MORE ' + perl -MTest::More -e 0 +' + +skip_all_if_no_Test_More () { + if ! test_have_prereq PERL + then + skip_all='skipping perl interface tests, perl not available' + test_done + fi + + if ! test_have_prereq PERL_TEST_MORE + then + skip_all="Perl Test::More unavailable, skipping test" + test_done + fi +} diff --git a/t/t0202-gettext-perl.sh b/t/t0202-gettext-perl.sh index df2ea34932..043b190626 100755 --- a/t/t0202-gettext-perl.sh +++ b/t/t0202-gettext-perl.sh @@ -7,16 +7,8 @@ test_description='Perl gettext interface (Git::I18N)' TEST_PASSES_SANITIZE_LEAK=true . ./lib-gettext.sh - -if ! test_have_prereq PERL; then - skip_all='skipping perl interface tests, perl not available' - test_done -fi - -perl -MTest::More -e 0 2>/dev/null || { - skip_all="Perl Test::More unavailable, skipping test" - test_done -} +. "$TEST_DIRECTORY"/lib-perl.sh +skip_all_if_no_Test_More # The external test will outputs its own plan test_external_has_tap=1 diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh index 102c133112..17fc43f6e5 100755 --- a/t/t9700-perl-git.sh +++ b/t/t9700-perl-git.sh @@ -5,16 +5,9 @@ test_description='perl interface (Git.pm)' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-perl.sh -if ! test_have_prereq PERL; then - skip_all='skipping perl interface tests, perl not available' - test_done -fi - -perl -MTest::More -e 0 2>/dev/null || { - skip_all="Perl Test::More unavailable, skipping test" - test_done -} +skip_all_if_no_Test_More # set up test repository From 5beca49a0b1f3c6d05a4437ca037ab2123a2de57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 28 Jul 2022 01:13:37 +0200 Subject: [PATCH 09/15] test-lib: simplify by removing test_external MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the "test_external" function added in [1]. This arguably makes the output of t9700-perl-git.sh and friends worse. But as we'll argue below the trade-off is worth it, since "chaining" to another TAP emitter in test-lib.sh is more trouble than it's worth. The new output of t9700-perl-git.sh is now: $ ./t9700-perl-git.sh ok 1 - set up test repository ok 2 - use t9700/test.pl to test Git.pm # passed all 2 test(s) 1..2 Whereas before this change it would be: $ ./t9700-perl-git.sh ok 1 - set up test repository # run 1: Perl API (perl /home/avar/g/git/t/t9700/test.pl) ok 2 - use Git; [... omitting tests 3..46 from t/t9700/test.pl ...] ok 47 - unquote escape sequences 1..47 # test_external test Perl API was ok # test_external_without_stderr test no stderr: Perl API was ok At the time of its addition supporting "test_external" was easy, but when test-lib.sh itself started to emit TAP in [2] we needed to make everything surrounding the emission of the plan consider "test_external". I added that support in [2] so that we could run: prove ./t9700-perl-git.sh :: -v But since then in [3] the door has been closed on combining $HARNESS_ACTIVE and -v, we'll now just die: $ prove ./t9700-perl-git.sh :: -v Bailout called. Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log So the only use of this has been that *if* we had failure in one of these tests we could e.g. in CI see which test failed based on the test number. Now we'll need to look at the full verbose logs to get that same information. I think this trade-off is acceptable given the reduction in complexity, and it brings these tests in line with other similar tests, e.g. the reftable tests added in [4] will be condensed down to just one test, which invokes the C helper: $ ./t0032-reftable-unittest.sh ok 1 - unittests # passed all 1 test(s) 1..1 It would still be nice to have that ":: -v" form work again, it never *really* worked, but even though we've had edge cases test output screwing up the TAP it mostly worked between d998bd4ab67 and [3], so we may have been overzealous in forbidding it outright. I have local patches which I'm planning to submit sooner than later that get us to that goal, and in a way that isn't buggy. In the meantime getting rid of this special case makes hacking on this area of test-lib.sh easier, as we'll do in subsequent commits. The switch from "perl" to "$PERL_PATH" here is because "perl" is defined as a shell function in the test suite, see a5bf824f3b4 (t: prevent '-x' tracing from interfering with test helpers' stderr, 2018-02-25). On e.g. the OSX CI the "command perl"... will be part of the emitted stderr. 1. fb32c410087 (t/test-lib.sh: add test_external and test_external_without_stderr, 2008-06-19) 2. d998bd4ab67 (test-lib: Make the test_external_* functions TAP-aware, 2010-06-24) 3. 614fe015212 (test-lib: bail out when "-v" used under "prove", 2016-10-22) 4. ef8a6c62687 (reftable: utility functions, 2021-10-07) Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- .../netrc/t-git-credential-netrc.sh | 7 +- t/README | 26 ------ t/t0202-gettext-perl.sh | 10 +-- t/t9700-perl-git.sh | 10 +-- t/test-lib-functions.sh | 89 +------------------ t/test-lib.sh | 40 ++++----- 6 files changed, 28 insertions(+), 154 deletions(-) diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh b/contrib/credential/netrc/t-git-credential-netrc.sh index ff17a9460c..bf2777308a 100755 --- a/contrib/credential/netrc/t-git-credential-netrc.sh +++ b/contrib/credential/netrc/t-git-credential-netrc.sh @@ -13,13 +13,10 @@ 'set up test repository' \ 'git config --add gpg.program test.git-config-gpg' - # The external test will outputs its own plan - test_external_has_tap=1 - export PERL5LIB="$GITPERLLIB" - test_external \ - 'git-credential-netrc' \ + test_expect_success 'git-credential-netrc' ' perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl + ' test_done ) diff --git a/t/README b/t/README index 0664aee7ed..98f69ed13d 100644 --- a/t/README +++ b/t/README @@ -938,32 +938,6 @@ see test-lib-functions.sh for the full list and their options. test_done fi - - test_external []