Skip to content

Commit 8c1d669

Browse files
rjustogitster
authored andcommitted
test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 47c6d4d commit 8c1d669

File tree

3 files changed

+17
-47
lines changed

3 files changed

+17
-47
lines changed

ci/lib.sh

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,6 @@ linux-musl)
374374
linux-leaks|linux-reftable-leaks)
375375
export SANITIZE=leak
376376
export GIT_TEST_PASSING_SANITIZE_LEAK=true
377-
export GIT_TEST_SANITIZE_LEAK_LOG=true
378377
;;
379378
linux-asan-ubsan)
380379
export SANITIZE=address,undefined

t/README

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -382,33 +382,9 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true" and those tests that
382382
pass under "SANITIZE=leak". This is especially useful when testing a
383383
series that fixes various memory leaks with "git rebase -x".
384384

385-
GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
386-
"test-results/$TEST_NAME.leak/trace.*" files. The logs include a
387-
"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
388-
make logs +machine-readable.
389-
390-
With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
391-
before exiting and exit on failure if the logs showed that we had a
392-
memory leak, even if the test itself would have otherwise passed. This
393-
allows us to catch e.g. missing &&-chaining. This is especially useful
394-
when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
395-
396385
GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
397386
will run to completion faster, and result in the same failing
398-
tests. The only practical reason to run
399-
GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
400-
combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
401-
first failing test case our leak logs won't show subsequent leaks we
402-
might have run into.
403-
404-
GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
405-
leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
406-
run "git" (or "test-tool" etc.) without properly checking the exit
407-
code, or git will invoke itself and fail to ferry the abort() exit
408-
code to the original caller. When the two modes are combined we'll
409-
look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
410-
the test run to see if had memory leaks which the test itself didn't
411-
catch.
387+
tests.
412388

413389
GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
414390
default to n.

t/test-lib.sh

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1271,10 +1271,10 @@ check_test_results_san_file_ () {
12711271
invert_exit_code=t
12721272
elif test "$test_failure" = 0
12731273
then
1274-
say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
1274+
say "Our logs revealed a memory leak, exit non-zero!" &&
12751275
invert_exit_code=t
12761276
else
1277-
say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
1277+
say "Our logs revealed a memory leak..."
12781278
fi
12791279
}
12801280

@@ -1578,33 +1578,28 @@ then
15781578
test_done
15791579
fi
15801580

1581-
if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
1581+
if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
15821582
then
1583-
if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
1584-
then
1585-
BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
1586-
fi &&
1587-
TEST_RESULTS_SAN_FILE="$TEST_RESULTS_SAN_DIR/$TEST_RESULTS_SAN_FILE_PFX"
1583+
BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
1584+
fi &&
1585+
TEST_RESULTS_SAN_FILE="$TEST_RESULTS_SAN_DIR/$TEST_RESULTS_SAN_FILE_PFX"
15881586

1589-
# In case "test-results" is left over from a previous
1590-
# run: Only report if new leaks show up.
1591-
TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP=$(nr_san_dir_leaks_)
1587+
# In case "test-results" is left over from a previous
1588+
# run: Only report if new leaks show up.
1589+
TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP=$(nr_san_dir_leaks_)
15921590

1593-
# Don't litter *.leak dirs if there was nothing to report
1594-
test_atexit "rmdir \"$TEST_RESULTS_SAN_DIR\" 2>/dev/null || :"
1591+
# Don't litter *.leak dirs if there was nothing to report
1592+
test_atexit "rmdir \"$TEST_RESULTS_SAN_DIR\" 2>/dev/null || :"
1593+
1594+
prepend_var LSAN_OPTIONS : dedup_token_length=9999
1595+
prepend_var LSAN_OPTIONS : log_exe_name=1
1596+
prepend_var LSAN_OPTIONS : log_path=\"$TEST_RESULTS_SAN_FILE\"
1597+
export LSAN_OPTIONS
15951598

1596-
prepend_var LSAN_OPTIONS : dedup_token_length=9999
1597-
prepend_var LSAN_OPTIONS : log_exe_name=1
1598-
prepend_var LSAN_OPTIONS : log_path=\"$TEST_RESULTS_SAN_FILE\"
1599-
export LSAN_OPTIONS
1600-
fi
16011599
elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
16021600
test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
16031601
then
16041602
BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
1605-
elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
1606-
then
1607-
BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
16081603
fi
16091604

16101605
if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&

0 commit comments

Comments
 (0)