From 47c6d4dad22a751068a4975f1c4177cc6c0c41d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Justo?= Date: Sun, 30 Jun 2024 08:42:06 +0200 Subject: [PATCH 1/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a test that leaks runs with GIT_TEST_SANITIZE_LEAK_LOG=true, the test returns zero, which is not what we want. In the if-else's chain we have in "check_test_results_san_file_", we consider three variables: $passes_sanitize_leak, $sanitize_leak_check and, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true" at that point). For the first two variables we have different considerations depending on the value of $test_failure, which makes sense. However, for the third, GIT_TEST_SANITIZE_LEAK_LOG, we don't; regardless of $test_failure, we use "invert_exit_code=t" to produce a non-zero return value. That assumes "$test_failure" is always zero at that point. But it may not be: $ git checkout v2.40.1 $ make test SANITIZE=leak T=t3200-branch.sh # this fails $ make test SANITIZE=leak GIT_TEST_SANITIZE_LEAK_LOG=true T=t3200-branch.sh # this succeeds [...] With GIT_TEST_SANITIZE_LEAK_LOG=true, our logs revealed a memory leak, exiting with a non-zero status! # faked up failures as TODO & now exiting with 0 due to --invert-exit-code We need to use "invert_exit_code=t" only when "$test_failure" is zero. Let's add the missing conditions in the if-else's chain to make it work as expected. Helped-by: Eric Sunshine Helped-by: Jeff King Signed-off-by: Rubén Justo Acked-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 79d3e0e7d9..7ed6d3fc47 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1269,9 +1269,12 @@ check_test_results_san_file_ () { 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 + elif test "$test_failure" = 0 + then say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" && invert_exit_code=t + else + say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..." fi } From 8c1d6691bc514e2c1c01a807e872b5dddcb2485b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Justo?= Date: Thu, 11 Jul 2024 23:10:51 +0900 Subject: [PATCH 2/2] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As we currently describe in t/README, it can happen that: 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. Therefore, GIT_TEST_SANITIZE_LEAK_LOG=true is needed to be set to capture all memory leaks triggered by our tests. It seems unnecessary to force users to remember this option, as forgetting it could lead to missed memory leaks. We could solve the problem by making it "true" by default, but that might suggest we think "false" makes sense, which isn't the case. Therefore, the best approach is to remove the option entirely while maintaining the capability to detect memory leaks in blind spots of our tests. Signed-off-by: Rubén Justo Signed-off-by: Junio C Hamano --- ci/lib.sh | 1 - t/README | 26 +------------------------- t/test-lib.sh | 37 ++++++++++++++++--------------------- 3 files changed, 17 insertions(+), 47 deletions(-) diff --git a/ci/lib.sh b/ci/lib.sh index ff66ad356b..fe52954828 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -374,7 +374,6 @@ linux-musl) linux-leaks|linux-reftable-leaks) export SANITIZE=leak export GIT_TEST_PASSING_SANITIZE_LEAK=true - export GIT_TEST_SANITIZE_LEAK_LOG=true ;; linux-asan-ubsan) export SANITIZE=address,undefined diff --git a/t/README b/t/README index d9e0e07506..ea620de17e 100644 --- a/t/README +++ b/t/README @@ -382,33 +382,9 @@ 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. - -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 -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_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. +tests. GIT_TEST_PROTOCOL_VERSION=, when set, makes 'protocol.version' default to n. diff --git a/t/test-lib.sh b/t/test-lib.sh index 7ed6d3fc47..54247604cb 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1271,10 +1271,10 @@ check_test_results_san_file_ () { invert_exit_code=t elif test "$test_failure" = 0 then - say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" && + say "Our logs revealed a memory leak, exit non-zero!" && invert_exit_code=t else - say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..." + say "Our logs revealed a memory leak..." fi } @@ -1578,33 +1578,28 @@ then test_done fi - if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false + if ! mkdir -p "$TEST_RESULTS_SAN_DIR" 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" + BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR" + 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_) + # 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 || :" + # 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 - 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 "$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 -then - BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true" fi if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&