Re: [PATCH v2 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> 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?

> @@ -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::"
> +}
> +


> 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?

> -	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