| Message ID | 20240924213540.GA1142403@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 95c679ad861403d12178f871834bf3aa69981bc6 |
| Headers | show |
| Series | LSan quality of life improvements | expand |
On Tue, Sep 24, 2024 at 05:35:40PM -0400, Jeff King wrote: > We ask LSan to record the logs of all leaks in test-results/, which is > useful for finding leaks that didn't trigger a test failure. > > We don't clean out the leak/ directory for each test before running it, > though. Instead, we count the number of files it has, and complain only > if we ended up with more when the script finishes. So we shouldn't > trigger any output if you've made a script leak free. But if you simply > _reduced_ the number of leaks, then there is an annoying outcome: we do > not record which logs were from this run and which were from previous > ones. So when we dump them to stdout, you get a mess of > possibly-outdated leaks. This is very confusing when you are in an > edit-compile-test cycle trying to fix leaks. > > The instructions do note that you should "rm -rf test-results/" if you > want to avoid this. But I'm having trouble seeing how this cumulative > count could ever be useful. It is not even counting the number of leaks, > but rather the number of processes with at least one leak! > > So let's just blow away the per-test leak/ directory before running. We > already overwrite the ".out" file in test-results/ in the same way, so > this is following that pattern. > > Running "make test" isn't affected by this, since it blows away all of > test-results/ already. This only comes up when you are iterating on a > single script that you're running manually. I'm very much in favor of this change. I frequently re-ran tests with leak checking enabled only to realize that, oops, I forgot to "rm -rf" the leak directory first. So eventually I ended up with the following command: $ rm -rf /tmp/git-tests/ && make -C .. -j20 SANITIZE=leak && ./t5310-pack-bitmaps.sh -ix Every time I didn't use it I soon came to regret it. Patrick
diff --git a/t/test-lib.sh b/t/test-lib.sh index e718efe4c6..7d4471fbc5 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -322,7 +322,6 @@ 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 @@ -1215,42 +1214,15 @@ 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 + if test "$(nr_san_dir_leaks_)" = 0 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 @@ -1586,16 +1558,13 @@ then test_done fi + rm -rf "$TEST_RESULTS_SAN_DIR" 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" - # 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 || :"
We ask LSan to record the logs of all leaks in test-results/, which is useful for finding leaks that didn't trigger a test failure. We don't clean out the leak/ directory for each test before running it, though. Instead, we count the number of files it has, and complain only if we ended up with more when the script finishes. So we shouldn't trigger any output if you've made a script leak free. But if you simply _reduced_ the number of leaks, then there is an annoying outcome: we do not record which logs were from this run and which were from previous ones. So when we dump them to stdout, you get a mess of possibly-outdated leaks. This is very confusing when you are in an edit-compile-test cycle trying to fix leaks. The instructions do note that you should "rm -rf test-results/" if you want to avoid this. But I'm having trouble seeing how this cumulative count could ever be useful. It is not even counting the number of leaks, but rather the number of processes with at least one leak! So let's just blow away the per-test leak/ directory before running. We already overwrite the ".out" file in test-results/ in the same way, so this is following that pattern. Running "make test" isn't affected by this, since it blows away all of test-results/ already. This only comes up when you are iterating on a single script that you're running manually. Signed-off-by: Jeff King <peff@peff.net> --- t/test-lib.sh | 35 ++--------------------------------- 1 file changed, 2 insertions(+), 33 deletions(-)