* [PATCH v2 01/14] test-lib: use $1, not $@ in test_known_broken_{ok,failure}_
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 02/14] test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler Ævar Arnfjörð Bjarmason
` (13 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 7726d1da88a..3f11ce35112 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 () {
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 02/14] test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 01/14] test-lib: use $1, not $@ in test_known_broken_{ok,failure}_ Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 03/14] test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT Ævar Arnfjörð Bjarmason
` (12 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 3f11ce35112..c8c84ef9b14 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"
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 03/14] test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 01/14] test-lib: use $1, not $@ in test_known_broken_{ok,failure}_ Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 02/14] test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 04/14] test-lib: add a --invert-exit-code switch Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 c8c84ef9b14..118720493bb 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"
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 04/14] test-lib: add a --invert-exit-code switch
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 03/14] test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 05/14] t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 17a268ccd1b..502b4bcf9ea 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 118720493bb..31213b5f95b 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 ;;
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 05/14] t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 04/14] test-lib: add a --invert-exit-code switch Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 06/14] test-lib: add a SANITIZE=leak logging mode Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Reword the documentation added in 956d2e4639b (tests: add a test mode
for SANITIZE=leak, run it in CI, 2021-09-23) for brevity, and to avoid
a merge conflict with another topic, per [1].
1. https://lore.kernel.org/git/220719.86lespun6l.gmgdl@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/README | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/t/README b/t/README
index 4f9981cf5e3..7f409392537 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=<boolean> forces split-index mode on the whole
test suite. Accept any boolean values that are accepted by git-config.
-GIT_TEST_PASSING_SANITIZE_LEAK=<boolean> 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=<n>, when set, makes 'protocol.version'
default to n.
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 06/14] test-lib: add a SANITIZE=leak logging mode
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 05/14] t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 07/14] t/Makefile: don't remove test-results in "clean-except-prove-cache" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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/<test-name>.leak" directory.
That new path is consistent with the existing
"test-results/<test-name>.<type>" 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.<pid>" instead of
the default of "trace.<pid>". 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 <avarab@gmail.com>
---
t/README | 5 +++++
t/test-lib.sh | 30 +++++++++++++++++++++++++++++-
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/t/README b/t/README
index 7f409392537..0664aee7ed9 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=<n>, when set, makes 'protocol.version'
default to n.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 31213b5f95b..f5150400c14 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"
+}
+
# skip non-whitelisted tests when compiled with SANITIZE=leak
if test -n "$SANITIZE_LEAK"
then
@@ -1462,9 +1470,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
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 07/14] t/Makefile: don't remove test-results in "clean-except-prove-cache"
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 06/14] test-lib: add a SANITIZE=leak logging mode Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 08/14] tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 01e82e56d15..1ed174a8cf3 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 276898eb6bd..3d278bb0edb 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 7f56e52f767..1c80c0c79a0 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
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 08/14] tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 07/14] t/Makefile: don't remove test-results in "clean-except-prove-cache" Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 09/14] test-lib: simplify by removing test_external Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
.../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 07227d02287..ff17a9460cd 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 00000000000..d0bf509a167
--- /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 df2ea34932b..043b190626c 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 102c133112c..17fc43f6e57 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
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 09/14] test-lib: simplify by removing test_external
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 08/14] tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 10/14] test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
.../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 ff17a9460cd..bf2777308a5 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 0664aee7ed9..98f69ed13d2 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 [<prereq>] <message> <external> <script>
-
- Execute a <script> with an <external> interpreter (like perl). This
- was added for tests like t9700-perl-git.sh which do most of their
- work in an external test script.
-
- test_external \
- 'GitwebCache::*FileCache*' \
- perl "$TEST_DIRECTORY"/t9503/test_cache_interface.pl
-
- If the test is outputting its own TAP you should set the
- test_external_has_tap variable somewhere before calling the first
- test_external* function. See t9700-perl-git.sh for an example.
-
- # The external test will outputs its own plan
- test_external_has_tap=1
-
- - test_external_without_stderr [<prereq>] <message> <external> <script>
-
- Like test_external but fail if there's any output on stderr,
- instead of checking the exit code.
-
- test_external_without_stderr \
- 'Perl API' \
- perl "$TEST_DIRECTORY"/t9700/test.pl
-
- test_expect_code <exit-code> <command>
Run a command and ensure that it exits with the given exit code.
diff --git a/t/t0202-gettext-perl.sh b/t/t0202-gettext-perl.sh
index 043b190626c..5a6f28051bd 100755
--- a/t/t0202-gettext-perl.sh
+++ b/t/t0202-gettext-perl.sh
@@ -10,11 +10,9 @@ TEST_PASSES_SANITIZE_LEAK=true
. "$TEST_DIRECTORY"/lib-perl.sh
skip_all_if_no_Test_More
-# The external test will outputs its own plan
-test_external_has_tap=1
-
-test_external_without_stderr \
- 'Perl Git::I18N API' \
- perl "$TEST_DIRECTORY"/t0202/test.pl
+test_expect_success 'run t0202/test.pl to test Git::I18N.pm' '
+ "$PERL_PATH" "$TEST_DIRECTORY"/t0202/test.pl 2>stderr &&
+ test_must_be_empty stderr
+'
test_done
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 17fc43f6e57..a48fac6f07a 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -43,11 +43,9 @@ test_expect_success \
git config --add test.pathmulti bar
'
-# The external test will outputs its own plan
-test_external_has_tap=1
-
-test_external_without_stderr \
- 'Perl API' \
- perl "$TEST_DIRECTORY"/t9700/test.pl
+test_expect_success 'use t9700/test.pl to test Git.pm' '
+ "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
+ test_must_be_empty stderr
+'
test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6da7273f1d5..869595d4cee 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -633,7 +633,7 @@ test_hook () {
# - Explicitly using test_have_prereq.
#
# - Implicitly by specifying the prerequisite tag in the calls to
-# test_expect_{success,failure} and test_external{,_without_stderr}.
+# test_expect_{success,failure}
#
# The single parameter is the prerequisite tag (a simple word, in all
# capital letters by convention).
@@ -836,93 +836,6 @@ test_expect_success () {
test_finish_
}
-# test_external runs external test scripts that provide continuous
-# test output about their progress, and succeeds/fails on
-# zero/non-zero exit code. It outputs the test output on stdout even
-# in non-verbose mode, and announces the external script with "# run
-# <n>: ..." before running it. When providing relative paths, keep in
-# mind that all scripts run in "trash directory".
-# Usage: test_external description command arguments...
-# Example: test_external 'Perl API' perl ../path/to/test.pl
-test_external () {
- test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 3 ||
- BUG "not 3 or 4 parameters to test_external"
- descr="$1"
- shift
- test_verify_prereq
- export test_prereq
- if ! test_skip "$descr" "$@"
- then
- # Announce the script to reduce confusion about the
- # test output that follows.
- say_color "" "# run $test_count: $descr ($*)"
- # Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG
- # to be able to use them in script
- export TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG
- # Run command; redirect its stderr to &4 as in
- # test_run_, but keep its stdout on our stdout even in
- # non-verbose mode.
- "$@" 2>&4
- if test "$?" = 0
- then
- if test $test_external_has_tap -eq 0; then
- test_ok_ "$descr"
- else
- say_color "" "# test_external test $descr was ok"
- test_success=$(($test_success + 1))
- fi
- else
- if test $test_external_has_tap -eq 0; then
- test_failure_ "$descr" "$@"
- else
- say_color error "# test_external test $descr failed: $@"
- test_failure=$(($test_failure + 1))
- fi
- fi
- fi
-}
-
-# Like test_external, but in addition tests that the command generated
-# no output on stderr.
-test_external_without_stderr () {
- # The temporary file has no (and must have no) security
- # implications.
- tmp=${TMPDIR:-/tmp}
- stderr="$tmp/git-external-stderr.$$.tmp"
- test_external "$@" 4> "$stderr"
- test -f "$stderr" || error "Internal error: $stderr disappeared."
- descr="no stderr: $1"
- shift
- say >&3 "# expecting no stderr from previous command"
- if test ! -s "$stderr"
- then
- rm "$stderr"
-
- if test $test_external_has_tap -eq 0; then
- test_ok_ "$descr"
- else
- say_color "" "# test_external_without_stderr test $descr was ok"
- test_success=$(($test_success + 1))
- fi
- else
- if test "$verbose" = t
- then
- output=$(echo; echo "# Stderr is:"; cat "$stderr")
- else
- output=
- fi
- # rm first in case test_failure exits.
- rm "$stderr"
- if test $test_external_has_tap -eq 0; then
- test_failure_ "$descr" "$@" "$output"
- else
- say_color error "# test_external_without_stderr test $descr failed: $@: $output"
- test_failure=$(($test_failure + 1))
- fi
- fi
-}
-
# debugging-friendly alternatives to "test [-f|-d|-e]"
# The commands test the existence or non-existence of $1
test_path_is_file () {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index f5150400c14..453d980bcf7 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1231,27 +1231,24 @@ test_done () {
fi
case "$test_failure" in
0)
- if test $test_external_has_tap -eq 0
+ if test $test_remaining -gt 0
then
- if test $test_remaining -gt 0
- then
- say_color pass "# passed all $msg"
- fi
-
- # Maybe print SKIP message
- test -z "$skip_all" || skip_all="# SKIP $skip_all"
- case "$test_count" in
- 0)
- say "1..$test_count${skip_all:+ $skip_all}"
- ;;
- *)
- test -z "$skip_all" ||
- say_color warn "$skip_all"
- say "1..$test_count"
- ;;
- esac
+ say_color pass "# passed all $msg"
fi
+ # Maybe print SKIP message
+ test -z "$skip_all" || skip_all="# SKIP $skip_all"
+ case "$test_count" in
+ 0)
+ say "1..$test_count${skip_all:+ $skip_all}"
+ ;;
+ *)
+ test -z "$skip_all" ||
+ say_color warn "$skip_all"
+ say "1..$test_count"
+ ;;
+ esac
+
if test -n "$stress" && test -n "$invert_exit_code"
then
# We're about to move our "$TRASH_DIRECTORY"
@@ -1286,11 +1283,8 @@ test_done () {
exit 0 ;;
*)
- if test $test_external_has_tap -eq 0
- then
- say_color error "# failed $test_failure among $msg"
- say "1..$test_count"
- fi
+ say_color error "# failed $test_failure among $msg"
+ say "1..$test_count"
if test -n "$invert_exit_code"
then
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 10/14] test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 09/14] test-lib: simplify by removing test_external Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 11/14] test-lib: have the "check" mode for SANITIZE=leak consider leak logs Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Add a new "GIT_TEST_PASSING_SANITIZE_LEAK=check" mode to the
test-lib.sh.
As noted in the updated "t/README" this compliments the existing
"GIT_TEST_PASSING_SANITIZE_LEAK=true" mode added in
956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in CI,
2021-09-23).
Rather than document this all in one (even more) dense paragraph split
up the discussion of how it combines with --immediate into its own
paragraph following the discussion of
"GIT_TEST_SANITIZE_LEAK_LOG=true".
Before the removal of "test_external" in a preceding commit we would
have had to special-case t9700-perl-git.sh and t0202-gettext-perl.sh.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/README | 17 +++++++++++++++++
t/test-lib.sh | 34 ++++++++++++++++++++++++++--------
2 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/t/README b/t/README
index 98f69ed13d2..e13063195e5 100644
--- a/t/README
+++ b/t/README
@@ -371,11 +371,28 @@ 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_PASSING_SANITIZE_LEAK=check checks that our
+"TEST_PASSES_SANITIZE_LEAK=true" markings are current. Rather than
+skipping those tests that haven't set "TEST_PASSES_SANITIZE_LEAK=true"
+before sourcing "test-lib.sh" this mode runs them with
+"--invert-exit-code". This is used to check that there's a one-to-one
+mapping between "TEST_PASSES_SANITIZE_LEAK=true" and those tests that
+pass under "SANITIZE=leak". This is especially useful when testing a
+series that fixes various memory leaks with "git rebase -x".
+
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_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
+will run to completion faster, and result in the same failing
+tests. The only practical reason to run
+GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
+combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
+first failing test case our leak logs won't show subsequent leaks we
+might have run into.
+
GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
default to n.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 453d980bcf7..5785eaf08dc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1452,17 +1452,34 @@ BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK () {
# 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
+ # Normalize with test_bool_env
+ passes_sanitize_leak=
+
+ # We need to see TEST_PASSES_SANITIZE_LEAK in "git
+ # env--helper" (via test_bool_env)
+ export TEST_PASSES_SANITIZE_LEAK
+ if test_bool_env TEST_PASSES_SANITIZE_LEAK false
+ then
+ passes_sanitize_leak=t
+ fi
+
+ if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
then
- # We need to see it in "git env--helper" (via
- # test_bool_env)
- export TEST_PASSES_SANITIZE_LEAK
+ if test -n "$invert_exit_code"
+ then
+ BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=check"
+ fi
- if ! test_bool_env TEST_PASSES_SANITIZE_LEAK false
+ if test -z "$passes_sanitize_leak"
then
- skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
- test_done
+ say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
+ invert_exit_code=t
fi
+ elif test -z "$passes_sanitize_leak" &&
+ test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+ then
+ skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+ test_done
fi
if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
@@ -1481,7 +1498,8 @@ then
prepend_var LSAN_OPTIONS : log_path=\"$TEST_RESULTS_SAN_FILE\"
export LSAN_OPTIONS
fi
-elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
+ test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
then
BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 11/14] test-lib: have the "check" mode for SANITIZE=leak consider leak logs
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 10/14] test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 12/14] leak tests: mark passing SANITIZE=leak tests as leak-free Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
As noted in previous on-list discussions[1] we have various tests that
will falsely report being leak-free because we're missing the relevant
exit code from LSAN as summarized below.
We should fix those issues, but in the meantime and as an additional
sanity check we can and should consider our own ASAN logs before
reporting that a test is leak-free.
Before this compiling with SANITIZE=leak and running:
./t6407-merge-binary.sh
Will exit successfully, now we'll get an error and an informative
message on:
GIT_TEST_SANITIZE_LEAK_LOG=true ./t6407-merge-binary.sh
Even better, as noted in the updated t/README we'll now error out when
combined with the "check" mode:
GIT_TEST_PASSING_SANITIZE_LEAK=check \
GIT_TEST_SANITIZE_LEAK_LOG=true \
./t4058-diff-duplicates.sh
Why do we miss these leaks? Because:
* We have leaks inside "test_expect_failure" blocks, which by design
will not distinguish a "normal" failure from an abort() or
segfault. See [1] for a discussion of it shortcomings.
* We have "git" invocations outside of "test_expect_success",
e.g. setup code in the main body of the test, or in test helper
functions that don't use &&-chaining.
* Our tests will otherwise catch segfaults and abort(), but if we
invoke a command that invokes another command it needs to ferry the
exit code up to us.
Notably a command that e.g. might invoke "git pack-objects" might
itself exit with status 128 if that "pack-objects" segfaults or
abort()'s. If the test invoking the parent command(s) is using
"test_must_fail" we'll consider it an expected "ok" failure.
* run-command.c doesn't (but probably should) ferry up such exit
codes, so for e.g. "git push" tests where we expect a failure and an
underlying "git" command fails we won't ferry up the segfault or
abort exit code.
* We have gitweb.perl and some other perl code ignoring return values
from close(), i.e. ignoring exit codes from "git rev-parse" et al.
* We have in-tree shellscripts like "git-merge-one-file.sh" invoking
git commands, they'll usually return their own exit codes on "git"
failure, rather then ferrying up segfault or abort() exit code.
E.g. these invocations in git-merge-one-file.sh leak, but aren't
reflected in the "git merge" exit code:
src1=$(git unpack-file $2)
src2=$(git unpack-file $3)
That case would be easily "fixed" by adding a line like this after
each assignment:
test $? -ne 0 && exit $?
But we'd then in e.g. "t6407-merge-binary.sh" run into
write_tree_trivial() in "builtin/merge.c" calling die() instead of
ferrying up the relevant exit code.
Let's remove "TEST_PASSES_SANITIZE_LEAK=true" from tests we
were falsely marking as leak-free.
In the case of t6407-merge-binary.sh it was marked as leak-free in
9081a421a6d (checkout: fix "branch info" memory leaks,
2021-11-16). I'd previously removed other bad
"TEST_PASSES_SANITIZE_LEAK=true" opt-ins in the series merged in
ea05fd5fbf7 (Merge branch 'ab/keep-git-exit-codes-in-tests',
2022-03-16). The case of t1060-object-corruption.sh is more subtle,
and will be discussed in a subsequent commit.
1. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/README | 15 +++++++
t/t1060-object-corruption.sh | 1 -
t/t6407-merge-binary.sh | 1 -
t/test-lib.sh | 80 ++++++++++++++++++++++++++++++++++++
4 files changed, 95 insertions(+), 2 deletions(-)
diff --git a/t/README b/t/README
index e13063195e5..2f439f96589 100644
--- a/t/README
+++ b/t/README
@@ -385,6 +385,12 @@ GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
make logs +machine-readable.
+With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
+before exiting and exit on failure if the logs showed that we had a
+memory leak, even if the test itself would have otherwise passed. This
+allows us to catch e.g. missing &&-chaining. This is especially useful
+when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
+
GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
will run to completion faster, and result in the same failing
tests. The only practical reason to run
@@ -393,6 +399,15 @@ combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
first failing test case our leak logs won't show subsequent leaks we
might have run into.
+GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
+leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
+run "git" (or "test-tool" etc.) without properly checking the exit
+code, or git will invoke itself and fail to ferry the abort() exit
+code to the original caller. When the two modes are combined we'll
+look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
+the test run to see if had memory leaks which the test itself didn't
+catch.
+
GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
default to n.
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index 5b8e47e346c..e8a58b15897 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -2,7 +2,6 @@
test_description='see how we handle various forms of corruption'
-TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# convert "1234abcd" to ".git/objects/12/34abcd"
diff --git a/t/t6407-merge-binary.sh b/t/t6407-merge-binary.sh
index 0753fc95f45..e8a28717cec 100755
--- a/t/t6407-merge-binary.sh
+++ b/t/t6407-merge-binary.sh
@@ -5,7 +5,6 @@ test_description='ask merge-recursive to merge binary files'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5785eaf08dc..227a7efa241 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -309,6 +309,7 @@ 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"
+TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP=
TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
@@ -316,6 +317,16 @@ case "$TRASH_DIRECTORY" in
*) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
esac
+# Utility functions using $TEST_RESULTS_* variables
+nr_san_dir_leaks_ () {
+ # stderr piped to /dev/null because the directory may have
+ # been "rmdir"'d already.
+ find "$TEST_RESULTS_SAN_DIR" \
+ -type f \
+ -name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
+ wc -l
+}
+
# If --stress was passed, run this test repeatedly in several parallel loops.
if test "$GIT_TEST_STRESS_STARTED" = "done"
then
@@ -1191,6 +1202,66 @@ test_atexit_handler () {
teardown_malloc_check
}
+sanitize_leak_log_message_ () {
+ local new="$1" &&
+ local old="$2" &&
+ local file="$3" &&
+
+ printf "With SANITIZE=leak at exit we have %d leak logs, but started with %d
+
+This means that we have a blindspot where git is leaking but we're
+losing the exit code somewhere, or not propagating it appropriately
+upwards!
+
+See the logs at \"%s.*\";
+those logs are reproduced below." \
+ "$new" "$old" "$file"
+}
+
+check_test_results_san_file_ () {
+ if test -z "$TEST_RESULTS_SAN_FILE"
+ then
+ return
+ fi &&
+ local old="$TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP" &&
+ local new="$(nr_san_dir_leaks_)" &&
+
+ if test $new -le $old
+ then
+ return
+ fi &&
+ local out="$(sanitize_leak_log_message_ "$new" "$old" "$TEST_RESULTS_SAN_FILE")" &&
+ say_color error "$out" &&
+ if test "$old" != 0
+ then
+ echo &&
+ say_color error "The logs include output from past runs to avoid" &&
+ say_color error "that remove 'test-results' between runs."
+ fi &&
+ say_color error "$(cat "$TEST_RESULTS_SAN_FILE".*)" &&
+
+ if test -n "$passes_sanitize_leak" && test "$test_failure" = 0
+ then
+ say "As TEST_PASSES_SANITIZE_LEAK=true and our logs show we're leaking, exit non-zero!" &&
+ invert_exit_code=t
+ elif test -n "$passes_sanitize_leak"
+ then
+ say "As TEST_PASSES_SANITIZE_LEAK=true and our logs show we're leaking, and we're failing for other reasons too..." &&
+ invert_exit_code=
+ elif test -n "$sanitize_leak_check" && test "$test_failure" = 0
+ then
+ say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
+ invert_exit_code=
+ elif test -n "$sanitize_leak_check"
+ then
+ say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
+ invert_exit_code=t
+ else
+ say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
+ invert_exit_code=t
+ fi
+}
+
test_done () {
# Run the atexit commands _before_ the trash directory is
# removed, so the commands can access pidfiles and socket files.
@@ -1270,6 +1341,8 @@ test_done () {
error "Tests passed but test cleanup failed; aborting"
fi
+ check_test_results_san_file_ "$test_failure"
+
if test -z "$skip_all" && test -n "$invert_exit_code"
then
say_color warn "# faking up non-zero exit with --invert-exit-code"
@@ -1286,6 +1359,8 @@ test_done () {
say_color error "# failed $test_failure among $msg"
say "1..$test_count"
+ check_test_results_san_file_ "$test_failure"
+
if test -n "$invert_exit_code"
then
_invert_exit_code_failure_end_blurb
@@ -1465,6 +1540,7 @@ then
if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
then
+ sanitize_leak_check=t
if test -n "$invert_exit_code"
then
BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=check"
@@ -1490,6 +1566,10 @@ then
fi &&
TEST_RESULTS_SAN_FILE="$TEST_RESULTS_SAN_DIR/$TEST_RESULTS_SAN_FILE_PFX"
+ # In case "test-results" is left over from a previous
+ # run: Only report if new leaks show up.
+ TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP=$(nr_san_dir_leaks_)
+
# Don't litter *.leak dirs if there was nothing to report
test_atexit "rmdir \"$TEST_RESULTS_SAN_DIR\" 2>/dev/null || :"
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 12/14] leak tests: mark passing SANITIZE=leak tests as leak-free
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (10 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 11/14] test-lib: have the "check" mode for SANITIZE=leak consider leak logs Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 13/14] upload-pack: fix a memory leak in create_pack_file() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Mark those remaining tests that pass when run under SANITIZE=leak with
TEST_PASSES_SANITIZE_LEAK=true, these were either omitted in
f346fcb62a0 (Merge branch 'ab/mark-leak-free-tests-even-more',
2021-12-15) and 5a4f8381b68 (Merge branch 'ab/mark-leak-free-tests',
2021-10-25), or have had their memory leaks fixed since then.
With this change there's now a a one-to-one mapping between those
tests that we have opted-in via "TEST_PASSES_SANITIZE_LEAK=true", and
those that pass with the new "check" mode:
GIT_TEST_PASSING_SANITIZE_LEAK=check \
GIT_TEST_SANITIZE_LEAK_LOG=true \
make test SANITIZE=leak
Note that the "GIT_TEST_SANITIZE_LEAK_LOG=true" is needed due to the
edge cases noted in a preceding commit, i.e. in some cases we'd pass
the test itself, but still have outstanding leaks due to ignored exit
codes.
The "GIT_TEST_SANITIZE_LEAK_LOG=true" corrects for that, we're only
marking those tests as passing that really don't have any leaks,
whether that was reflected in their exit code or not.
Note that the change here to "t9100-git-svn-basic.sh" is marking that
test as passing under SANITIZE=leak, we're removing a
"TEST_FAILS_SANITIZE_LEAK=true" line, not
"TEST_PASSES_SANITIZE_LEAK=true". See 7a98d9ab00d (revisions API: have
release_revisions() release "cmdline", 2022-04-13) for the
introduction of that t/lib-git-svn.sh-specific variable.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t0027-auto-crlf.sh | 1 +
t/t0032-reftable-unittest.sh | 1 +
t/t0033-safe-directory.sh | 1 +
t/t0050-filesystem.sh | 1 +
t/t0095-bloom.sh | 2 ++
t/t1405-main-ref-store.sh | 1 +
t/t1407-worktree-ref-store.sh | 1 +
t/t1418-reflog-exists.sh | 1 +
t/t1701-racy-split-index.sh | 1 +
t/t2006-checkout-index-basic.sh | 1 +
t/t2023-checkout-m.sh | 1 +
t/t2205-add-worktree-config.sh | 1 +
t/t3012-ls-files-dedup.sh | 1 +
t/t4017-diff-retval.sh | 1 +
t/t4051-diff-function-context.sh | 1 +
t/t4057-diff-combined-paths.sh | 1 +
t/t4114-apply-typechange.sh | 1 +
t/t4301-merge-tree-write-tree.sh | 1 +
t/t5315-pack-objects-compression.sh | 1 +
t/t5351-unpack-large-objects.sh | 1 +
t/t5402-post-merge-hook.sh | 1 +
t/t5503-tagfollow.sh | 1 +
t/t6404-recursive-merge.sh | 1 +
t/t6405-merge-symlinks.sh | 1 +
t/t6408-merge-up-to-date.sh | 1 +
t/t6411-merge-filemode.sh | 1 +
t/t6413-merge-crlf.sh | 1 +
t/t6415-merge-dir-to-symlink.sh | 1 +
t/t6425-merge-rename-delete.sh | 1 +
t/t6431-merge-criscross.sh | 1 +
t/t7060-wtstatus.sh | 1 +
t/t7062-wtstatus-ignorecase.sh | 1 +
t/t7110-reset-merge.sh | 1 +
t/t7111-reset-table.sh | 1 +
t/t7609-mergetool--lib.sh | 1 +
t/t9100-git-svn-basic.sh | 1 -
t/t9700-perl-git.sh | 2 ++
t/t9901-git-web--browse.sh | 1 +
38 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 7f80f463930..a22e0e1382c 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -2,6 +2,7 @@
test_description='CRLF conversion all combinations'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
compare_files () {
diff --git a/t/t0032-reftable-unittest.sh b/t/t0032-reftable-unittest.sh
index 0ed14971a58..471cb37ac28 100755
--- a/t/t0032-reftable-unittest.sh
+++ b/t/t0032-reftable-unittest.sh
@@ -5,6 +5,7 @@
test_description='reftable unittests'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'unittests' '
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 3908597d42d..7db3344092d 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -2,6 +2,7 @@
test_description='verify safe.directory checks'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
GIT_TEST_ASSUME_DIFFERENT_OWNER=1
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index 5c9dc90d0b0..325eb1c3cd0 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -5,6 +5,7 @@ test_description='Various filesystem issues'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
auml=$(printf '\303\244')
diff --git a/t/t0095-bloom.sh b/t/t0095-bloom.sh
index daeb4a5e3e7..b567383eb83 100755
--- a/t/t0095-bloom.sh
+++ b/t/t0095-bloom.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='Testing the various Bloom filter computations in bloom.c'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'compute unseeded murmur3 hash for empty string' '
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 51f82916281..e4627cf1b61 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -5,6 +5,7 @@ test_description='test main ref store api'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
RUN="test-tool ref-store main"
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index ad8006c8139..05b1881c591 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -5,6 +5,7 @@ test_description='test worktree ref store api'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
RWT="test-tool ref-store worktree:wt"
diff --git a/t/t1418-reflog-exists.sh b/t/t1418-reflog-exists.sh
index d51ecd5e925..2268bca3c11 100755
--- a/t/t1418-reflog-exists.sh
+++ b/t/t1418-reflog-exists.sh
@@ -4,6 +4,7 @@ test_description='Test reflog display routines'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh
index 5dc221ef382..d8fa489998a 100755
--- a/t/t1701-racy-split-index.sh
+++ b/t/t1701-racy-split-index.sh
@@ -5,6 +5,7 @@
test_description='racy split index'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
index 7705e3a3170..5d119871d41 100755
--- a/t/t2006-checkout-index-basic.sh
+++ b/t/t2006-checkout-index-basic.sh
@@ -3,6 +3,7 @@
test_description='basic checkout-index tests
'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'checkout-index --gobbledegook' '
diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh
index 7b327b75449..81e772fb4eb 100755
--- a/t/t2023-checkout-m.sh
+++ b/t/t2023-checkout-m.sh
@@ -7,6 +7,7 @@ Ensures that checkout -m on a resolved file restores the conflicted file'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t2205-add-worktree-config.sh b/t/t2205-add-worktree-config.sh
index 43d950de640..98265ba1b49 100755
--- a/t/t2205-add-worktree-config.sh
+++ b/t/t2205-add-worktree-config.sh
@@ -17,6 +17,7 @@ outside the repository. Two instances for which this can occur are tested:
repository can be added to the index.
'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success '1a: setup--config worktree' '
diff --git a/t/t3012-ls-files-dedup.sh b/t/t3012-ls-files-dedup.sh
index 2682b1f43a6..190e2f6eed7 100755
--- a/t/t3012-ls-files-dedup.sh
+++ b/t/t3012-ls-files-dedup.sh
@@ -2,6 +2,7 @@
test_description='git ls-files --deduplicate test'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index ed461f481e2..5bc28ad9f04 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -5,6 +5,7 @@ test_description='Return value of diffs'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 4838a1df8b4..725278ad19c 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -2,6 +2,7 @@
test_description='diff function context'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
dir="$TEST_DIRECTORY/t4051"
diff --git a/t/t4057-diff-combined-paths.sh b/t/t4057-diff-combined-paths.sh
index 04b8a1542a8..9a7505cbb8b 100755
--- a/t/t4057-diff-combined-paths.sh
+++ b/t/t4057-diff-combined-paths.sh
@@ -5,6 +5,7 @@ test_description='combined diff show only paths that are different to all parent
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# verify that diffc.expect matches output of
diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh
index da3e64f8110..8ff36407667 100755
--- a/t/t4114-apply-typechange.sh
+++ b/t/t4114-apply-typechange.sh
@@ -7,6 +7,7 @@ test_description='git apply should not get confused with type changes.
'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup repository and commits' '
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index f091259a55e..a243e3c5176 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -2,6 +2,7 @@
test_description='git merge-tree --write-tree'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# This test is ort-specific
diff --git a/t/t5315-pack-objects-compression.sh b/t/t5315-pack-objects-compression.sh
index 8bacd96275b..c80ea9e8b71 100755
--- a/t/t5315-pack-objects-compression.sh
+++ b/t/t5315-pack-objects-compression.sh
@@ -2,6 +2,7 @@
test_description='pack-object compression configuration'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t5351-unpack-large-objects.sh b/t/t5351-unpack-large-objects.sh
index 8ce8aa3b147..b1703350be3 100755
--- a/t/t5351-unpack-large-objects.sh
+++ b/t/t5351-unpack-large-objects.sh
@@ -5,6 +5,7 @@
test_description='git unpack-objects with large objects'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
prepare_dest () {
diff --git a/t/t5402-post-merge-hook.sh b/t/t5402-post-merge-hook.sh
index 915af2de95e..46ebdfbeeba 100755
--- a/t/t5402-post-merge-hook.sh
+++ b/t/t5402-post-merge-hook.sh
@@ -7,6 +7,7 @@ test_description='Test the post-merge hook.'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 195fc64dd44..5ebbaa48968 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -5,6 +5,7 @@ test_description='test automatic tag following'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# End state of the repository:
diff --git a/t/t6404-recursive-merge.sh b/t/t6404-recursive-merge.sh
index b8735c6db4d..36215518b6e 100755
--- a/t/t6404-recursive-merge.sh
+++ b/t/t6404-recursive-merge.sh
@@ -4,6 +4,7 @@ test_description='Test merge without common ancestors'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# This scenario is based on a real-world repository of Shawn Pearce.
diff --git a/t/t6405-merge-symlinks.sh b/t/t6405-merge-symlinks.sh
index 7435fce71e0..29e2b25ce5d 100755
--- a/t/t6405-merge-symlinks.sh
+++ b/t/t6405-merge-symlinks.sh
@@ -11,6 +11,7 @@ if core.symlinks is false.'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t6408-merge-up-to-date.sh b/t/t6408-merge-up-to-date.sh
index 7763c1ba980..8a1ba6d23a7 100755
--- a/t/t6408-merge-up-to-date.sh
+++ b/t/t6408-merge-up-to-date.sh
@@ -2,6 +2,7 @@
test_description='merge fast-forward and up to date'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t6411-merge-filemode.sh b/t/t6411-merge-filemode.sh
index 6ae2489286c..b6182723aae 100755
--- a/t/t6411-merge-filemode.sh
+++ b/t/t6411-merge-filemode.sh
@@ -4,6 +4,7 @@ test_description='merge: handle file mode'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'set up mode change in one branch' '
diff --git a/t/t6413-merge-crlf.sh b/t/t6413-merge-crlf.sh
index affea255fe9..b4f4a313f48 100755
--- a/t/t6413-merge-crlf.sh
+++ b/t/t6413-merge-crlf.sh
@@ -11,6 +11,7 @@ test_description='merge conflict in crlf repo
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t6415-merge-dir-to-symlink.sh b/t/t6415-merge-dir-to-symlink.sh
index 2655e295f5a..ae00492c768 100755
--- a/t/t6415-merge-dir-to-symlink.sh
+++ b/t/t6415-merge-dir-to-symlink.sh
@@ -4,6 +4,7 @@ test_description='merging when a directory was replaced with a symlink'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'create a commit where dir a/b changed to symlink' '
diff --git a/t/t6425-merge-rename-delete.sh b/t/t6425-merge-rename-delete.sh
index 459b431a60d..93cd2869b12 100755
--- a/t/t6425-merge-rename-delete.sh
+++ b/t/t6425-merge-rename-delete.sh
@@ -4,6 +4,7 @@ test_description='Merge-recursive rename/delete conflict message'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'rename/delete' '
diff --git a/t/t6431-merge-criscross.sh b/t/t6431-merge-criscross.sh
index 3824756a02e..3fe14cd73e8 100755
--- a/t/t6431-merge-criscross.sh
+++ b/t/t6431-merge-criscross.sh
@@ -2,6 +2,7 @@
test_description='merge-recursive backend test'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# A <- create some files
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 0f4344c55e6..aaeb4a53344 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -5,6 +5,7 @@ test_description='basic work tree status reporting'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t7062-wtstatus-ignorecase.sh b/t/t7062-wtstatus-ignorecase.sh
index 73709dbeee2..caf372a3d42 100755
--- a/t/t7062-wtstatus-ignorecase.sh
+++ b/t/t7062-wtstatus-ignorecase.sh
@@ -2,6 +2,7 @@
test_description='git-status with core.ignorecase=true'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'status with hash collisions' '
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 3d62e10b53f..eb881be95b6 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -5,6 +5,7 @@
test_description='Tests for "git reset" with "--merge" and "--keep" options'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t7111-reset-table.sh b/t/t7111-reset-table.sh
index ce421ad5ac4..78f25c1c7ea 100755
--- a/t/t7111-reset-table.sh
+++ b/t/t7111-reset-table.sh
@@ -5,6 +5,7 @@
test_description='Tests to check that "reset" options follow a known table'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
diff --git a/t/t7609-mergetool--lib.sh b/t/t7609-mergetool--lib.sh
index 330d6d603d7..8b1c3bd39f2 100755
--- a/t/t7609-mergetool--lib.sh
+++ b/t/t7609-mergetool--lib.sh
@@ -4,6 +4,7 @@ test_description='git mergetool
Testing basic merge tools options'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'mergetool --tool=vimdiff creates the expected layout' '
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 7c5b847f584..fea41b3c360 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -8,7 +8,6 @@ test_description='git svn basic tests'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-TEST_FAILS_SANITIZE_LEAK=true
. ./lib-git-svn.sh
prepare_utf8_locale
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index a48fac6f07a..4aa5d90d328 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -4,6 +4,8 @@
#
test_description='perl interface (Git.pm)'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-perl.sh
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
index de7152f8271..19f56e5680f 100755
--- a/t/t9901-git-web--browse.sh
+++ b/t/t9901-git-web--browse.sh
@@ -5,6 +5,7 @@ test_description='git web--browse basic tests
This test checks that git web--browse can handle various valid URLs.'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_web_browse () {
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 13/14] upload-pack: fix a memory leak in create_pack_file()
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (11 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 12/14] leak tests: mark passing SANITIZE=leak tests as leak-free Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-20 21:21 ` [PATCH v2 14/14] CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaks Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Fix a memory leak that's been reported by some versions of "gcc" since
"output_state" became malloc'd in 55a9651d26a (upload-pack.c: increase
output buffer size, 2021-12-14).
In e75d2f7f734 (revisions API: have release_revisions() release
"filter", 2022-04-13) it was correctly marked as leak-free, the only
path through this function that doesn't reach the free(output_state)
is if we "goto fail", and that will invoke "die()".
Such leaks are not included with SANITIZE=leak (but e.g. valgrind will
still report them), but under some gcc optimization (I have not been
able to reproduce it with "clang") we'll report a leak here
anyway. E.g. gcc v12 with "-O2" and above will trigger it, but not
clang v13 with any "-On".
The GitHub CI would also run into this leak if the "linux-leaks" job
was made to run with "GIT_TEST_SANITIZE_LEAK_LOG=true".
See [1] for a past case where gcc had similar trouble analyzing leaks
involving a die() invocation in the function.
1. https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t1060-object-corruption.sh | 1 +
upload-pack.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index e8a58b15897..5b8e47e346c 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -2,6 +2,7 @@
test_description='see how we handle various forms of corruption'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# convert "1234abcd" to ".git/objects/12/34abcd"
diff --git a/upload-pack.c b/upload-pack.c
index 3a851b36066..b3884d3f4de 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -455,6 +455,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
return;
fail:
+ free(output_state);
send_client_data(3, abort_msg, sizeof(abort_msg),
pack_data->use_sideband);
die("git upload-pack: %s", abort_msg);
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v2 14/14] CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaks
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (12 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 13/14] upload-pack: fix a memory leak in create_pack_file() Ævar Arnfjörð Bjarmason
@ 2022-07-20 21:21 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-20 21:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
As noted in a preceding commit the leak checking done by
"GIT_TEST_PASSING_SANITIZE_LEAK=true" (added in [1]) is incomplete
without combining it with "GIT_TEST_SANITIZE_LEAK_LOG=true".
Let's run our CI with that, to ensure that we catch cases where our
tests are missing the abort() exit code resulting from a leak for
whatever reason. The reasons for that are discussed in detail in a
preceding commit.
1. 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in
CI, 2021-09-23)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
ci/lib.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/ci/lib.sh b/ci/lib.sh
index f095519f8db..1b0cc2b57db 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -276,6 +276,7 @@ linux-musl)
linux-leaks)
export SANITIZE=leak
export GIT_TEST_PASSING_SANITIZE_LEAK=true
+ export GIT_TEST_SANITIZE_LEAK_LOG=true
;;
esac
--
2.37.1.1064.gc96144cf387
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests
2022-07-20 21:21 ` [PATCH v2 00/14] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (13 preceding siblings ...)
2022-07-20 21:21 ` [PATCH v2 14/14] CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaks Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 01/15] test-lib: use $1, not $@ in test_known_broken_{ok,failure}_ Ævar Arnfjörð Bjarmason
` (14 more replies)
14 siblings, 15 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
This series adds a "check" leak-testing mode, which allows us to
assert that the tests we mark as leak free and those that don't leak
are one-to-one mapped to one another.
Changes since v2:
* Should fix linux-leaks CI failures when merged with "seen"
* Rebase on landed topic(s), including updating the mention of the
now-landed whitelist/blacklist topic.
* Stopped marking t6415 as leak-free. GCC at some versions and higher
optimization levels in some versions will spot that it leaks.
* A new commit to undo various !SANITIZE_LEAK prereqs, for leaks that
have since been solved.
* Note that the v2 of this marked t1500 as leak-free, but another
topic added a leak there. That's since been fixed:
20220726210020.3397249-1-calvinwan@google.com
1. https://lore.kernel.org/git/cover-v2-00.14-00000000000-20220720T211221Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (15):
test-lib: use $1, not $@ in test_known_broken_{ok,failure}_
test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler
test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT
test-lib: add a --invert-exit-code switch
t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description
test-lib: add a SANITIZE=leak logging mode
t/Makefile: don't remove test-results in "clean-except-prove-cache"
tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh
test-lib: simplify by removing test_external
test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode
test-lib: have the "check" mode for SANITIZE=leak consider leak logs
leak tests: don't skip some tests under SANITIZE=leak
leak tests: mark passing SANITIZE=leak tests as leak-free
upload-pack: fix a memory leak in create_pack_file()
CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaks
ci/lib.sh | 1 +
.../netrc/t-git-credential-netrc.sh | 18 +-
contrib/scalar/t/Makefile | 2 +-
contrib/subtree/t/Makefile | 2 +-
t/Makefile | 2 +-
t/README | 73 ++---
t/lib-perl.sh | 19 ++
t/t0000-basic.sh | 72 +++++
t/t0002-gitfile.sh | 2 +-
t/t0004-unwritable.sh | 2 +-
t/t0027-auto-crlf.sh | 1 +
t/t0032-reftable-unittest.sh | 1 +
t/t0033-safe-directory.sh | 1 +
t/t0050-filesystem.sh | 1 +
t/t0095-bloom.sh | 2 +
t/t0202-gettext-perl.sh | 22 +-
t/t1405-main-ref-store.sh | 1 +
t/t1407-worktree-ref-store.sh | 1 +
t/t1418-reflog-exists.sh | 1 +
t/t1503-rev-parse-verify.sh | 2 +-
t/t1701-racy-split-index.sh | 1 +
t/t2006-checkout-index-basic.sh | 1 +
t/t2023-checkout-m.sh | 1 +
t/t2205-add-worktree-config.sh | 1 +
t/t3001-ls-files-others-exclude.sh | 4 +-
t/t3012-ls-files-dedup.sh | 1 +
t/t3305-notes-fanout.sh | 2 +-
t/t4017-diff-retval.sh | 1 +
t/t4020-diff-external.sh | 4 +-
t/t4051-diff-function-context.sh | 1 +
t/t4057-diff-combined-paths.sh | 1 +
t/t4114-apply-typechange.sh | 1 +
t/t4301-merge-tree-write-tree.sh | 1 +
t/t5315-pack-objects-compression.sh | 1 +
t/t5351-unpack-large-objects.sh | 1 +
t/t5402-post-merge-hook.sh | 1 +
t/t5503-tagfollow.sh | 1 +
t/t6102-rev-list-unexpected-objects.sh | 4 +-
t/t6404-recursive-merge.sh | 1 +
t/t6405-merge-symlinks.sh | 1 +
t/t6407-merge-binary.sh | 1 -
t/t6408-merge-up-to-date.sh | 1 +
t/t6411-merge-filemode.sh | 1 +
t/t6413-merge-crlf.sh | 1 +
t/t6425-merge-rename-delete.sh | 1 +
t/t6431-merge-criscross.sh | 1 +
t/t7060-wtstatus.sh | 1 +
t/t7062-wtstatus-ignorecase.sh | 1 +
t/t7110-reset-merge.sh | 1 +
t/t7111-reset-table.sh | 1 +
t/t7609-mergetool--lib.sh | 1 +
t/t9100-git-svn-basic.sh | 1 -
t/t9700-perl-git.sh | 23 +-
t/t9901-git-web--browse.sh | 1 +
t/test-lib-functions.sh | 89 +------
t/test-lib.sh | 250 ++++++++++++++----
upload-pack.c | 1 +
57 files changed, 405 insertions(+), 227 deletions(-)
create mode 100644 t/lib-perl.sh
Range-diff against v2:
1: e53cf647b44 = 1: 65930cc1445 test-lib: use $1, not $@ in test_known_broken_{ok,failure}_
2: 00af775bd0d = 2: 242f3c9ed20 test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler
3: 419bc2c6a6e = 3: 385d5c52d5a test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT
4: 668c25f4d7e = 4: f3144af2190 test-lib: add a --invert-exit-code switch
5: a26cb02db0a ! 5: 80a36a47b37 t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description
@@ Commit message
t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description
Reword the documentation added in 956d2e4639b (tests: add a test mode
- for SANITIZE=leak, run it in CI, 2021-09-23) for brevity, and to avoid
- a merge conflict with another topic, per [1].
+ for SANITIZE=leak, run it in CI, 2021-09-23) for brevity.
- 1. https://lore.kernel.org/git/220719.86lespun6l.gmgdl@evledraar.gmail.com/
+ 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 <avarab@gmail.com>
@@ t/README: excluded as so much relies on it, but this might change in the future.
GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
default to n.
+
+ ## t/test-lib.sh ##
+@@ t/test-lib.sh: 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
6: f1acf762899 ! 6: 4f9b2396c15 test-lib: add a SANITIZE=leak logging mode
@@ t/test-lib.sh: then
+ BAIL_OUT "$1 has no effect except when compiled with SANITIZE=leak"
+}
+
- # 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
@@ t/test-lib.sh: then
test_done
fi
7: 0723e90df7b = 7: c7ea6dc013b t/Makefile: don't remove test-results in "clean-except-prove-cache"
8: 987d9d0e98c = 8: 53ea78bf417 tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh
9: 20bd31615e4 = 9: 871d266f435 test-lib: simplify by removing test_external
10: 78a47d2b348 ! 10: 32564b8fcb3 test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode
@@ t/README: declared themselves as leak-free by setting
## t/test-lib.sh ##
@@ t/test-lib.sh: BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK () {
- # 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
11: 8cc6ab390db = 11: fb7e5e1b4cb test-lib: have the "check" mode for SANITIZE=leak consider leak logs
-: ----------- > 12: 6df9078e266 leak tests: don't skip some tests under SANITIZE=leak
12: e3c8909207b ! 13: 28255ac3239 leak tests: mark passing SANITIZE=leak tests as leak-free
@@ t/t6413-merge-crlf.sh: test_description='merge conflict in crlf repo
test_expect_success setup '
- ## t/t6415-merge-dir-to-symlink.sh ##
-@@ t/t6415-merge-dir-to-symlink.sh: test_description='merging when a directory was replaced with a symlink'
- GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
- export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-
-+TEST_PASSES_SANITIZE_LEAK=true
- . ./test-lib.sh
-
- test_expect_success 'create a commit where dir a/b changed to symlink' '
-
## t/t6425-merge-rename-delete.sh ##
@@ t/t6425-merge-rename-delete.sh: test_description='Merge-recursive rename/delete conflict message'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
13: 07b6572aea9 = 14: c1436269c64 upload-pack: fix a memory leak in create_pack_file()
14: eaa35d1bc59 = 15: e4acdb18ec2 CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaks
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply [flat|nested] 57+ messages in thread* [PATCH v3 01/15] test-lib: use $1, not $@ in test_known_broken_{ok,failure}_
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 02/15] test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler Ævar Arnfjörð Bjarmason
` (13 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 7726d1da88a..3f11ce35112 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 () {
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 02/15] test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 01/15] test-lib: use $1, not $@ in test_known_broken_{ok,failure}_ Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 03/15] test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT Ævar Arnfjörð Bjarmason
` (12 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 3f11ce35112..c8c84ef9b14 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"
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 03/15] test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 01/15] test-lib: use $1, not $@ in test_known_broken_{ok,failure}_ Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 02/15] test-lib: don't set GIT_EXIT_OK before calling test_atexit_handler Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 04/15] test-lib: add a --invert-exit-code switch Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 c8c84ef9b14..118720493bb 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"
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 04/15] test-lib: add a --invert-exit-code switch
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 03/15] test-lib: fix GIT_EXIT_OK logic errors, use BAIL_OUT Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 05/15] t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 17a268ccd1b..502b4bcf9ea 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 118720493bb..31213b5f95b 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 ;;
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 05/15] t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 04/15] test-lib: add a --invert-exit-code switch Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 06/15] test-lib: add a SANITIZE=leak logging mode Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
t/README | 10 ++++------
t/test-lib.sh | 1 -
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/t/README b/t/README
index 4f9981cf5e3..7f409392537 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=<boolean> forces split-index mode on the whole
test suite. Accept any boolean values that are accepted by git-config.
-GIT_TEST_PASSING_SANITIZE_LEAK=<boolean> 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=<n>, when set, makes 'protocol.version'
default to n.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 31213b5f95b..f8adb92f027 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
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 06/15] test-lib: add a SANITIZE=leak logging mode
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 05/15] t/README: reword the "GIT_TEST_PASSING_SANITIZE_LEAK" description Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 07/15] t/Makefile: don't remove test-results in "clean-except-prove-cache" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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/<test-name>.leak" directory.
That new path is consistent with the existing
"test-results/<test-name>.<type>" 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.<pid>" instead of
the default of "trace.<pid>". 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 <avarab@gmail.com>
---
t/README | 5 +++++
t/test-lib.sh | 30 +++++++++++++++++++++++++++++-
2 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/t/README b/t/README
index 7f409392537..0664aee7ed9 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=<n>, when set, makes 'protocol.version'
default to n.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index f8adb92f027..557f77c9717 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
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 07/15] t/Makefile: don't remove test-results in "clean-except-prove-cache"
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 06/15] test-lib: add a SANITIZE=leak logging mode Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-09-20 10:54 ` [PATCH] t/Makefile: remove 'test-results' on 'make clean' SZEDER Gábor
2022-07-27 23:13 ` [PATCH v3 08/15] tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
14 siblings, 1 reply; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 01e82e56d15..1ed174a8cf3 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 276898eb6bd..3d278bb0edb 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 7f56e52f767..1c80c0c79a0 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
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH] t/Makefile: remove 'test-results' on 'make clean'
2022-07-27 23:13 ` [PATCH v3 07/15] t/Makefile: don't remove test-results in "clean-except-prove-cache" Ævar Arnfjörð Bjarmason
@ 2022-09-20 10:54 ` SZEDER Gábor
2022-09-20 19:51 ` Jeff King
2022-09-20 20:16 ` [PATCH v2] " SZEDER Gábor
0 siblings, 2 replies; 57+ messages in thread
From: SZEDER Gábor @ 2022-09-20 10:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Ævar Arnfjörð Bjarmason, SZEDER Gábor
The 't/test-results' directory and its contents are by-products of the
test process, so 'make clean' should remove them, but, alas, this has
been broken since ee65b194d (t/Makefile: don't remove test-results in
"clean-except-prove-cache", 2022-07-28).
The 'clean' target in 't/Makefile' was not directly responsible for
removing the 'test-results' directory, but relied on its dependency
'clean-except-prove-cache' to do that [1]. ee65b194d broke this,
because it only removed the 'rm -r test-results' command from the
'clean-except-prove-cache' target instead of moving it to the 'clean'
target, resulting in stray 't/test-results' directories.
Add that missing cleanup command to 't/Makefile', and all sub-Makefiles
touched by ee65b194d as well.
[1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs,
2012-05-02)
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
contrib/scalar/t/Makefile | 1 +
contrib/subtree/t/Makefile | 1 +
t/Makefile | 1 +
3 files changed, 3 insertions(+)
diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile
index 1ed174a8cf..e0bf2e32cb 100644
--- a/contrib/scalar/t/Makefile
+++ b/contrib/scalar/t/Makefile
@@ -46,6 +46,7 @@ clean-except-prove-cache:
$(RM) -r valgrind/bin
clean: clean-except-prove-cache
+ $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
$(RM) .prove
test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
diff --git a/contrib/subtree/t/Makefile b/contrib/subtree/t/Makefile
index 3d278bb0ed..4655e0987b 100644
--- a/contrib/subtree/t/Makefile
+++ b/contrib/subtree/t/Makefile
@@ -51,6 +51,7 @@ clean-except-prove-cache:
$(RM) -r valgrind/bin
clean: clean-except-prove-cache
+ $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
$(RM) .prove
test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
diff --git a/t/Makefile b/t/Makefile
index 1c80c0c79a..cb04481114 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -66,6 +66,7 @@ clean-except-prove-cache: clean-chainlint
$(RM) -r valgrind/bin
clean: clean-except-prove-cache
+ $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
$(RM) .prove
clean-chainlint:
--
2.37.3.1160.g17e5960226
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH] t/Makefile: remove 'test-results' on 'make clean'
2022-09-20 10:54 ` [PATCH] t/Makefile: remove 'test-results' on 'make clean' SZEDER Gábor
@ 2022-09-20 19:51 ` Jeff King
2022-09-20 20:11 ` SZEDER Gábor
2022-09-20 20:16 ` [PATCH v2] " SZEDER Gábor
1 sibling, 1 reply; 57+ messages in thread
From: Jeff King @ 2022-09-20 19:51 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, git, Ævar Arnfjörð Bjarmason
On Tue, Sep 20, 2022 at 12:54:07PM +0200, SZEDER Gábor wrote:
> The 't/test-results' directory and its contents are by-products of the
> test process, so 'make clean' should remove them, but, alas, this has
> been broken since ee65b194d (t/Makefile: don't remove test-results in
> "clean-except-prove-cache", 2022-07-28).
I don't have that commit. I assume you mean fee65b194d, and what you
have here was a version before it hit 'next'.
> Add that missing cleanup command to 't/Makefile', and all sub-Makefiles
> touched by ee65b194d as well.
Ditto here.
> contrib/scalar/t/Makefile | 1 +
> contrib/subtree/t/Makefile | 1 +
> t/Makefile | 1 +
That patch itself looks sensible to me.
-Peff
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH] t/Makefile: remove 'test-results' on 'make clean'
2022-09-20 19:51 ` Jeff King
@ 2022-09-20 20:11 ` SZEDER Gábor
2022-09-20 20:42 ` Jeff King
0 siblings, 1 reply; 57+ messages in thread
From: SZEDER Gábor @ 2022-09-20 20:11 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Ævar Arnfjörð Bjarmason
On Tue, Sep 20, 2022 at 03:51:14PM -0400, Jeff King wrote:
> On Tue, Sep 20, 2022 at 12:54:07PM +0200, SZEDER Gábor wrote:
>
> > The 't/test-results' directory and its contents are by-products of the
> > test process, so 'make clean' should remove them, but, alas, this has
> > been broken since ee65b194d (t/Makefile: don't remove test-results in
> > "clean-except-prove-cache", 2022-07-28).
>
> I don't have that commit.
Uh-oh. Me neither :)
> I assume you mean fee65b194d, and what you
> have here was a version before it hit 'next'.
Looking at the reflog, it seems that while rewording the commit
message I inadvertently deleted the first character of the SHA1.
> > Add that missing cleanup command to 't/Makefile', and all sub-Makefiles
> > touched by ee65b194d as well.
>
> Ditto here.
>
> > contrib/scalar/t/Makefile | 1 +
> > contrib/subtree/t/Makefile | 1 +
> > t/Makefile | 1 +
>
> That patch itself looks sensible to me.
>
> -Peff
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH] t/Makefile: remove 'test-results' on 'make clean'
2022-09-20 20:11 ` SZEDER Gábor
@ 2022-09-20 20:42 ` Jeff King
0 siblings, 0 replies; 57+ messages in thread
From: Jeff King @ 2022-09-20 20:42 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, git, Ævar Arnfjörð Bjarmason
On Tue, Sep 20, 2022 at 10:11:50PM +0200, SZEDER Gábor wrote:
> On Tue, Sep 20, 2022 at 03:51:14PM -0400, Jeff King wrote:
> > On Tue, Sep 20, 2022 at 12:54:07PM +0200, SZEDER Gábor wrote:
> >
> > > The 't/test-results' directory and its contents are by-products of the
> > > test process, so 'make clean' should remove them, but, alas, this has
> > > been broken since ee65b194d (t/Makefile: don't remove test-results in
> > > "clean-except-prove-cache", 2022-07-28).
> >
> > I don't have that commit.
>
> Uh-oh. Me neither :)
>
> > I assume you mean fee65b194d, and what you
> > have here was a version before it hit 'next'.
>
> Looking at the reflog, it seems that while rewording the commit
> message I inadvertently deleted the first character of the SHA1.
Heh. You'd think I would have notice the similarity when pasting in the
new one. :) But yeah, that is definitely what happened.
-Peff
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2] t/Makefile: remove 'test-results' on 'make clean'
2022-09-20 10:54 ` [PATCH] t/Makefile: remove 'test-results' on 'make clean' SZEDER Gábor
2022-09-20 19:51 ` Jeff King
@ 2022-09-20 20:16 ` SZEDER Gábor
2022-09-21 6:59 ` Ævar Arnfjörð Bjarmason
2022-09-21 17:52 ` Junio C Hamano
1 sibling, 2 replies; 57+ messages in thread
From: SZEDER Gábor @ 2022-09-20 20:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
SZEDER Gábor
The 't/test-results' directory and its contents are by-products of the
test process, so 'make clean' should remove them, but, alas, this has
been broken since fee65b194d (t/Makefile: don't remove test-results in
"clean-except-prove-cache", 2022-07-28).
The 'clean' target in 't/Makefile' was not directly responsible for
removing the 'test-results' directory, but relied on its dependency
'clean-except-prove-cache' to do that [1]. ee65b194d broke this,
because it only removed the 'rm -r test-results' command from the
'clean-except-prove-cache' target instead of moving it to the 'clean'
target, resulting in stray 't/test-results' directories.
Add that missing cleanup command to 't/Makefile', and to all
sub-Makefiles touched by that commit as well.
[1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs,
2012-05-02)
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
Range-diff:
1: 36a3a71ea5 ! 1: ccb28d7ce7 t/Makefile: remove 'test-results' on 'make clean'
@@ Commit message
The 't/test-results' directory and its contents are by-products of the
test process, so 'make clean' should remove them, but, alas, this has
- been broken since ee65b194d (t/Makefile: don't remove test-results in
+ been broken since fee65b194d (t/Makefile: don't remove test-results in
"clean-except-prove-cache", 2022-07-28).
The 'clean' target in 't/Makefile' was not directly responsible for
@@ Commit message
'clean-except-prove-cache' target instead of moving it to the 'clean'
target, resulting in stray 't/test-results' directories.
- Add that missing cleanup command to 't/Makefile', and all sub-Makefiles
- touched by ee65b194d as well.
+ Add that missing cleanup command to 't/Makefile', and to all
+ sub-Makefiles touched by that commit as well.
[1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs,
2012-05-02)
contrib/scalar/t/Makefile | 1 +
contrib/subtree/t/Makefile | 1 +
t/Makefile | 1 +
3 files changed, 3 insertions(+)
diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile
index 1ed174a8cf..e0bf2e32cb 100644
--- a/contrib/scalar/t/Makefile
+++ b/contrib/scalar/t/Makefile
@@ -46,6 +46,7 @@ clean-except-prove-cache:
$(RM) -r valgrind/bin
clean: clean-except-prove-cache
+ $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
$(RM) .prove
test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
diff --git a/contrib/subtree/t/Makefile b/contrib/subtree/t/Makefile
index 3d278bb0ed..4655e0987b 100644
--- a/contrib/subtree/t/Makefile
+++ b/contrib/subtree/t/Makefile
@@ -51,6 +51,7 @@ clean-except-prove-cache:
$(RM) -r valgrind/bin
clean: clean-except-prove-cache
+ $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
$(RM) .prove
test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
diff --git a/t/Makefile b/t/Makefile
index 1c80c0c79a..cb04481114 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -66,6 +66,7 @@ clean-except-prove-cache: clean-chainlint
$(RM) -r valgrind/bin
clean: clean-except-prove-cache
+ $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
$(RM) .prove
clean-chainlint:
--
2.37.3.1160.g17e5960226
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v2] t/Makefile: remove 'test-results' on 'make clean'
2022-09-20 20:16 ` [PATCH v2] " SZEDER Gábor
@ 2022-09-21 6:59 ` Ævar Arnfjörð Bjarmason
2022-09-21 17:49 ` Junio C Hamano
2022-09-21 17:52 ` Junio C Hamano
1 sibling, 1 reply; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-09-21 6:59 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git, Jeff King
On Tue, Sep 20 2022, SZEDER Gábor wrote:
> The 't/test-results' directory and its contents are by-products of the
> test process, so 'make clean' should remove them, but, alas, this has
> been broken since fee65b194d (t/Makefile: don't remove test-results in
> "clean-except-prove-cache", 2022-07-28).
>
> The 'clean' target in 't/Makefile' was not directly responsible for
> removing the 'test-results' directory, but relied on its dependency
> 'clean-except-prove-cache' to do that [1]. ee65b194d broke this,
> because it only removed the 'rm -r test-results' command from the
> 'clean-except-prove-cache' target instead of moving it to the 'clean'
> target, resulting in stray 't/test-results' directories.
>
> Add that missing cleanup command to 't/Makefile', and to all
> sub-Makefiles touched by that commit as well.
>
> [1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs,
> 2012-05-02)
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
Thanks, and sorry about the breakage. I've looked this over carefully &
it fixes the edge-case you noted without making anything else that I
could spot worse.
In case it helps:
Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
It's still a bit odd to have a "clean" that cleans up a thing it *might
have* generated, i.e. sometimes we create & use these via a Makefile
target, and sometimes by manually invoking test scripts.
But any such issues far pre-date this fix (or my commit).
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2] t/Makefile: remove 'test-results' on 'make clean'
2022-09-21 6:59 ` Ævar Arnfjörð Bjarmason
@ 2022-09-21 17:49 ` Junio C Hamano
0 siblings, 0 replies; 57+ messages in thread
From: Junio C Hamano @ 2022-09-21 17:49 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: SZEDER Gábor, git, Jeff King
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Tue, Sep 20 2022, SZEDER Gábor wrote:
>
>> The 't/test-results' directory and its contents are by-products of the
>> test process, so 'make clean' should remove them, but, alas, this has
>> been broken since fee65b194d (t/Makefile: don't remove test-results in
>> "clean-except-prove-cache", 2022-07-28).
>>
>> The 'clean' target in 't/Makefile' was not directly responsible for
>> removing the 'test-results' directory, but relied on its dependency
>> 'clean-except-prove-cache' to do that [1]. ee65b194d broke this,
>> because it only removed the 'rm -r test-results' command from the
>> 'clean-except-prove-cache' target instead of moving it to the 'clean'
>> target, resulting in stray 't/test-results' directories.
>>
>> Add that missing cleanup command to 't/Makefile', and to all
>> sub-Makefiles touched by that commit as well.
>>
>> [1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs,
>> 2012-05-02)
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>
> Thanks, and sorry about the breakage. I've looked this over carefully &
> it fixes the edge-case you noted without making anything else that I
> could spot worse.
>
> In case it helps:
>
> Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> It's still a bit odd to have a "clean" that cleans up a thing it *might
> have* generated, i.e. sometimes we create & use these via a Makefile
> target, and sometimes by manually invoking test scripts.
I think this is perfectly fine, but we could also use "distclean"
for cleaning up something that might have been created.
Thanks, both.
Will queue.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2] t/Makefile: remove 'test-results' on 'make clean'
2022-09-20 20:16 ` [PATCH v2] " SZEDER Gábor
2022-09-21 6:59 ` Ævar Arnfjörð Bjarmason
@ 2022-09-21 17:52 ` Junio C Hamano
2022-09-26 9:08 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2022-09-21 17:52 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Jeff King, Ævar Arnfjörð Bjarmason
SZEDER Gábor <szeder.dev@gmail.com> writes:
> The 't/test-results' directory and its contents are by-products of the
> test process, so 'make clean' should remove them, but, alas, this has
> been broken since fee65b194d (t/Makefile: don't remove test-results in
> "clean-except-prove-cache", 2022-07-28).
What I find more disturbing is this is part of "leak check" topic,
and I see no reason why we need to futz with "make clean" rule in
the makefile while extending SANITIZE=leak support. We really
should yell louder when we see topics that tries to do too much
"while at it" and reject them to minimize the risk of introducing
this kind of breakage.
Will queue.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2] t/Makefile: remove 'test-results' on 'make clean'
2022-09-21 17:52 ` Junio C Hamano
@ 2022-09-26 9:08 ` Ævar Arnfjörð Bjarmason
2022-09-26 19:08 ` Junio C Hamano
0 siblings, 1 reply; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-09-26 9:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: SZEDER Gábor, git, Jeff King
On Wed, Sep 21 2022, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> The 't/test-results' directory and its contents are by-products of the
>> test process, so 'make clean' should remove them, but, alas, this has
>> been broken since fee65b194d (t/Makefile: don't remove test-results in
>> "clean-except-prove-cache", 2022-07-28).
>
> What I find more disturbing is this is part of "leak check" topic,
> and I see no reason why we need to futz with "make clean" rule in
> the makefile while extending SANITIZE=leak support. We really
> should yell louder when we see topics that tries to do too much
> "while at it" and reject them to minimize the risk of introducing
> this kind of breakage.
FWIW this wasn't a "while at it", it was critical to making another part
of that topic work at all.
It adds a "check" mode, which I use to check that the
TEST_PASSES_SANITIZE_LEAK=true markings are correct.
Intrinsic to how that works (and [1] has more details) is that we "pass"
all tests, when some of them really failed under SANITIZE=leak. I.e. a
failure of a test not opted in to TEST_PASSES_SANITIZE_LEAK=true is
considered OK.
Without fee65b194d that combined with GIT_TEST_SANITIZE_LEAK_LOG=true
added in the same topic would make post-test leak analysis I do useless,
because before fee65b194d the Makefile made the assumption that
"test-results" directories were only need in case the tests failed.
1. e92684e1a26 (test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check
mode, 2022-07-28)
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2] t/Makefile: remove 'test-results' on 'make clean'
2022-09-26 9:08 ` Ævar Arnfjörð Bjarmason
@ 2022-09-26 19:08 ` Junio C Hamano
0 siblings, 0 replies; 57+ messages in thread
From: Junio C Hamano @ 2022-09-26 19:08 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: SZEDER Gábor, git, Jeff King
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> FWIW this wasn't a "while at it", it was critical to making another part
> of that topic work at all.
Perhaps it was critical within the approach of the topic took, and
if a different approach was taken or the scope of topic was reduced,
you would have avoided it. I would still consider such an unnecessary
change an unwated "whilte at it".
The moral of the story is that the topic took too big a bite to be
done without causing unnecessary mistakes like this.
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v3 08/15] tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 07/15] t/Makefile: don't remove test-results in "clean-except-prove-cache" Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 09/15] test-lib: simplify by removing test_external Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
.../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 07227d02287..ff17a9460cd 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 00000000000..d0bf509a167
--- /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 df2ea34932b..043b190626c 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 102c133112c..17fc43f6e57 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
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 09/15] test-lib: simplify by removing test_external
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 08/15] tests: move copy/pasted PERL + Test::More checks to a lib-perl.sh Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 10/15] test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
.../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 ff17a9460cd..bf2777308a5 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 0664aee7ed9..98f69ed13d2 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 [<prereq>] <message> <external> <script>
-
- Execute a <script> with an <external> interpreter (like perl). This
- was added for tests like t9700-perl-git.sh which do most of their
- work in an external test script.
-
- test_external \
- 'GitwebCache::*FileCache*' \
- perl "$TEST_DIRECTORY"/t9503/test_cache_interface.pl
-
- If the test is outputting its own TAP you should set the
- test_external_has_tap variable somewhere before calling the first
- test_external* function. See t9700-perl-git.sh for an example.
-
- # The external test will outputs its own plan
- test_external_has_tap=1
-
- - test_external_without_stderr [<prereq>] <message> <external> <script>
-
- Like test_external but fail if there's any output on stderr,
- instead of checking the exit code.
-
- test_external_without_stderr \
- 'Perl API' \
- perl "$TEST_DIRECTORY"/t9700/test.pl
-
- test_expect_code <exit-code> <command>
Run a command and ensure that it exits with the given exit code.
diff --git a/t/t0202-gettext-perl.sh b/t/t0202-gettext-perl.sh
index 043b190626c..5a6f28051bd 100755
--- a/t/t0202-gettext-perl.sh
+++ b/t/t0202-gettext-perl.sh
@@ -10,11 +10,9 @@ TEST_PASSES_SANITIZE_LEAK=true
. "$TEST_DIRECTORY"/lib-perl.sh
skip_all_if_no_Test_More
-# The external test will outputs its own plan
-test_external_has_tap=1
-
-test_external_without_stderr \
- 'Perl Git::I18N API' \
- perl "$TEST_DIRECTORY"/t0202/test.pl
+test_expect_success 'run t0202/test.pl to test Git::I18N.pm' '
+ "$PERL_PATH" "$TEST_DIRECTORY"/t0202/test.pl 2>stderr &&
+ test_must_be_empty stderr
+'
test_done
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 17fc43f6e57..a48fac6f07a 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -43,11 +43,9 @@ test_expect_success \
git config --add test.pathmulti bar
'
-# The external test will outputs its own plan
-test_external_has_tap=1
-
-test_external_without_stderr \
- 'Perl API' \
- perl "$TEST_DIRECTORY"/t9700/test.pl
+test_expect_success 'use t9700/test.pl to test Git.pm' '
+ "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
+ test_must_be_empty stderr
+'
test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8c44856eaec..c6479f24eb5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -633,7 +633,7 @@ test_hook () {
# - Explicitly using test_have_prereq.
#
# - Implicitly by specifying the prerequisite tag in the calls to
-# test_expect_{success,failure} and test_external{,_without_stderr}.
+# test_expect_{success,failure}
#
# The single parameter is the prerequisite tag (a simple word, in all
# capital letters by convention).
@@ -835,93 +835,6 @@ test_expect_success () {
test_finish_
}
-# test_external runs external test scripts that provide continuous
-# test output about their progress, and succeeds/fails on
-# zero/non-zero exit code. It outputs the test output on stdout even
-# in non-verbose mode, and announces the external script with "# run
-# <n>: ..." before running it. When providing relative paths, keep in
-# mind that all scripts run in "trash directory".
-# Usage: test_external description command arguments...
-# Example: test_external 'Perl API' perl ../path/to/test.pl
-test_external () {
- test "$#" = 4 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 3 ||
- BUG "not 3 or 4 parameters to test_external"
- descr="$1"
- shift
- test_verify_prereq
- export test_prereq
- if ! test_skip "$descr" "$@"
- then
- # Announce the script to reduce confusion about the
- # test output that follows.
- say_color "" "# run $test_count: $descr ($*)"
- # Export TEST_DIRECTORY, TRASH_DIRECTORY and GIT_TEST_LONG
- # to be able to use them in script
- export TEST_DIRECTORY TRASH_DIRECTORY GIT_TEST_LONG
- # Run command; redirect its stderr to &4 as in
- # test_run_, but keep its stdout on our stdout even in
- # non-verbose mode.
- "$@" 2>&4
- if test "$?" = 0
- then
- if test $test_external_has_tap -eq 0; then
- test_ok_ "$descr"
- else
- say_color "" "# test_external test $descr was ok"
- test_success=$(($test_success + 1))
- fi
- else
- if test $test_external_has_tap -eq 0; then
- test_failure_ "$descr" "$@"
- else
- say_color error "# test_external test $descr failed: $@"
- test_failure=$(($test_failure + 1))
- fi
- fi
- fi
-}
-
-# Like test_external, but in addition tests that the command generated
-# no output on stderr.
-test_external_without_stderr () {
- # The temporary file has no (and must have no) security
- # implications.
- tmp=${TMPDIR:-/tmp}
- stderr="$tmp/git-external-stderr.$$.tmp"
- test_external "$@" 4> "$stderr"
- test -f "$stderr" || error "Internal error: $stderr disappeared."
- descr="no stderr: $1"
- shift
- say >&3 "# expecting no stderr from previous command"
- if test ! -s "$stderr"
- then
- rm "$stderr"
-
- if test $test_external_has_tap -eq 0; then
- test_ok_ "$descr"
- else
- say_color "" "# test_external_without_stderr test $descr was ok"
- test_success=$(($test_success + 1))
- fi
- else
- if test "$verbose" = t
- then
- output=$(echo; echo "# Stderr is:"; cat "$stderr")
- else
- output=
- fi
- # rm first in case test_failure exits.
- rm "$stderr"
- if test $test_external_has_tap -eq 0; then
- test_failure_ "$descr" "$@" "$output"
- else
- say_color error "# test_external_without_stderr test $descr failed: $@: $output"
- test_failure=$(($test_failure + 1))
- fi
- fi
-}
-
# debugging-friendly alternatives to "test [-f|-d|-e]"
# The commands test the existence or non-existence of $1
test_path_is_file () {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 557f77c9717..067927eb06c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1231,27 +1231,24 @@ test_done () {
fi
case "$test_failure" in
0)
- if test $test_external_has_tap -eq 0
+ if test $test_remaining -gt 0
then
- if test $test_remaining -gt 0
- then
- say_color pass "# passed all $msg"
- fi
-
- # Maybe print SKIP message
- test -z "$skip_all" || skip_all="# SKIP $skip_all"
- case "$test_count" in
- 0)
- say "1..$test_count${skip_all:+ $skip_all}"
- ;;
- *)
- test -z "$skip_all" ||
- say_color warn "$skip_all"
- say "1..$test_count"
- ;;
- esac
+ say_color pass "# passed all $msg"
fi
+ # Maybe print SKIP message
+ test -z "$skip_all" || skip_all="# SKIP $skip_all"
+ case "$test_count" in
+ 0)
+ say "1..$test_count${skip_all:+ $skip_all}"
+ ;;
+ *)
+ test -z "$skip_all" ||
+ say_color warn "$skip_all"
+ say "1..$test_count"
+ ;;
+ esac
+
if test -n "$stress" && test -n "$invert_exit_code"
then
# We're about to move our "$TRASH_DIRECTORY"
@@ -1286,11 +1283,8 @@ test_done () {
exit 0 ;;
*)
- if test $test_external_has_tap -eq 0
- then
- say_color error "# failed $test_failure among $msg"
- say "1..$test_count"
- fi
+ say_color error "# failed $test_failure among $msg"
+ say "1..$test_count"
if test -n "$invert_exit_code"
then
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 10/15] test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 09/15] test-lib: simplify by removing test_external Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 11/15] test-lib: have the "check" mode for SANITIZE=leak consider leak logs Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Add a new "GIT_TEST_PASSING_SANITIZE_LEAK=check" mode to the
test-lib.sh.
As noted in the updated "t/README" this compliments the existing
"GIT_TEST_PASSING_SANITIZE_LEAK=true" mode added in
956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in CI,
2021-09-23).
Rather than document this all in one (even more) dense paragraph split
up the discussion of how it combines with --immediate into its own
paragraph following the discussion of
"GIT_TEST_SANITIZE_LEAK_LOG=true".
Before the removal of "test_external" in a preceding commit we would
have had to special-case t9700-perl-git.sh and t0202-gettext-perl.sh.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/README | 17 +++++++++++++++++
t/test-lib.sh | 34 ++++++++++++++++++++++++++--------
2 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/t/README b/t/README
index 98f69ed13d2..e13063195e5 100644
--- a/t/README
+++ b/t/README
@@ -371,11 +371,28 @@ 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_PASSING_SANITIZE_LEAK=check checks that our
+"TEST_PASSES_SANITIZE_LEAK=true" markings are current. Rather than
+skipping those tests that haven't set "TEST_PASSES_SANITIZE_LEAK=true"
+before sourcing "test-lib.sh" this mode runs them with
+"--invert-exit-code". This is used to check that there's a one-to-one
+mapping between "TEST_PASSES_SANITIZE_LEAK=true" and those tests that
+pass under "SANITIZE=leak". This is especially useful when testing a
+series that fixes various memory leaks with "git rebase -x".
+
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_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
+will run to completion faster, and result in the same failing
+tests. The only practical reason to run
+GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
+combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
+first failing test case our leak logs won't show subsequent leaks we
+might have run into.
+
GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
default to n.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 067927eb06c..3d0c8896e0e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1451,17 +1451,34 @@ BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK () {
if test -n "$SANITIZE_LEAK"
then
- if test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+ # Normalize with test_bool_env
+ passes_sanitize_leak=
+
+ # We need to see TEST_PASSES_SANITIZE_LEAK in "git
+ # env--helper" (via test_bool_env)
+ export TEST_PASSES_SANITIZE_LEAK
+ if test_bool_env TEST_PASSES_SANITIZE_LEAK false
+ then
+ passes_sanitize_leak=t
+ fi
+
+ if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
then
- # We need to see it in "git env--helper" (via
- # test_bool_env)
- export TEST_PASSES_SANITIZE_LEAK
+ if test -n "$invert_exit_code"
+ then
+ BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=check"
+ fi
- if ! test_bool_env TEST_PASSES_SANITIZE_LEAK false
+ if test -z "$passes_sanitize_leak"
then
- skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
- test_done
+ say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
+ invert_exit_code=t
fi
+ elif test -z "$passes_sanitize_leak" &&
+ test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+ then
+ skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+ test_done
fi
if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
@@ -1480,7 +1497,8 @@ then
prepend_var LSAN_OPTIONS : log_path=\"$TEST_RESULTS_SAN_FILE\"
export LSAN_OPTIONS
fi
-elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
+ test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
then
BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 11/15] test-lib: have the "check" mode for SANITIZE=leak consider leak logs
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 10/15] test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 12/15] leak tests: don't skip some tests under SANITIZE=leak Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
As noted in previous on-list discussions[1] we have various tests that
will falsely report being leak-free because we're missing the relevant
exit code from LSAN as summarized below.
We should fix those issues, but in the meantime and as an additional
sanity check we can and should consider our own ASAN logs before
reporting that a test is leak-free.
Before this compiling with SANITIZE=leak and running:
./t6407-merge-binary.sh
Will exit successfully, now we'll get an error and an informative
message on:
GIT_TEST_SANITIZE_LEAK_LOG=true ./t6407-merge-binary.sh
Even better, as noted in the updated t/README we'll now error out when
combined with the "check" mode:
GIT_TEST_PASSING_SANITIZE_LEAK=check \
GIT_TEST_SANITIZE_LEAK_LOG=true \
./t4058-diff-duplicates.sh
Why do we miss these leaks? Because:
* We have leaks inside "test_expect_failure" blocks, which by design
will not distinguish a "normal" failure from an abort() or
segfault. See [1] for a discussion of it shortcomings.
* We have "git" invocations outside of "test_expect_success",
e.g. setup code in the main body of the test, or in test helper
functions that don't use &&-chaining.
* Our tests will otherwise catch segfaults and abort(), but if we
invoke a command that invokes another command it needs to ferry the
exit code up to us.
Notably a command that e.g. might invoke "git pack-objects" might
itself exit with status 128 if that "pack-objects" segfaults or
abort()'s. If the test invoking the parent command(s) is using
"test_must_fail" we'll consider it an expected "ok" failure.
* run-command.c doesn't (but probably should) ferry up such exit
codes, so for e.g. "git push" tests where we expect a failure and an
underlying "git" command fails we won't ferry up the segfault or
abort exit code.
* We have gitweb.perl and some other perl code ignoring return values
from close(), i.e. ignoring exit codes from "git rev-parse" et al.
* We have in-tree shellscripts like "git-merge-one-file.sh" invoking
git commands, they'll usually return their own exit codes on "git"
failure, rather then ferrying up segfault or abort() exit code.
E.g. these invocations in git-merge-one-file.sh leak, but aren't
reflected in the "git merge" exit code:
src1=$(git unpack-file $2)
src2=$(git unpack-file $3)
That case would be easily "fixed" by adding a line like this after
each assignment:
test $? -ne 0 && exit $?
But we'd then in e.g. "t6407-merge-binary.sh" run into
write_tree_trivial() in "builtin/merge.c" calling die() instead of
ferrying up the relevant exit code.
Let's remove "TEST_PASSES_SANITIZE_LEAK=true" from tests we
were falsely marking as leak-free.
In the case of t6407-merge-binary.sh it was marked as leak-free in
9081a421a6d (checkout: fix "branch info" memory leaks,
2021-11-16). I'd previously removed other bad
"TEST_PASSES_SANITIZE_LEAK=true" opt-ins in the series merged in
ea05fd5fbf7 (Merge branch 'ab/keep-git-exit-codes-in-tests',
2022-03-16). The case of t1060-object-corruption.sh is more subtle,
and will be discussed in a subsequent commit.
1. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/README | 15 +++++++
t/t1060-object-corruption.sh | 1 -
t/t6407-merge-binary.sh | 1 -
t/test-lib.sh | 80 ++++++++++++++++++++++++++++++++++++
4 files changed, 95 insertions(+), 2 deletions(-)
diff --git a/t/README b/t/README
index e13063195e5..2f439f96589 100644
--- a/t/README
+++ b/t/README
@@ -385,6 +385,12 @@ GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
make logs +machine-readable.
+With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
+before exiting and exit on failure if the logs showed that we had a
+memory leak, even if the test itself would have otherwise passed. This
+allows us to catch e.g. missing &&-chaining. This is especially useful
+when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
+
GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
will run to completion faster, and result in the same failing
tests. The only practical reason to run
@@ -393,6 +399,15 @@ combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
first failing test case our leak logs won't show subsequent leaks we
might have run into.
+GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
+leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
+run "git" (or "test-tool" etc.) without properly checking the exit
+code, or git will invoke itself and fail to ferry the abort() exit
+code to the original caller. When the two modes are combined we'll
+look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
+the test run to see if had memory leaks which the test itself didn't
+catch.
+
GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
default to n.
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index 5b8e47e346c..e8a58b15897 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -2,7 +2,6 @@
test_description='see how we handle various forms of corruption'
-TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# convert "1234abcd" to ".git/objects/12/34abcd"
diff --git a/t/t6407-merge-binary.sh b/t/t6407-merge-binary.sh
index 0753fc95f45..e8a28717cec 100755
--- a/t/t6407-merge-binary.sh
+++ b/t/t6407-merge-binary.sh
@@ -5,7 +5,6 @@ test_description='ask merge-recursive to merge binary files'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3d0c8896e0e..10258def7be 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -309,6 +309,7 @@ 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"
+TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP=
TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
@@ -316,6 +317,16 @@ case "$TRASH_DIRECTORY" in
*) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
esac
+# Utility functions using $TEST_RESULTS_* variables
+nr_san_dir_leaks_ () {
+ # stderr piped to /dev/null because the directory may have
+ # been "rmdir"'d already.
+ find "$TEST_RESULTS_SAN_DIR" \
+ -type f \
+ -name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
+ wc -l
+}
+
# If --stress was passed, run this test repeatedly in several parallel loops.
if test "$GIT_TEST_STRESS_STARTED" = "done"
then
@@ -1191,6 +1202,66 @@ test_atexit_handler () {
teardown_malloc_check
}
+sanitize_leak_log_message_ () {
+ local new="$1" &&
+ local old="$2" &&
+ local file="$3" &&
+
+ printf "With SANITIZE=leak at exit we have %d leak logs, but started with %d
+
+This means that we have a blindspot where git is leaking but we're
+losing the exit code somewhere, or not propagating it appropriately
+upwards!
+
+See the logs at \"%s.*\";
+those logs are reproduced below." \
+ "$new" "$old" "$file"
+}
+
+check_test_results_san_file_ () {
+ if test -z "$TEST_RESULTS_SAN_FILE"
+ then
+ return
+ fi &&
+ local old="$TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP" &&
+ local new="$(nr_san_dir_leaks_)" &&
+
+ if test $new -le $old
+ then
+ return
+ fi &&
+ local out="$(sanitize_leak_log_message_ "$new" "$old" "$TEST_RESULTS_SAN_FILE")" &&
+ say_color error "$out" &&
+ if test "$old" != 0
+ then
+ echo &&
+ say_color error "The logs include output from past runs to avoid" &&
+ say_color error "that remove 'test-results' between runs."
+ fi &&
+ say_color error "$(cat "$TEST_RESULTS_SAN_FILE".*)" &&
+
+ if test -n "$passes_sanitize_leak" && test "$test_failure" = 0
+ then
+ say "As TEST_PASSES_SANITIZE_LEAK=true and our logs show we're leaking, exit non-zero!" &&
+ invert_exit_code=t
+ elif test -n "$passes_sanitize_leak"
+ then
+ say "As TEST_PASSES_SANITIZE_LEAK=true and our logs show we're leaking, and we're failing for other reasons too..." &&
+ invert_exit_code=
+ elif test -n "$sanitize_leak_check" && test "$test_failure" = 0
+ then
+ say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
+ invert_exit_code=
+ elif test -n "$sanitize_leak_check"
+ then
+ say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
+ invert_exit_code=t
+ else
+ say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
+ invert_exit_code=t
+ fi
+}
+
test_done () {
# Run the atexit commands _before_ the trash directory is
# removed, so the commands can access pidfiles and socket files.
@@ -1270,6 +1341,8 @@ test_done () {
error "Tests passed but test cleanup failed; aborting"
fi
+ check_test_results_san_file_ "$test_failure"
+
if test -z "$skip_all" && test -n "$invert_exit_code"
then
say_color warn "# faking up non-zero exit with --invert-exit-code"
@@ -1286,6 +1359,8 @@ test_done () {
say_color error "# failed $test_failure among $msg"
say "1..$test_count"
+ check_test_results_san_file_ "$test_failure"
+
if test -n "$invert_exit_code"
then
_invert_exit_code_failure_end_blurb
@@ -1464,6 +1539,7 @@ then
if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
then
+ sanitize_leak_check=t
if test -n "$invert_exit_code"
then
BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=check"
@@ -1489,6 +1565,10 @@ then
fi &&
TEST_RESULTS_SAN_FILE="$TEST_RESULTS_SAN_DIR/$TEST_RESULTS_SAN_FILE_PFX"
+ # In case "test-results" is left over from a previous
+ # run: Only report if new leaks show up.
+ TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP=$(nr_san_dir_leaks_)
+
# Don't litter *.leak dirs if there was nothing to report
test_atexit "rmdir \"$TEST_RESULTS_SAN_DIR\" 2>/dev/null || :"
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 12/15] leak tests: don't skip some tests under SANITIZE=leak
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (10 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 11/15] test-lib: have the "check" mode for SANITIZE=leak consider leak logs Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 13/15] leak tests: mark passing SANITIZE=leak tests as leak-free Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
The '!SANITIZE_LEAK' prerequisite added in 956d2e4639b (tests: add a
test mode for SANITIZE=leak, run it in CI, 2021-09-23) has been used
in various tests to skip individual tests in otherwise leak-free
tests.
Let's change the cases that have become leak-free since then to run
under SANITIZE=leak.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t0002-gitfile.sh | 2 +-
t/t0004-unwritable.sh | 2 +-
t/t1503-rev-parse-verify.sh | 2 +-
t/t3001-ls-files-others-exclude.sh | 4 ++--
t/t3305-notes-fanout.sh | 2 +-
t/t4020-diff-external.sh | 4 ++--
t/t6102-rev-list-unexpected-objects.sh | 4 ++--
7 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index f6356db183b..26eaca095a2 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -65,7 +65,7 @@ test_expect_success 'check commit-tree' '
test_path_is_file "$REAL/objects/$(objpath $SHA)"
'
-test_expect_success !SANITIZE_LEAK 'check rev-list' '
+test_expect_success 'check rev-list' '
git update-ref "HEAD" "$SHA" &&
git rev-list HEAD >actual &&
echo $SHA >expected &&
diff --git a/t/t0004-unwritable.sh b/t/t0004-unwritable.sh
index 2e9d652d826..8114fac73b3 100755
--- a/t/t0004-unwritable.sh
+++ b/t/t0004-unwritable.sh
@@ -31,7 +31,7 @@ test_expect_success WRITE_TREE_OUT 'write-tree output on unwritable repository'
test_cmp expect out.write-tree
'
-test_expect_success POSIXPERM,SANITY,!SANITIZE_LEAK 'commit should notice unwritable repository' '
+test_expect_success POSIXPERM,SANITY 'commit should notice unwritable repository' '
test_when_finished "chmod 775 .git/objects .git/objects/??" &&
chmod a-w .git/objects .git/objects/?? &&
test_must_fail git commit -m second 2>out.commit
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index ba43168d123..bc136833c10 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -132,7 +132,7 @@ test_expect_success 'use --default' '
test_must_fail git rev-parse --verify --default bar
'
-test_expect_success !SANITIZE_LEAK 'main@{n} for various n' '
+test_expect_success 'main@{n} for various n' '
git reflog >out &&
N=$(wc -l <out) &&
Nm1=$(($N-1)) &&
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index e07ac6c6dce..1ed0aa967ec 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -103,7 +103,7 @@ test_expect_success 'git ls-files --others with various exclude options.' '
test_cmp expect output
'
-test_expect_success !SANITIZE_LEAK 'restore gitignore' '
+test_expect_success 'restore gitignore' '
git checkout --ignore-skip-worktree-bits $allignores &&
rm .git/index
'
@@ -126,7 +126,7 @@ cat > expect << EOF
# three/
EOF
-test_expect_success !SANITIZE_LEAK 'git status honors core.excludesfile' \
+test_expect_success 'git status honors core.excludesfile' \
'test_cmp expect output'
test_expect_success 'trailing slash in exclude allows directory match(1)' '
diff --git a/t/t3305-notes-fanout.sh b/t/t3305-notes-fanout.sh
index 64a9915761a..22ffe5bcb99 100755
--- a/t/t3305-notes-fanout.sh
+++ b/t/t3305-notes-fanout.sh
@@ -51,7 +51,7 @@ test_expect_success 'creating many notes with git-notes' '
done
'
-test_expect_success !SANITIZE_LEAK 'many notes created correctly with git-notes' '
+test_expect_success 'many notes created correctly with git-notes' '
git log >output.raw &&
grep "^ " output.raw >output &&
i=$num_notes &&
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 858a5522f96..c1ac09ecc71 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -33,7 +33,7 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment' '
'
-test_expect_success !SANITIZE_LEAK 'GIT_EXTERNAL_DIFF environment should apply only to diff' '
+test_expect_success 'GIT_EXTERNAL_DIFF environment should apply only to diff' '
GIT_EXTERNAL_DIFF=echo git log -p -1 HEAD >out &&
grep "^diff --git a/file b/file" out
@@ -74,7 +74,7 @@ test_expect_success 'diff.external' '
test_cmp expect actual
'
-test_expect_success !SANITIZE_LEAK 'diff.external should apply only to diff' '
+test_expect_success 'diff.external should apply only to diff' '
test_config diff.external echo &&
git log -p -1 HEAD >out &&
grep "^diff --git a/file b/file" out
diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh
index cf0195e8263..4a9a4436e21 100755
--- a/t/t6102-rev-list-unexpected-objects.sh
+++ b/t/t6102-rev-list-unexpected-objects.sh
@@ -17,7 +17,7 @@ test_expect_success 'setup unexpected non-blob entry' '
broken_tree="$(git hash-object -w --literally -t tree broken-tree)"
'
-test_expect_success !SANITIZE_LEAK 'TODO (should fail!): traverse unexpected non-blob entry (lone)' '
+test_expect_success 'TODO (should fail!): traverse unexpected non-blob entry (lone)' '
sed "s/Z$//" >expect <<-EOF &&
$broken_tree Z
$tree foo
@@ -121,7 +121,7 @@ test_expect_success 'setup unexpected non-blob tag' '
tag=$(git hash-object -w --literally -t tag broken-tag)
'
-test_expect_success !SANITIZE_LEAK 'TODO (should fail!): traverse unexpected non-blob tag (lone)' '
+test_expect_success 'TODO (should fail!): traverse unexpected non-blob tag (lone)' '
git rev-list --objects $tag
'
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 13/15] leak tests: mark passing SANITIZE=leak tests as leak-free
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (11 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 12/15] leak tests: don't skip some tests under SANITIZE=leak Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 14/15] upload-pack: fix a memory leak in create_pack_file() Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 15/15] CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaks Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Mark those remaining tests that pass when run under SANITIZE=leak with
TEST_PASSES_SANITIZE_LEAK=true, these were either omitted in
f346fcb62a0 (Merge branch 'ab/mark-leak-free-tests-even-more',
2021-12-15) and 5a4f8381b68 (Merge branch 'ab/mark-leak-free-tests',
2021-10-25), or have had their memory leaks fixed since then.
With this change there's now a a one-to-one mapping between those
tests that we have opted-in via "TEST_PASSES_SANITIZE_LEAK=true", and
those that pass with the new "check" mode:
GIT_TEST_PASSING_SANITIZE_LEAK=check \
GIT_TEST_SANITIZE_LEAK_LOG=true \
make test SANITIZE=leak
Note that the "GIT_TEST_SANITIZE_LEAK_LOG=true" is needed due to the
edge cases noted in a preceding commit, i.e. in some cases we'd pass
the test itself, but still have outstanding leaks due to ignored exit
codes.
The "GIT_TEST_SANITIZE_LEAK_LOG=true" corrects for that, we're only
marking those tests as passing that really don't have any leaks,
whether that was reflected in their exit code or not.
Note that the change here to "t9100-git-svn-basic.sh" is marking that
test as passing under SANITIZE=leak, we're removing a
"TEST_FAILS_SANITIZE_LEAK=true" line, not
"TEST_PASSES_SANITIZE_LEAK=true". See 7a98d9ab00d (revisions API: have
release_revisions() release "cmdline", 2022-04-13) for the
introduction of that t/lib-git-svn.sh-specific variable.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t0027-auto-crlf.sh | 1 +
t/t0032-reftable-unittest.sh | 1 +
t/t0033-safe-directory.sh | 1 +
t/t0050-filesystem.sh | 1 +
t/t0095-bloom.sh | 2 ++
t/t1405-main-ref-store.sh | 1 +
t/t1407-worktree-ref-store.sh | 1 +
t/t1418-reflog-exists.sh | 1 +
t/t1701-racy-split-index.sh | 1 +
t/t2006-checkout-index-basic.sh | 1 +
t/t2023-checkout-m.sh | 1 +
t/t2205-add-worktree-config.sh | 1 +
t/t3012-ls-files-dedup.sh | 1 +
t/t4017-diff-retval.sh | 1 +
t/t4051-diff-function-context.sh | 1 +
t/t4057-diff-combined-paths.sh | 1 +
t/t4114-apply-typechange.sh | 1 +
t/t4301-merge-tree-write-tree.sh | 1 +
t/t5315-pack-objects-compression.sh | 1 +
t/t5351-unpack-large-objects.sh | 1 +
t/t5402-post-merge-hook.sh | 1 +
t/t5503-tagfollow.sh | 1 +
t/t6404-recursive-merge.sh | 1 +
t/t6405-merge-symlinks.sh | 1 +
t/t6408-merge-up-to-date.sh | 1 +
t/t6411-merge-filemode.sh | 1 +
t/t6413-merge-crlf.sh | 1 +
t/t6425-merge-rename-delete.sh | 1 +
t/t6431-merge-criscross.sh | 1 +
t/t7060-wtstatus.sh | 1 +
t/t7062-wtstatus-ignorecase.sh | 1 +
t/t7110-reset-merge.sh | 1 +
t/t7111-reset-table.sh | 1 +
t/t7609-mergetool--lib.sh | 1 +
t/t9100-git-svn-basic.sh | 1 -
t/t9700-perl-git.sh | 2 ++
t/t9901-git-web--browse.sh | 1 +
37 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 7f80f463930..a22e0e1382c 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -2,6 +2,7 @@
test_description='CRLF conversion all combinations'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
compare_files () {
diff --git a/t/t0032-reftable-unittest.sh b/t/t0032-reftable-unittest.sh
index 0ed14971a58..471cb37ac28 100755
--- a/t/t0032-reftable-unittest.sh
+++ b/t/t0032-reftable-unittest.sh
@@ -5,6 +5,7 @@
test_description='reftable unittests'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'unittests' '
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index f4d737dadd0..aecb308cf66 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -2,6 +2,7 @@
test_description='verify safe.directory checks'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
GIT_TEST_ASSUME_DIFFERENT_OWNER=1
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index 5c9dc90d0b0..325eb1c3cd0 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -5,6 +5,7 @@ test_description='Various filesystem issues'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
auml=$(printf '\303\244')
diff --git a/t/t0095-bloom.sh b/t/t0095-bloom.sh
index daeb4a5e3e7..b567383eb83 100755
--- a/t/t0095-bloom.sh
+++ b/t/t0095-bloom.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='Testing the various Bloom filter computations in bloom.c'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'compute unseeded murmur3 hash for empty string' '
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 51f82916281..e4627cf1b61 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -5,6 +5,7 @@ test_description='test main ref store api'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
RUN="test-tool ref-store main"
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index ad8006c8139..05b1881c591 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -5,6 +5,7 @@ test_description='test worktree ref store api'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
RWT="test-tool ref-store worktree:wt"
diff --git a/t/t1418-reflog-exists.sh b/t/t1418-reflog-exists.sh
index d51ecd5e925..2268bca3c11 100755
--- a/t/t1418-reflog-exists.sh
+++ b/t/t1418-reflog-exists.sh
@@ -4,6 +4,7 @@ test_description='Test reflog display routines'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh
index 5dc221ef382..d8fa489998a 100755
--- a/t/t1701-racy-split-index.sh
+++ b/t/t1701-racy-split-index.sh
@@ -5,6 +5,7 @@
test_description='racy split index'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
index 7705e3a3170..5d119871d41 100755
--- a/t/t2006-checkout-index-basic.sh
+++ b/t/t2006-checkout-index-basic.sh
@@ -3,6 +3,7 @@
test_description='basic checkout-index tests
'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'checkout-index --gobbledegook' '
diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh
index 7b327b75449..81e772fb4eb 100755
--- a/t/t2023-checkout-m.sh
+++ b/t/t2023-checkout-m.sh
@@ -7,6 +7,7 @@ Ensures that checkout -m on a resolved file restores the conflicted file'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t2205-add-worktree-config.sh b/t/t2205-add-worktree-config.sh
index 43d950de640..98265ba1b49 100755
--- a/t/t2205-add-worktree-config.sh
+++ b/t/t2205-add-worktree-config.sh
@@ -17,6 +17,7 @@ outside the repository. Two instances for which this can occur are tested:
repository can be added to the index.
'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success '1a: setup--config worktree' '
diff --git a/t/t3012-ls-files-dedup.sh b/t/t3012-ls-files-dedup.sh
index 2682b1f43a6..190e2f6eed7 100755
--- a/t/t3012-ls-files-dedup.sh
+++ b/t/t3012-ls-files-dedup.sh
@@ -2,6 +2,7 @@
test_description='git ls-files --deduplicate test'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index ed461f481e2..5bc28ad9f04 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -5,6 +5,7 @@ test_description='Return value of diffs'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 4838a1df8b4..725278ad19c 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -2,6 +2,7 @@
test_description='diff function context'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
dir="$TEST_DIRECTORY/t4051"
diff --git a/t/t4057-diff-combined-paths.sh b/t/t4057-diff-combined-paths.sh
index 04b8a1542a8..9a7505cbb8b 100755
--- a/t/t4057-diff-combined-paths.sh
+++ b/t/t4057-diff-combined-paths.sh
@@ -5,6 +5,7 @@ test_description='combined diff show only paths that are different to all parent
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# verify that diffc.expect matches output of
diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh
index da3e64f8110..8ff36407667 100755
--- a/t/t4114-apply-typechange.sh
+++ b/t/t4114-apply-typechange.sh
@@ -7,6 +7,7 @@ test_description='git apply should not get confused with type changes.
'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup repository and commits' '
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index f091259a55e..a243e3c5176 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -2,6 +2,7 @@
test_description='git merge-tree --write-tree'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# This test is ort-specific
diff --git a/t/t5315-pack-objects-compression.sh b/t/t5315-pack-objects-compression.sh
index 8bacd96275b..c80ea9e8b71 100755
--- a/t/t5315-pack-objects-compression.sh
+++ b/t/t5315-pack-objects-compression.sh
@@ -2,6 +2,7 @@
test_description='pack-object compression configuration'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t5351-unpack-large-objects.sh b/t/t5351-unpack-large-objects.sh
index f785cb06173..4cfda648a5e 100755
--- a/t/t5351-unpack-large-objects.sh
+++ b/t/t5351-unpack-large-objects.sh
@@ -5,6 +5,7 @@
test_description='git unpack-objects with large objects'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
prepare_dest () {
diff --git a/t/t5402-post-merge-hook.sh b/t/t5402-post-merge-hook.sh
index 915af2de95e..46ebdfbeeba 100755
--- a/t/t5402-post-merge-hook.sh
+++ b/t/t5402-post-merge-hook.sh
@@ -7,6 +7,7 @@ test_description='Test the post-merge hook.'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 195fc64dd44..5ebbaa48968 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -5,6 +5,7 @@ test_description='test automatic tag following'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# End state of the repository:
diff --git a/t/t6404-recursive-merge.sh b/t/t6404-recursive-merge.sh
index b8735c6db4d..36215518b6e 100755
--- a/t/t6404-recursive-merge.sh
+++ b/t/t6404-recursive-merge.sh
@@ -4,6 +4,7 @@ test_description='Test merge without common ancestors'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# This scenario is based on a real-world repository of Shawn Pearce.
diff --git a/t/t6405-merge-symlinks.sh b/t/t6405-merge-symlinks.sh
index 7435fce71e0..29e2b25ce5d 100755
--- a/t/t6405-merge-symlinks.sh
+++ b/t/t6405-merge-symlinks.sh
@@ -11,6 +11,7 @@ if core.symlinks is false.'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t6408-merge-up-to-date.sh b/t/t6408-merge-up-to-date.sh
index 7763c1ba980..8a1ba6d23a7 100755
--- a/t/t6408-merge-up-to-date.sh
+++ b/t/t6408-merge-up-to-date.sh
@@ -2,6 +2,7 @@
test_description='merge fast-forward and up to date'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t6411-merge-filemode.sh b/t/t6411-merge-filemode.sh
index 6ae2489286c..b6182723aae 100755
--- a/t/t6411-merge-filemode.sh
+++ b/t/t6411-merge-filemode.sh
@@ -4,6 +4,7 @@ test_description='merge: handle file mode'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'set up mode change in one branch' '
diff --git a/t/t6413-merge-crlf.sh b/t/t6413-merge-crlf.sh
index affea255fe9..b4f4a313f48 100755
--- a/t/t6413-merge-crlf.sh
+++ b/t/t6413-merge-crlf.sh
@@ -11,6 +11,7 @@ test_description='merge conflict in crlf repo
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t6425-merge-rename-delete.sh b/t/t6425-merge-rename-delete.sh
index 459b431a60d..93cd2869b12 100755
--- a/t/t6425-merge-rename-delete.sh
+++ b/t/t6425-merge-rename-delete.sh
@@ -4,6 +4,7 @@ test_description='Merge-recursive rename/delete conflict message'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'rename/delete' '
diff --git a/t/t6431-merge-criscross.sh b/t/t6431-merge-criscross.sh
index 3824756a02e..3fe14cd73e8 100755
--- a/t/t6431-merge-criscross.sh
+++ b/t/t6431-merge-criscross.sh
@@ -2,6 +2,7 @@
test_description='merge-recursive backend test'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# A <- create some files
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 0f4344c55e6..aaeb4a53344 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -5,6 +5,7 @@ test_description='basic work tree status reporting'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t7062-wtstatus-ignorecase.sh b/t/t7062-wtstatus-ignorecase.sh
index 73709dbeee2..caf372a3d42 100755
--- a/t/t7062-wtstatus-ignorecase.sh
+++ b/t/t7062-wtstatus-ignorecase.sh
@@ -2,6 +2,7 @@
test_description='git-status with core.ignorecase=true'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'status with hash collisions' '
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 3d62e10b53f..eb881be95b6 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -5,6 +5,7 @@
test_description='Tests for "git reset" with "--merge" and "--keep" options'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t7111-reset-table.sh b/t/t7111-reset-table.sh
index ce421ad5ac4..78f25c1c7ea 100755
--- a/t/t7111-reset-table.sh
+++ b/t/t7111-reset-table.sh
@@ -5,6 +5,7 @@
test_description='Tests to check that "reset" options follow a known table'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
diff --git a/t/t7609-mergetool--lib.sh b/t/t7609-mergetool--lib.sh
index 330d6d603d7..8b1c3bd39f2 100755
--- a/t/t7609-mergetool--lib.sh
+++ b/t/t7609-mergetool--lib.sh
@@ -4,6 +4,7 @@ test_description='git mergetool
Testing basic merge tools options'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'mergetool --tool=vimdiff creates the expected layout' '
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 7c5b847f584..fea41b3c360 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -8,7 +8,6 @@ test_description='git svn basic tests'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-TEST_FAILS_SANITIZE_LEAK=true
. ./lib-git-svn.sh
prepare_utf8_locale
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index a48fac6f07a..4aa5d90d328 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -4,6 +4,8 @@
#
test_description='perl interface (Git.pm)'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-perl.sh
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
index de7152f8271..19f56e5680f 100755
--- a/t/t9901-git-web--browse.sh
+++ b/t/t9901-git-web--browse.sh
@@ -5,6 +5,7 @@ test_description='git web--browse basic tests
This test checks that git web--browse can handle various valid URLs.'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_web_browse () {
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 14/15] upload-pack: fix a memory leak in create_pack_file()
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (12 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 13/15] leak tests: mark passing SANITIZE=leak tests as leak-free Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
2022-07-27 23:13 ` [PATCH v3 15/15] CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaks Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Fix a memory leak that's been reported by some versions of "gcc" since
"output_state" became malloc'd in 55a9651d26a (upload-pack.c: increase
output buffer size, 2021-12-14).
In e75d2f7f734 (revisions API: have release_revisions() release
"filter", 2022-04-13) it was correctly marked as leak-free, the only
path through this function that doesn't reach the free(output_state)
is if we "goto fail", and that will invoke "die()".
Such leaks are not included with SANITIZE=leak (but e.g. valgrind will
still report them), but under some gcc optimization (I have not been
able to reproduce it with "clang") we'll report a leak here
anyway. E.g. gcc v12 with "-O2" and above will trigger it, but not
clang v13 with any "-On".
The GitHub CI would also run into this leak if the "linux-leaks" job
was made to run with "GIT_TEST_SANITIZE_LEAK_LOG=true".
See [1] for a past case where gcc had similar trouble analyzing leaks
involving a die() invocation in the function.
1. https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t1060-object-corruption.sh | 1 +
upload-pack.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index e8a58b15897..5b8e47e346c 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -2,6 +2,7 @@
test_description='see how we handle various forms of corruption'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# convert "1234abcd" to ".git/objects/12/34abcd"
diff --git a/upload-pack.c b/upload-pack.c
index 09f48317b02..b217a1f469e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -455,6 +455,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
return;
fail:
+ free(output_state);
send_client_data(3, abort_msg, sizeof(abort_msg),
pack_data->use_sideband);
die("git upload-pack: %s", abort_msg);
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 15/15] CI: use "GIT_TEST_SANITIZE_LEAK_LOG=true" in linux-leaks
2022-07-27 23:13 ` [PATCH v3 00/15] leak test: add "check" test mode, mark leak-free tests Ævar Arnfjörð Bjarmason
` (13 preceding siblings ...)
2022-07-27 23:13 ` [PATCH v3 14/15] upload-pack: fix a memory leak in create_pack_file() Ævar Arnfjörð Bjarmason
@ 2022-07-27 23:13 ` Ævar Arnfjörð Bjarmason
14 siblings, 0 replies; 57+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-27 23:13 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee,
Ævar Arnfjörð Bjarmason
As noted in a preceding commit the leak checking done by
"GIT_TEST_PASSING_SANITIZE_LEAK=true" (added in [1]) is incomplete
without combining it with "GIT_TEST_SANITIZE_LEAK_LOG=true".
Let's run our CI with that, to ensure that we catch cases where our
tests are missing the abort() exit code resulting from a leak for
whatever reason. The reasons for that are discussed in detail in a
preceding commit.
1. 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in
CI, 2021-09-23)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
ci/lib.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/ci/lib.sh b/ci/lib.sh
index f095519f8db..1b0cc2b57db 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -276,6 +276,7 @@ linux-musl)
linux-leaks)
export SANITIZE=leak
export GIT_TEST_PASSING_SANITIZE_LEAK=true
+ export GIT_TEST_SANITIZE_LEAK_LOG=true
;;
esac
--
2.37.1.1127.g4ecf1c08f67
^ permalink raw reply related [flat|nested] 57+ messages in thread