Re: [PATCH v2 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure
flat view
From: Junio C Hamano <hidden>
Date: 2026-09-28 20:46:56
"Harald Nordgren via GitGitGadget" [off-list ref] writes:
From: Harald Nordgren <redacted>
A leak is only discovered once, at the end of a whole script, well
after every test has already reported ok, and it gets no annotation at
all, so a leak-sanitizer job's only visible failure is:
Process completed with exit code 1.
Give a leak its own annotation. Point it at the test script, the exact
line isn't known, only which script the leak turned up in, and put the
full sanitizer report in a log group next to it, so it stays visible
and isn't capped to a handful of lines.
Once a script has one leak, it keeps running: the sanitizer log
directory is never cleared between tests, so every later test in the
same script sees the same leftover log entries and also reports "not
ok", burying the one real failure in copies of itself. Stop a
leak-sanitizer script at its first failure with --immediate instead.OK. So the idea is that we do not have sanitizer report per test_expect_* block but showing the single one over and over, whether the next test_expect_* block has leaks, is not helpful, so we just immediately kill the test script after the first leak?
quoted hunk ↗ jump to hunk
@@ -53,4 +58,15 @@ finalize_test_case_output () { echo >>$github_markup_output "::endgroup::" } +finalize_test_leak_output () { + # The exact line the leak turned up on isn't known, only the script, + # so point at line 1. + github_annotation_ error "t/$github_markup_script_name" 1 \ + "memory leak logged in $this_test" + + echo >>$github_markup_output "::group::leak: $this_test.$test_count" + cat "$TEST_RESULTS_SAN_FILE".* >>$github_markup_output + echo >>$github_markup_output "::endgroup::" +} +
quoted hunk ↗ jump to hunk
diff --git a/t/test-lib.sh b/t/test-lib.sh index 1f0505e412..3552a19323 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh@@ -199,6 +199,7 @@ mark_option_requires_arg () { start_test_output () { :; } start_test_case_output () { :; } finalize_test_case_output () { :; } +finalize_test_leak_output () { :; } finalize_test_output () { :; } parse_option () {@@ -822,20 +823,23 @@ test_failure_ () { say_color error "not ok $test_count - ${pfx:+$pfx }$1" shift printf '%s\n' "$*" | sed -e 's/^/# /' + if test -n "$immediate" && test -n "$invert_exit_code" + then + say_color error "1..$test_count" + finalize_test_output + _invert_exit_code_failure_end_blurb + GIT_EXIT_OK=t + exit 0 + fi + # Write the annotation before the --immediate exit paths below, + # which call exit and would otherwise skip it. + finalize_test_case_output failure "$failure_label" "$@" 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 check_test_results_san_file_ "$test_failure" _error_exit fi
The two-line comment in the middle made me puzzled to see "exit 0" just above it. If "--immediate" is asked and we are checking leaks, shouldn't we be doing finalize_test_case_output regardless of the "invert" setting?
quoted hunk ↗ jump to hunk
- finalize_test_case_output failure "$failure_label" "$@" } test_known_broken_ok_ () {@@ -1218,6 +1222,7 @@ check_test_results_san_file_ () { return fi && say_color >&4 error "$(cat "$TEST_RESULTS_SAN_FILE".*)" && + finalize_test_leak_output && if test "$test_failure" = 0 then