ci: point leak-sanitizer failures at the actual test and error

From: Harald Nordgren <[email protected]>

A leak is only found once, at the end of a whole script, well after every test already reported ok, and the failure annotation carried no file or line, so all a reviewer ever saw was:

Process completed with exit code 1.

with nothing to click through to. Stop each leak-sanitizer script at its first failure instead of running the rest of an already-tainted script, and have both failure and leak annotations point at the real file and carry the actual error, for example:

t/t1507-rev-parse-upstream.sh, line 1:
memory leak logged around t1507.1
==ERROR: LeakSanitizer: detected memory leaks Direct leak of 60 byte(s) in 1 object(s) allocated from:
...
#5 in add_branch builtin/remote.c:135

---
ci: point leak-sanitizer failures at the actual test and error

I discovered while running CI on another GitHub pull request that it's very hard to see where the error is for the leak tests.

This will stop each leak-sanitizer script at its first failure and points annotations at the real file and error.

Proof that it works:
https://github.com/git/git/actions/runs/35871180948/job/107215430244

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2419%2FHaraldNordgren%2Fci-annotation-file-line-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2419/HaraldNordgren/ci-annotation-file-line-v1
Pull-Request: https://github.com/git/git/pull/2419

 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 49 +++++++++++++++++++++++-----
 t/test-lib.sh                        |  2 ++
 3 files changed, 44 insertions(+), 8 deletions(-)

index c6ccbf8c17..a89f480a78 100755
@@ -382,6 +382,7 @@ linux-leaks|linux-reftable-leaks)
 	export NO_CVS_TESTS=LetsSaveSomeTime
 	export NO_SVN_TESTS=LetsSaveSomeTime
 	export NO_P4_TESTS=LetsSaveSomeTime
+	GIT_TEST_OPTS="$GIT_TEST_OPTS --immediate"
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined

index 1f0505e412..a52589c6a2 100644
@@ -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 () {

@@ -1218,6 +1219,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

base-commit: 3bc0341126508f78f5869cbfc0005e987efdf0c7

gitgitgadget

Good point!

Harald

Hi Harald

Putting the leak output in the test results is very welcome, but does this mean that if there are two leaks we only report one?

Opening that link shows that the individual test failures are no-longer folded and I see some very strange scrolling behavior in firefox - when the page opens it scrolls to the bottom of the output of "ci/build-and-run-tests.sh" and if I try to scroll up it immediately scrolls back down as soon as my fingers leave the touchpad.

The patch below seems to do more than just changing the output to display the leak backtrace - it adds some escaping and changes the annotations. There is no explanation of what these changes do or why they are required.

Thanks

Phillip

It already had a behavior where one failure made every subsequent test in the script report "not ok" too, so lots of noise burying the real leaks.

I'll take a look at that.

I'll expand the commit message.

Harald

Link failure and leak annotations in CI to the test script, so both can be found from the job summary.

CI Job where failures and leaks are reported:
https://github.com/git/git/actions/runs/36449319996/job/109020434364?pr=2426

Changes in v2:

* Split into two commits, each explaining its own reasoning.
* Leak output is no longer capped or embedded in the message, it's now an uncapped fold, so multiple leaks in the same test both show in full. A second leak in a different test still won't show in the same run, --immediate stops the script at the first failure, but it no longer gets buried under every later test falsely reporting "not ok" either.
* Drops the giant unfolded message that annotations used to carry, which is what probably caused the scrolling behavior.

Harald Nordgren (2):
ci: annotate leaks and stop a leak-sanitizer script at its first
failure
ci: point test failures and fixed known breakages at their file and
line

 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 57 ++++++++++++++++++++++++----
 t/test-lib.sh                        | 21 ++++++----
 3 files changed, 63 insertions(+), 16 deletions(-)

base-commit: 34f06850c16c7f7ac822b1adc71354f11b0f2ca3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2419%2FHaraldNordgren%2Fci-annotation-file-line-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2419/HaraldNordgren/ci-annotation-file-line-v2
Pull-Request: https://github.com/git/git/pull/2419

Range-diff vs v1:

1: 61b0355780 ! 1: 46e13a6e77 ci: point leak-sanitizer failures at the actual test and error

     @@ Metadata
      Author: Harald Nordgren <[email protected]>
      
       ## Commit message ##
     -    ci: point leak-sanitizer failures at the actual test and error
     +    ci: annotate leaks and stop a leak-sanitizer script at its first failure
      
     -    A leak is only found once, at the end of a whole script, well after
     -    every test already reported ok, and the failure annotation carried
     -    no file or line, so all a reviewer ever saw was:
     +    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.
      
     -    with nothing to click through to. Stop each leak-sanitizer script at
     -    its first failure instead of running the rest of an already-tainted
     -    script, and have both failure and leak annotations point at the real
     -    file and carry the actual error, for example:
     +    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.
      
     -        t/t1507-rev-parse-upstream.sh, line 1:
     -        memory leak logged around t1507.1
     -        ==ERROR: LeakSanitizer: detected memory leaks
     -        Direct leak of 60 byte(s) in 1 object(s) allocated from:
     -            ...
     -            #5 in add_branch builtin/remote.c:135
     +    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.
     +
     +    A failing test already gets its own annotation once its script
     +    finishes, but --immediate exits as soon as that test fails, before
     +    reaching the code that writes it. Write the annotation first, so
     +    turning on --immediate here does not silently drop it.
      
          Signed-off-by: Harald Nordgren <[email protected]>
      
     @@ t/test-lib-github-workflow-markup.sh: start_test_output () {
       	>$github_markup_output
       	GIT_TEST_TEE_OFFSET=0
      +	github_markup_script_name=${0##*/}
     ++}
     ++
     ++github_annotation_ () {
     ++	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
       }
       
       # No need to override start_test_case_output
     -@@ t/test-lib-github-workflow-markup.sh: start_test_output () {
     - finalize_test_case_output () {
     - 	test_case_result=$1
     - 	shift
     -+
     -+	case "$test_case_result" in
     -+	ok|broken)
     -+		# Exit without printing the "ok" or "broken" tests
     -+		return
     -+		;;
     -+	esac
     -+
     -+	test_case_line=$(find_test_case_line_ "$1")
     -+	test_case_output=$(test-tool path-utils skip-n-bytes \
     -+		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET)
     -+
     - 	case "$test_case_result" in
     - 	failure)
     --		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1"
     -+		test_case_summary=$(printf '%s\n' "$test_case_output" |
     -+			tail -n 20 | github_escape_message_)
     -+		github_annotation_ error "t/$github_markup_script_name" "${test_case_line:-1}" \
     -+			"failed: $this_test.$test_count $1%0A%0A$test_case_summary"
     - 		;;
     - 	fixed)
     --		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
     --		;;
     --	ok|broken)
     --		# Exit without printing the "ok" or ""broken" tests
     --		return
     -+		github_annotation_ notice "t/$github_markup_script_name" "${test_case_line:-1}" \
     -+			"fixed: $this_test.$test_count $1"
     - 		;;
     - 	esac
     -+
     - 	echo >>$github_markup_output "::group::$test_case_result: $this_test.$test_count $*"
     --	test-tool >>$github_markup_output path-utils skip-n-bytes \
     --		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET
     -+	printf '%s\n' "$test_case_output" >>$github_markup_output
     +@@ t/test-lib-github-workflow-markup.sh: finalize_test_case_output () {
       	echo >>$github_markup_output "::endgroup::"
       }
       
      +finalize_test_leak_output () {
     -+	test_leak_summary=$(head -n 40 "$TEST_RESULTS_SAN_FILE".* |
     -+		github_escape_message_)
     ++	# 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 around $this_test.$test_count%0A%0A$test_leak_summary"
     -+}
     ++		"memory leak logged in $this_test"
      +
     - # No need to override finalize_test_output
     -+
     -+github_escape_message_ () {
     -+	sed -e ':a' -e 'N' -e '$!ba' -e 's/%/%25/g' -e 's/\r/%0D/g' -e 's/\n/%0A/g'
     -+}
     -+
     -+find_test_case_line_ () {
     -+	grep -n -F -- "$1" "$TEST_DIRECTORY/$github_markup_script_name" |
     -+	head -n 1 | cut -d: -f1
     ++	echo >>$github_markup_output "::group::leak: $this_test.$test_count"
     ++	cat "$TEST_RESULTS_SAN_FILE".* >>$github_markup_output
     ++	echo >>$github_markup_output "::endgroup::"
      +}
      +
     -+github_annotation_ () {
     -+	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
     -+}
     + # No need to override finalize_test_output
      
       ## t/test-lib.sh ##
      @@ t/test-lib.sh: mark_option_requires_arg () {
     @@ t/test-lib.sh: mark_option_requires_arg () {
       finalize_test_output () { :; }
       
       parse_option () {
     +@@ t/test-lib.sh: 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
     +-	finalize_test_case_output failure "$failure_label" "$@"
     + }
     + 
     + test_known_broken_ok_ () {
      @@ t/test-lib.sh: check_test_results_san_file_ () {
       		return
       	fi &&

-: ---------- > 2: bffa8fb030 ci: point test failures and fixed known breakages at their file and line

gitgitgadget

From: Harald Nordgren <[email protected]>

A test failure or a fixed known breakage gets an annotation that names the test but carries no file or line, so there is nothing to click through to from the GitHub UI.

Find the line a test is defined on by searching the script for its description as a fixed string, using the first match. A description can contain characters like [ or * that a regex search would misread, so match it literally. Fall back to line 1 when the description is not found verbatim, which happens when a test builds its description at runtime instead of writing it out literally.

A GitHub annotation is a single line, and a test description is always one line too, so only a % or a stray carriage return in it needs percent-encoding to keep the annotation intact. Escape % first, or a carriage return's own encoding would be mangled by a % substitution that ran after it.

---
 t/test-lib-github-workflow-markup.sh | 41 ++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 8 deletions(-)

index 0d54496358..67c5c3461c 100644
@@ -31,6 +31,21 @@ start_test_output () {
 	github_markup_script_name=${0##*/}
 }
 
+github_escape_message_ () {
+	# A test description is always one line, so only % and CR need
+	# escaping here. Escape % first, or CR's own %-encoding gets mangled.
+	sed -e 's/%/%25/g' -e 's/\r/%0D/g'
+}
+
+find_test_case_line_ () {
+	# A description can contain characters like [ or * that would
+	# corrupt a regex search, so match it literally and take the first
+	# hit; -- keeps a description starting with "-" from being read as
+	# an option.
+	grep -n -F -- "$1" "$TEST_DIRECTORY/$github_markup_script_name" |
+	head -n 1 | cut -d: -f1
+}
+
 github_annotation_ () {
 	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
 }

@@ -40,21 +55,31 @@ github_annotation_ () {
 finalize_test_case_output () {
 	test_case_result=$1
 	shift
+
+	case "$test_case_result" in
+	ok|broken)
+		# Exit without printing the "ok" or "broken" tests
+		return
+		;;
+	esac
+
+	test_case_line=$(find_test_case_line_ "$1")
+	test_case_description=$(printf '%s' "$1" | github_escape_message_)
+
 	case "$test_case_result" in
 	failure)
-		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1"
+		github_annotation_ error "t/$github_markup_script_name" "${test_case_line:-1}" \
+			"failed: $this_test.$test_count $test_case_description"
 		;;
 	fixed)
-		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
-		;;
-	ok|broken)
-		# Exit without printing the "ok" or ""broken" tests
-		return
+		github_annotation_ notice "t/$github_markup_script_name" "${test_case_line:-1}" \
+			"fixed: $this_test.$test_count $test_case_description"
 		;;
 	esac
+
 	echo >>$github_markup_output "::group::$test_case_result: $this_test.$test_count $*"
-	test-tool >>$github_markup_output path-utils skip-n-bytes \
-		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET
+	test-tool path-utils skip-n-bytes \
+		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET >>$github_markup_output
 	echo >>$github_markup_output "::endgroup::"
 }
 

gitgitgadget

From: Harald Nordgren <[email protected]>

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.

A failing test already gets its own annotation once its script finishes, but --immediate exits as soon as that test fails, before reaching the code that writes it. Write the annotation first, so turning on --immediate here does not silently drop it.

---
 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++
 t/test-lib.sh                        | 21 +++++++++++++--------
 3 files changed, 30 insertions(+), 8 deletions(-)

index c6ccbf8c17..a89f480a78 100755
@@ -382,6 +382,7 @@ linux-leaks|linux-reftable-leaks)
 	export NO_CVS_TESTS=LetsSaveSomeTime
 	export NO_SVN_TESTS=LetsSaveSomeTime
 	export NO_P4_TESTS=LetsSaveSomeTime
+	GIT_TEST_OPTS="$GIT_TEST_OPTS --immediate"
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined

index fa29a62aa3..0d54496358 100644
@@ -28,6 +28,11 @@ start_test_output () {
 	github_markup_output="${GIT_TEST_TEE_OUTPUT_FILE%.out}.markup"
 	>$github_markup_output
 	GIT_TEST_TEE_OFFSET=0
+	github_markup_script_name=${0##*/}
+}
+
+github_annotation_ () {
+	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
 }
 
 # No need to override start_test_case_output

@@ -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::"
+}
+
 # No need to override finalize_test_output

index 1f0505e412..3552a19323 100644
@@ -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
-	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

gitgitgadget

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?

Yes that's it, one leak makes continuing pointless since every later test would just see the same accumulated log, so we stop there instead.

I'll take a look at that, it might be a problem.

Harald

Link failure and leak annotations in CI to the test script, so both can be found from the job summary.

V3 CI Job where failures and leaks are reported:
https://github.com/git/git/actions/runs/36537917146/job/109306215909?pr=2426

Changes in v3:

* Fixed bug in the --immediate exit ordering: the --immediate && --invert-exit-code path called exit 0 before the test's annotation was written, now a single unconditional call covers both exit paths.
* github_escape_message_ no longer relies on \r being a portable sed escape sequence (not POSIX-guaranteed and BSD sed implementations can differ), it splices in the literal carriage-return byte via printf instead.
* Reverted unrelated test-tool line back to its original form.

Changes in v2:

* Split into two commits, each explaining its own reasoning.
* Leak output is no longer capped or embedded in the message, it's now an uncapped fold, so multiple leaks in the same test both show in full. A second leak in a different test still won't show in the same run, --immediate stops the script at the first failure, but it no longer gets buried under every later test falsely reporting "not ok" either.
* Drops the giant unfolded message that annotations used to carry, which is what probably caused the scrolling behavior.

Harald Nordgren (2):
ci: annotate leaks and stop a leak-sanitizer script at its first
failure
ci: point test failures and fixed known breakages at their file and
line

 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 54 ++++++++++++++++++++++++----
 t/test-lib.sh                        |  6 +++-
 3 files changed, 54 insertions(+), 7 deletions(-)

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2419%2FHaraldNordgren%2Fci-annotation-file-line-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2419/HaraldNordgren/ci-annotation-file-line-v3
Pull-Request: https://github.com/git/git/pull/2419

Range-diff vs v2:

1: 46e13a6e77 ! 1: b6a36820ae ci: annotate leaks and stop a leak-sanitizer script at its first failure

     @@ t/test-lib.sh: 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.
     ++	# Write the annotation before either --immediate exit path below,
     ++	# both of 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
     +@@ t/test-lib.sh: test_failure_ () {
       		check_test_results_san_file_ "$test_failure"
       		_error_exit
       	fi
 2:  bffa8fb030 ! 2:  750c360512 ci: point test failures and fixed known breakages at their file and line
     @@ t/test-lib-github-workflow-markup.sh: start_test_output () {
      +github_escape_message_ () {
      +	# A test description is always one line, so only % and CR need
      +	# escaping here. Escape % first, or CR's own %-encoding gets mangled.
     -+	sed -e 's/%/%25/g' -e 's/\r/%0D/g'
     ++	# \r is not a portable sed escape, so splice in the actual byte.
     ++	sed -e 's/%/%25/g' -e "s/$(printf '\r')/%0D/g"
      +}
      +
      +find_test_case_line_ () {
     @@ t/test-lib-github-workflow-markup.sh: github_annotation_ () {
       	esac
      +
       	echo >>$github_markup_output "::group::$test_case_result: $this_test.$test_count $*"
     --	test-tool >>$github_markup_output path-utils skip-n-bytes \
     --		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET
     -+	test-tool path-utils skip-n-bytes \
     -+		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET >>$github_markup_output
     - 	echo >>$github_markup_output "::endgroup::"
     - }
     - 
     + 	test-tool >>$github_markup_output path-utils skip-n-bytes \
     + 		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET

gitgitgadget

From: Harald Nordgren <[email protected]>

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.

A failing test already gets its own annotation once its script finishes, but --immediate exits as soon as that test fails, before reaching the code that writes it. Write the annotation first, so turning on --immediate here does not silently drop it.

---
 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++
 t/test-lib.sh                        |  6 +++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

index c6ccbf8c17..a89f480a78 100755
@@ -382,6 +382,7 @@ linux-leaks|linux-reftable-leaks)
 	export NO_CVS_TESTS=LetsSaveSomeTime
 	export NO_SVN_TESTS=LetsSaveSomeTime
 	export NO_P4_TESTS=LetsSaveSomeTime
+	GIT_TEST_OPTS="$GIT_TEST_OPTS --immediate"
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined

index fa29a62aa3..0d54496358 100644
@@ -28,6 +28,11 @@ start_test_output () {
 	github_markup_output="${GIT_TEST_TEE_OUTPUT_FILE%.out}.markup"
 	>$github_markup_output
 	GIT_TEST_TEE_OFFSET=0
+	github_markup_script_name=${0##*/}
+}
+
+github_annotation_ () {
+	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
 }
 
 # No need to override start_test_case_output

@@ -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::"
+}
+
 # No need to override finalize_test_output

index 1f0505e412..e74a12f1dd 100644
@@ -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,6 +823,9 @@ test_failure_ () {
 	say_color error "not ok $test_count - ${pfx:+$pfx }$1"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
+	# Write the annotation before either --immediate exit path below,
+	# both of 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"

@@ -835,7 +839,6 @@ test_failure_ () {
 		check_test_results_san_file_ "$test_failure"
 		_error_exit
 	fi
-	finalize_test_case_output failure "$failure_label" "$@"
 }
 
 test_known_broken_ok_ () {

@@ -1218,6 +1221,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

gitgitgadget

From: Harald Nordgren <[email protected]>

A test failure or a fixed known breakage gets an annotation that names the test but carries no file or line, so there is nothing to click through to from the GitHub UI.

Find the line a test is defined on by searching the script for its description as a fixed string, using the first match. A description can contain characters like [ or * that a regex search would misread, so match it literally. Fall back to line 1 when the description is not found verbatim, which happens when a test builds its description at runtime instead of writing it out literally.

A GitHub annotation is a single line, and a test description is always one line too, so only a % or a stray carriage return in it needs percent-encoding to keep the annotation intact. Escape % first, or a carriage return's own encoding would be mangled by a % substitution that ran after it.

---
 t/test-lib-github-workflow-markup.sh | 38 +++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 6 deletions(-)

index 0d54496358..66d2ccca18 100644
@@ -31,6 +31,22 @@ start_test_output () {
 	github_markup_script_name=${0##*/}
 }
 
+github_escape_message_ () {
+	# A test description is always one line, so only % and CR need
+	# escaping here. Escape % first, or CR's own %-encoding gets mangled.
+	# \r is not a portable sed escape, so splice in the actual byte.
+	sed -e 's/%/%25/g' -e "s/$(printf '\r')/%0D/g"
+}
+
+find_test_case_line_ () {
+	# A description can contain characters like [ or * that would
+	# corrupt a regex search, so match it literally and take the first
+	# hit; -- keeps a description starting with "-" from being read as
+	# an option.
+	grep -n -F -- "$1" "$TEST_DIRECTORY/$github_markup_script_name" |
+	head -n 1 | cut -d: -f1
+}
+
 github_annotation_ () {
 	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
 }

@@ -40,18 +56,28 @@ github_annotation_ () {
 finalize_test_case_output () {
 	test_case_result=$1
 	shift
+
+	case "$test_case_result" in
+	ok|broken)
+		# Exit without printing the "ok" or "broken" tests
+		return
+		;;
+	esac
+
+	test_case_line=$(find_test_case_line_ "$1")
+	test_case_description=$(printf '%s' "$1" | github_escape_message_)
+
 	case "$test_case_result" in
 	failure)
-		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1"
+		github_annotation_ error "t/$github_markup_script_name" "${test_case_line:-1}" \
+			"failed: $this_test.$test_count $test_case_description"
 		;;
 	fixed)
-		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
-		;;
-	ok|broken)
-		# Exit without printing the "ok" or ""broken" tests
-		return
+		github_annotation_ notice "t/$github_markup_script_name" "${test_case_line:-1}" \
+			"fixed: $this_test.$test_count $test_case_description"
 		;;
 	esac
+
 	echo >>$github_markup_output "::group::$test_case_result: $this_test.$test_count $*"
 	test-tool >>$github_markup_output path-utils skip-n-bytes \
 		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET

gitgitgadget

With these updates, the patches look good to me. Unless others spot problems I failed to see, let me mark the topic for 'next'.

Thanks.

Hi Harald

It is good that it reports the full LSAN but is it really necessary to emphasize that - why would anyone reading this think it might be abbreviated?

What does adding the file and line number to the annotation buy us? I was hoping there'd be a link in the output to the test source but I can't see anything like that. The output is displayed as

    Error: failed: t7603.3 pull c2, c3, c4, c5 into c1
    ▶failure: t7603.3 pull c2, c3, c4, c5 into c1
    Error: memory leak logged in t7603
    ▶leak: t7603.3

and clicking on the '▶' lines expands the output, but there is nothing about the source file as far as I can see. Ideally, if the test is failing due to a leak, it would be nice to report that as

    Error: leak detected in: t7603.3 pull c2, c3, c4, c5 into c1
    ▶failure: t7603.3 pull c2, c3, c4, c5 into c1

and display the test output and LSAN output together when the "▶failure:" line is clicked, rather than having separate sections for the test output and leak output. Having said that what you've already implemented is a clear improvement so I'd be happy to take that if you don't feel like devoting any more time to it.

I think that is probably a welcome improvement though if two different tests have different leaks it would be nice to be able to show both.

Thanks for working on this, it makes the LSAN output much more accessible.

Phillip

Hi Harald

Have you got an example of this? As I said in my last mail, I can't see any links in the output from the linux-leaks job.

This second sentence doesn't really add anything - you've already said we're searching for a fixed string.

Ironically, it is the dynamically generated tests where a line number would be most useful, but there is no easy way to determine what line we should be using.

Why do we need to escape the test descriptions when we haven't been doing so up to now? Also if the test description is a single line why are we worring about '\r'? If it is so important to escape the output why does this patch not convert the existing annotations like the "group::" on in the trailing context lines?

Thanks

Phillip

I've left a couple of comments. This version is a nice improvement on the status quo, but I'd like some clarity on what the filename and line number annotations actually do, and why we selectively escape the annotations.

Thanks

Phillip

That's poor wording on my side, I'll clarify.

Yeah, that can be simplified.

Harald

Thanks.

Link failure and leak annotations in CI to the test script, so both can be found from the job summary.

V4 CI Job where failures and leaks are reported:
https://github.com/git/git/actions/runs/36760014277/job/110039964565?pr=2426

Changes in v4:

* Clarify commit messages and simplify escaping logic.

Changes in v3:

* Fixed bug in the --immediate exit ordering: the --immediate && --invert-exit-code path called exit 0 before the test's annotation was written, now a single unconditional call covers both exit paths.
* github_escape_message_ no longer relies on \r being a portable sed escape sequence (not POSIX-guaranteed and BSD sed implementations can differ), it splices in the literal carriage-return byte via printf instead.
* Reverted unrelated test-tool line back to its original form.

Changes in v2:

* Split into two commits, each explaining its own reasoning.
* Leak output is no longer capped or embedded in the message, it's now an uncapped fold, so multiple leaks in the same test both show in full. A second leak in a different test still won't show in the same run, --immediate stops the script at the first failure, but it no longer gets buried under every later test falsely reporting "not ok" either.
* Drops the giant unfolded message that annotations used to carry, which is what probably caused the scrolling behavior.

Harald Nordgren (2):
ci: annotate leaks and stop a leak-sanitizer script at its first
failure
ci: point test failures and fixed known breakages at their file and
line

 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 54 ++++++++++++++++++++++++----
 t/test-lib.sh                        |  6 +++-
 3 files changed, 54 insertions(+), 7 deletions(-)

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2419%2FHaraldNordgren%2Fci-annotation-file-line-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2419/HaraldNordgren/ci-annotation-file-line-v4
Pull-Request: https://github.com/git/git/pull/2419

Range-diff vs v3:

1: b6a36820ae ! 1: 138394b48b ci: annotate leaks and stop a leak-sanitizer script at its first failure
@@ Commit message

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.
+ sanitizer report in a log group next to it, so it stays visible.

Once a script has one leak, it keeps running: the sanitizer log
directory is never cleared between tests, so every later test in the
2: 750c360512 ! 2: 8ec2b53d82 ci: point test failures and fixed known breakages at their file and line
@@ Commit message
ci: point test failures and fixed known breakages at their file and line

A test failure or a fixed known breakage gets an annotation that names
- the test but carries no file or line, so there is nothing to click
- through to from the GitHub UI.
+ the test but says nothing about where it's defined, so a reviewer has
+ to search the script by hand to find it.

Find the line a test is defined on by searching the script for its
- description as a fixed string, using the first match. A description
- can contain characters like [ or * that a regex search would
- misread, so match it literally. Fall back to line 1 when the
- description is not found verbatim, which happens when a test builds
- its description at runtime instead of writing it out literally.
+ description as a fixed string, using the first match. Fall back to
+ line 1 when the description is not found verbatim, which happens when
+ a test builds its description at runtime instead of writing it out
+ literally.

- A GitHub annotation is a single line, and a test description is always
- one line too, so only a % or a stray carriage return in it needs
- percent-encoding to keep the annotation intact. Escape % first, or a
- carriage return's own encoding would be mangled by a % substitution
- that ran after it.
+ A GitHub annotation is a single line, so a % in a test description
+ has to be percent-encoded as %25, or GitHub misreads it as its own
+ escape sequence. for-each-ref's format atoms use plenty of them, e.g.
+ %(raw).

@@ t/test-lib-github-workflow-markup.sh: start_test_output () {
}

+github_escape_message_ () {
-+ # A test description is always one line, so only % and CR need
-+ # escaping here. Escape % first, or CR's own %-encoding gets mangled.
-+ # \r is not a portable sed escape, so splice in the actual byte.
-+ sed -e 's/%/%25/g' -e "s/$(printf '\r')/%0D/g"
++ # % has to be escaped or GitHub misreads it as the start of its own
++ # percent-encoding (e.g. a literal %(raw) in a for-each-ref test
++ # description).
++ sed -e 's/%/%25/g'
+}
+
+find_test_case_line_ () {
+ # A description can contain characters like [ or * that would
+ # corrupt a regex search, so match it literally and take the first
-+ # hit; -- keeps a description starting with "-" from being read as
-+ # an option.
++ # hit. The -- keeps a description starting with "-" from being read
++ # as an option.
+ grep -n -F -- "$1" "$TEST_DIRECTORY/$github_markup_script_name" |
+ head -n 1 | cut -d: -f1
+}

gitgitgadget

From: Harald Nordgren <[email protected]>

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 sanitizer report in a log group next to it, so it stays visible.

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.

A failing test already gets its own annotation once its script finishes, but --immediate exits as soon as that test fails, before reaching the code that writes it. Write the annotation first, so turning on --immediate here does not silently drop it.

---
 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++
 t/test-lib.sh                        |  6 +++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

index c6ccbf8c17..a89f480a78 100755
@@ -382,6 +382,7 @@ linux-leaks|linux-reftable-leaks)
 	export NO_CVS_TESTS=LetsSaveSomeTime
 	export NO_SVN_TESTS=LetsSaveSomeTime
 	export NO_P4_TESTS=LetsSaveSomeTime
+	GIT_TEST_OPTS="$GIT_TEST_OPTS --immediate"
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined

index fa29a62aa3..0d54496358 100644
@@ -28,6 +28,11 @@ start_test_output () {
 	github_markup_output="${GIT_TEST_TEE_OUTPUT_FILE%.out}.markup"
 	>$github_markup_output
 	GIT_TEST_TEE_OFFSET=0
+	github_markup_script_name=${0##*/}
+}
+
+github_annotation_ () {
+	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
 }
 
 # No need to override start_test_case_output

@@ -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::"
+}
+
 # No need to override finalize_test_output

index 1f0505e412..e74a12f1dd 100644
@@ -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,6 +823,9 @@ test_failure_ () {
 	say_color error "not ok $test_count - ${pfx:+$pfx }$1"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
+	# Write the annotation before either --immediate exit path below,
+	# both of 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"

@@ -835,7 +839,6 @@ test_failure_ () {
 		check_test_results_san_file_ "$test_failure"
 		_error_exit
 	fi
-	finalize_test_case_output failure "$failure_label" "$@"
 }
 
 test_known_broken_ok_ () {

@@ -1218,6 +1221,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

gitgitgadget

From: Harald Nordgren <[email protected]>

A test failure or a fixed known breakage gets an annotation that names the test but says nothing about where it's defined, so a reviewer has to search the script by hand to find it.

Find the line a test is defined on by searching the script for its description as a fixed string, using the first match. Fall back to line 1 when the description is not found verbatim, which happens when a test builds its description at runtime instead of writing it out literally.

A GitHub annotation is a single line, so a % in a test description has to be percent-encoded as %25, or GitHub misreads it as its own escape sequence. for-each-ref's format atoms use plenty of them, e.g. %(raw).

---
 t/test-lib-github-workflow-markup.sh | 38 +++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 6 deletions(-)

index 0d54496358..6fee4dfb22 100644
@@ -31,6 +31,22 @@ start_test_output () {
 	github_markup_script_name=${0##*/}
 }
 
+github_escape_message_ () {
+	# % has to be escaped or GitHub misreads it as the start of its own
+	# percent-encoding (e.g. a literal %(raw) in a for-each-ref test
+	# description).
+	sed -e 's/%/%25/g'
+}
+
+find_test_case_line_ () {
+	# A description can contain characters like [ or * that would
+	# corrupt a regex search, so match it literally and take the first
+	# hit. The -- keeps a description starting with "-" from being read
+	# as an option.
+	grep -n -F -- "$1" "$TEST_DIRECTORY/$github_markup_script_name" |
+	head -n 1 | cut -d: -f1
+}
+
 github_annotation_ () {
 	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
 }

@@ -40,18 +56,28 @@ github_annotation_ () {
 finalize_test_case_output () {
 	test_case_result=$1
 	shift
+
+	case "$test_case_result" in
+	ok|broken)
+		# Exit without printing the "ok" or "broken" tests
+		return
+		;;
+	esac
+
+	test_case_line=$(find_test_case_line_ "$1")
+	test_case_description=$(printf '%s' "$1" | github_escape_message_)
+
 	case "$test_case_result" in
 	failure)
-		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1"
+		github_annotation_ error "t/$github_markup_script_name" "${test_case_line:-1}" \
+			"failed: $this_test.$test_count $test_case_description"
 		;;
 	fixed)
-		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
-		;;
-	ok|broken)
-		# Exit without printing the "ok" or ""broken" tests
-		return
+		github_annotation_ notice "t/$github_markup_script_name" "${test_case_line:-1}" \
+			"fixed: $this_test.$test_count $test_case_description"
 		;;
 	esac
+
 	echo >>$github_markup_output "::group::$test_case_result: $this_test.$test_count $*"
 	test-tool >>$github_markup_output path-utils skip-n-bytes \
 		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET

gitgitgadget

Hi Harald

I'm afraid I'm still not clear what this does in practical terms. What appears in the test output that the user sees that didn't before?

That's a useful example of why we want to escape the output which makes it all the more puzzling that we don't escape the existing annotations that I mentioned last time.

Thanks

Phillip

I agree this is still hard to read. My interpretation of the above is

We only say "the t1234 script failed" (in the first paragraph that makes an observation of the status quo), and we try to find the test_expect_success block and show it as the finer-grained clue (the second paragraph).

but that may be way off the mark.

Thanks.

This feels like a rabbit hole and probably better to just drop the escaping altogether. It seems that the only thing that would need escaping is the literal '%25', '%' in ASCII, but it doesn't even appear in any of our tests. See:

- https://github.com/git/git/actions/runs/36976989079/job/110742888375?pr=2435
- https://github.com/git/git/actions/runs/36977041388/job/110743045462?pr=2436

I'll just drop this now. If needed, better to pick it up in a different topic. Thanks for pursuing this!

Harald

Link failure and leak annotations in CI to the test script, so both can be found from the job summary.

V5 CI Job where failures and leaks are reported:
https://github.com/git/git/actions/runs/36979818141/job/110752027863?pr=2426

Changes in v5:

* Removed % escaping entirely, verified on CI that it isn't needed. Every existing test description that uses % renders correctly unescaped.
* Rewrote the file/line commit message with a concrete example (failed:
t1060.17 partial clone of corrupted repository).

Changes in v4:

* Clarify commit messages and simplify escaping logic.

Changes in v3:

* Fixed bug in the --immediate exit ordering: the --immediate && --invert-exit-code path called exit 0 before the test's annotation was written, now a single unconditional call covers both exit paths.
* github_escape_message_ no longer relies on \r being a portable sed escape sequence (not POSIX-guaranteed and BSD sed implementations can differ), it splices in the literal carriage-return byte via printf instead.
* Reverted unrelated test-tool line back to its original form.

Changes in v2:

* Split into two commits, each explaining its own reasoning.
* Leak output is no longer capped or embedded in the message, it's now an uncapped fold, so multiple leaks in the same test both show in full. A second leak in a different test still won't show in the same run, --immediate stops the script at the first failure, but it no longer gets buried under every later test falsely reporting "not ok" either.
* Drops the giant unfolded message that annotations used to carry, which is what probably caused the scrolling behavior.

Harald Nordgren (2):
ci: annotate leaks and stop a leak-sanitizer script at its first
failure
ci: point test failures and fixed known breakages at their file and
line

 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 46 ++++++++++++++++++++++++----
 t/test-lib.sh                        |  6 +++-
 3 files changed, 46 insertions(+), 7 deletions(-)

base-commit: c46c1e37724f0478939de636ab8ea5a89086d532
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2419%2FHaraldNordgren%2Fci-annotation-file-line-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2419/HaraldNordgren/ci-annotation-file-line-v5
Pull-Request: https://github.com/git/git/pull/2419

Range-diff vs v4:

 1:  138394b48b = 1:  851efeec8b ci: annotate leaks and stop a leak-sanitizer script at its first failure
 2:  8ec2b53d82 ! 2:  46f93a9e16 ci: point test failures and fixed known breakages at their file and line
     @@ Metadata
       ## Commit message ##
          ci: point test failures and fixed known breakages at their file and line
      
     -    A test failure or a fixed known breakage gets an annotation that names
     -    the test but says nothing about where it's defined, so a reviewer has
     -    to search the script by hand to find it.
     +    When a test fails, GitHub shows an annotation naming it, for example:
      
     -    Find the line a test is defined on by searching the script for its
     -    description as a fixed string, using the first match. Fall back to
     +        failed: t1060.17 partial clone of corrupted repository
     +
     +    but the location GitHub attaches to that annotation is the CI
     +    workflow file itself, not the test script, so there is nothing
     +    pointing at where the test actually lives.
     +
     +    Find the line a test is defined on by searching its script for the
     +    test's own description as a fixed string, using the first match, and
     +    attach that file and line to the annotation instead. Fall back to
          line 1 when the description is not found verbatim, which happens when
          a test builds its description at runtime instead of writing it out
          literally.
      
     -    A GitHub annotation is a single line, so a `%` in a test description
     -    has to be percent-encoded as `%25`, or GitHub misreads it as its own
     -    escape sequence. for-each-ref's format atoms use plenty of them, e.g.
     -    `%(raw)`.
     -
          Signed-off-by: Harald Nordgren <[email protected]>
      
       ## t/test-lib-github-workflow-markup.sh ##
     @@ t/test-lib-github-workflow-markup.sh: start_test_output () {
       	github_markup_script_name=${0##*/}
       }
       
     -+github_escape_message_ () {
     -+	# % has to be escaped or GitHub misreads it as the start of its own
     -+	# percent-encoding (e.g. a literal %(raw) in a for-each-ref test
     -+	# description).
     -+	sed -e 's/%/%25/g'
     -+}
     -+
      +find_test_case_line_ () {
      +	# A description can contain characters like [ or * that would
      +	# corrupt a regex search, so match it literally and take the first
     @@ t/test-lib-github-workflow-markup.sh: github_annotation_ () {
      +	esac
      +
      +	test_case_line=$(find_test_case_line_ "$1")
     -+	test_case_description=$(printf '%s' "$1" | github_escape_message_)
      +
       	case "$test_case_result" in
       	failure)
      -		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1"
      +		github_annotation_ error "t/$github_markup_script_name" "${test_case_line:-1}" \
     -+			"failed: $this_test.$test_count $test_case_description"
     ++			"failed: $this_test.$test_count $1"
       		;;
       	fixed)
      -		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
     @@ t/test-lib-github-workflow-markup.sh: github_annotation_ () {
      -		# Exit without printing the "ok" or ""broken" tests
      -		return
      +		github_annotation_ notice "t/$github_markup_script_name" "${test_case_line:-1}" \
     -+			"fixed: $this_test.$test_count $test_case_description"
     ++			"fixed: $this_test.$test_count $1"
       		;;
       	esac
      +

gitgitgadget

From: Harald Nordgren <[email protected]>

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 sanitizer report in a log group next to it, so it stays visible.

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.

A failing test already gets its own annotation once its script finishes, but --immediate exits as soon as that test fails, before reaching the code that writes it. Write the annotation first, so turning on --immediate here does not silently drop it.

---
 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++
 t/test-lib.sh                        |  6 +++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

index c6ccbf8c17..a89f480a78 100755
@@ -382,6 +382,7 @@ linux-leaks|linux-reftable-leaks)
 	export NO_CVS_TESTS=LetsSaveSomeTime
 	export NO_SVN_TESTS=LetsSaveSomeTime
 	export NO_P4_TESTS=LetsSaveSomeTime
+	GIT_TEST_OPTS="$GIT_TEST_OPTS --immediate"
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined

index fa29a62aa3..0d54496358 100644
@@ -28,6 +28,11 @@ start_test_output () {
 	github_markup_output="${GIT_TEST_TEE_OUTPUT_FILE%.out}.markup"
 	>$github_markup_output
 	GIT_TEST_TEE_OFFSET=0
+	github_markup_script_name=${0##*/}
+}
+
+github_annotation_ () {
+	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
 }
 
 # No need to override start_test_case_output

@@ -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::"
+}
+
 # No need to override finalize_test_output

index 321c2ba339..a893963f86 100644
@@ -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,6 +823,9 @@ test_failure_ () {
 	say_color error "not ok $test_count - ${pfx:+$pfx }$1"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
+	# Write the annotation before either --immediate exit path below,
+	# both of 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"

@@ -835,7 +839,6 @@ test_failure_ () {
 		check_test_results_san_file_ "$test_failure"
 		_error_exit
 	fi
-	finalize_test_case_output failure "$failure_label" "$@"
 }
 
 test_known_broken_ok_ () {

@@ -1218,6 +1221,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

gitgitgadget

From: Harald Nordgren <[email protected]>

When a test fails, GitHub shows an annotation naming it, for example:

failed: t1060.17 partial clone of corrupted repository

but the location GitHub attaches to that annotation is the CI workflow file itself, not the test script, so there is nothing pointing at where the test actually lives.

Find the line a test is defined on by searching its script for the test's own description as a fixed string, using the first match, and attach that file and line to the annotation instead. Fall back to line 1 when the description is not found verbatim, which happens when a test builds its description at runtime instead of writing it out literally.

---
 t/test-lib-github-workflow-markup.sh | 30 ++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

index 0d54496358..ac8c536231 100644
@@ -31,6 +31,15 @@ start_test_output () {
 	github_markup_script_name=${0##*/}
 }
 
+find_test_case_line_ () {
+	# A description can contain characters like [ or * that would
+	# corrupt a regex search, so match it literally and take the first
+	# hit. The -- keeps a description starting with "-" from being read
+	# as an option.
+	grep -n -F -- "$1" "$TEST_DIRECTORY/$github_markup_script_name" |
+	head -n 1 | cut -d: -f1
+}
+
 github_annotation_ () {
 	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
 }

@@ -40,18 +49,27 @@ github_annotation_ () {
 finalize_test_case_output () {
 	test_case_result=$1
 	shift
+
+	case "$test_case_result" in
+	ok|broken)
+		# Exit without printing the "ok" or "broken" tests
+		return
+		;;
+	esac
+
+	test_case_line=$(find_test_case_line_ "$1")
+
 	case "$test_case_result" in
 	failure)
-		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1"
+		github_annotation_ error "t/$github_markup_script_name" "${test_case_line:-1}" \
+			"failed: $this_test.$test_count $1"
 		;;
 	fixed)
-		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
-		;;
-	ok|broken)
-		# Exit without printing the "ok" or ""broken" tests
-		return
+		github_annotation_ notice "t/$github_markup_script_name" "${test_case_line:-1}" \
+			"fixed: $this_test.$test_count $1"
 		;;
 	esac
+
 	echo >>$github_markup_output "::group::$test_case_result: $this_test.$test_count $*"
 	test-tool >>$github_markup_output path-utils skip-n-bytes \
 		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET

gitgitgadget

Hi Harald

There does not appear to be any output relating to leaks in that job. The first patch hasn't changed so I'm not sure why that is.

You have added

When a test fails, GitHub shows an annotation naming it, for example:

failed: t1060.17 partial clone of corrupted repository

which shows an example of the current output without the filename or line annotations. There is no example of what that output changes to, so there is no way for someone reading that message to see what has actually changed. After spending some time clicking around in Github I think what that patch changes is not the test output of individual jobs which you linked to above, but what is displayed on the summary page at

https://github.com/git/git/actions/runs/36979818141?pr=2426

That page shows a list of annotations with links to the changes in the failed test file. That is a useful improvement but how you expected someone reading the commit message to understand what had changed when you did not give an example of the new output, and the changes are on a different page to the one you linked to in the cover letter is beyond me. I'm pretty exasperated that I've had to spend time messing about on Github trying to see what has changed because you could not provide a link and write a couple of sentences explaining it. After asking what this change did in v3 you replied that the commit message wasn't clear without explaining what the change actually did. When I asked what the change did in practical terms in response to v4 I got no reply. As you already know reviewer time is short on this list, so please, when someone asks a question answer it rather than replying with an obtuse comment or simply ignoring it and sending another patch.

Both these patches are useful improvements, but trying to get an explanation of what they did has been like trying getting blood out of a stone.

Thanks

Phillip

But it seems it is only useful if the test changed is in that example. If I look at the summary for the CI run from v3 of this series [1] then I can see a test failure in t1022

linux-leaks(ubuntu-rolling): t/t1022-read-tree-partial-clone.sh#L8
failed: t1022.1 read-tree in partial clone prefetches in one batch

If I click on the link [2] it does not take me to that test file though, because it was not changed. That makes this somewhat less useful than I initially thought. The current behavior is that when you click on those links in the summary page it takes you to the test output for the job that failed which seems more useful. For example [3] is recent test run that had a leak and clicking on

linux-leaks:(ubuntu-rolling):
failed: t1092.58 submodule handling

Takes me to [4] which where I can click to expand the output of the failing test.

Thanks

Phillip

[1] https://github.com/git/git/actions/runs/36537917146?pr=2426
[2] https://github.com/git/git/pull/2426/files#annotation_82189987516
[3] https://github.com/benknoble/git/actions/runs/36033463504
[4] https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:9:5333

Yes, unfortunately GitHub won't let us link to a line that was not changed in that PR.

Is this enough to call this a regression? Then maybe it's not worth doing this part at all.

Harald

I don't understand why this tone is necessary at all.

Yes, I should clarify that it affects the summary.

But I posted a comment regarding the regression you brought up in your following message, so we should decide what we want before progressing. The point of this whole topic was to make the leak reporting less bad, everything else was a bonus.

Harald

Hi Harald

Yes, I think we should drop this patch. The first step to debugging a test failure is to look at the test output, so the current behavior where clicking on the links on the summary page takes you to the test output is more useful than taking you to a diff that may not even show the test that failed. The first patch is definitely worth keeping as it makes it much easier to see the LSAN output.

Thanks

Phillip

I played with instead showing the file name (and line when available) as part of the annotation text, and leaving the linking as it is. I think it could gives us the best of both worlds:

memory leak logged in t1060 (t1060-object-corruption.sh)

and

failed: t1060.17 partial clone of corrupted repository
(t1060-object-corruption.sh:141)

What do you think?

Harald

I guess having a bit more detail could be useful, I certainly don't object.

Thanks

Phillip

Link failure and leak annotations in CI to the test script, so both can be found from the job summary.

V7 CI job where failures and leaks are reported:

* https://github.com/git/git/actions/runs/37205086805?pr=2426

Changes in v7:

* Revert the linking in annotation message, direct direct line fails when error pointed to an unchanged file in that PR, which regresses the experiences in many cases. Instead now show the file and line as plain text in the annotation.
* Don't name the the line number as 1 for leaks, where it's never available, just omit the line number.

Changes in v6:

* Update commit message.

Changes in v5:

* Removed % escaping entirely, verified on CI that it isn't needed. Every existing test description that uses % renders correctly unescaped.
* Rewrote the file/line commit message with a concrete example (failed:
t1060.17 partial clone of corrupted repository).

Changes in v4:

* Clarify commit messages and simplify escaping logic.

Changes in v3:

* Fixed bug in the --immediate exit ordering: the --immediate && --invert-exit-code path called exit 0 before the test's annotation was written, now a single unconditional call covers both exit paths.
* github_escape_message_ no longer relies on \r being a portable sed escape sequence (not POSIX-guaranteed and BSD sed implementations can differ), it splices in the literal carriage-return byte via printf instead.
* Reverted unrelated test-tool line back to its original form.

Changes in v2:

* Split into two commits, each explaining its own reasoning.
* Leak output is no longer capped or embedded in the message, it's now an uncapped fold, so multiple leaks in the same test both show in full. A second leak in a different test still won't show in the same run, --immediate stops the script at the first failure, but it no longer gets buried under every later test falsely reporting "not ok" either.
* Drops the giant unfolded message that annotations used to carry, which is what probably caused the scrolling behavior.

Harald Nordgren (2):
ci: annotate leaks and stop a leak-sanitizer script at its first
failure
ci: point test failures and fixed known breakages at their file and
line

 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 39 +++++++++++++++++++++++-----
 t/test-lib.sh                        |  6 ++++-
 3 files changed, 39 insertions(+), 7 deletions(-)

base-commit: 8103b446517e0c44e67561b9d0ccce56efa60a71
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2419%2FHaraldNordgren%2Fci-annotation-file-line-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2419/HaraldNordgren/ci-annotation-file-line-v6
Pull-Request: https://github.com/git/git/pull/2419

Range-diff vs v5:

1: 851efeec8b ! 1: 917f373f91 ci: annotate leaks and stop a leak-sanitizer script at its first failure
@@ Commit message

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
- sanitizer report in a log group next to it, so it stays visible.
+ Give a leak its own annotation, naming the script it turned up in, the
+ exact line isn't known, only which script:
+
+ memory leak logged in t1060 (t1060-object-corruption.sh)
+
+ Put the sanitizer report in a log group next to it, so it stays
+ visible.

Once a script has one leak, it keeps running: the sanitizer log directory is never cleared between tests, so every later test in the
@@ t/test-lib-github-workflow-markup.sh: start_test_output () {
>$github_markup_output
GIT_TEST_TEE_OFFSET=0
+ github_markup_script_name=${0##*/}
-+}
-+
-+github_annotation_ () {
-+ echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
}

# No need to override start_test_case_output
@@ t/test-lib-github-workflow-markup.sh: finalize_test_case_output () {
}

+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 \
++ "::error::memory leak logged in $this_test ($github_markup_script_name)"
+
+ echo >>$github_markup_output "::group::leak: $this_test.$test_count"
+ cat "$TEST_RESULTS_SAN_FILE".* >>$github_markup_output
2: 46f93a9e16 ! 2: acf1fbd250 ci: point test failures and fixed known breakages at their file and line

     @@ Metadata
       ## Commit message ##
          ci: point test failures and fixed known breakages at their file and line
      
     -    When a test fails, GitHub shows an annotation naming it, for example:
     +    A failing test gets an annotation in the Annotations list on its job's
     +    summary page, naming it, for example:
      
              failed: t1060.17 partial clone of corrupted repository
      
     -    but the location GitHub attaches to that annotation is the CI
     -    workflow file itself, not the test script, so there is nothing
     -    pointing at where the test actually lives.
     +    with no indication of where that test lives.
      
          Find the line a test is defined on by searching its script for the
          test's own description as a fixed string, using the first match, and
     -    attach that file and line to the annotation instead. Fall back to
     -    line 1 when the description is not found verbatim, which happens when
     -    a test builds its description at runtime instead of writing it out
     -    literally.
     +    add the file and line to the annotation's own message text:
     +
     +        failed: t1060.17 partial clone of corrupted repository (t1060-object-corruption.sh:141)
     +
     +    Fall back to naming just the script, with no line, when the
     +    description is not found verbatim, which happens when a test builds
     +    its description at runtime instead of writing it out literally.
      
          Signed-off-by: Harald Nordgren <[email protected]>
      
     @@ t/test-lib-github-workflow-markup.sh: start_test_output () {
      +	head -n 1 | cut -d: -f1
      +}
      +
     - github_annotation_ () {
     - 	echo >>$github_markup_output "::$1 file=$2,line=$3::$4"
     - }
     -@@ t/test-lib-github-workflow-markup.sh: github_annotation_ () {
     + # No need to override start_test_case_output
     + 
       finalize_test_case_output () {
       	test_case_result=$1
       	shift
     @@ t/test-lib-github-workflow-markup.sh: github_annotation_ () {
      +	esac
      +
      +	test_case_line=$(find_test_case_line_ "$1")
     ++	test_case_where="$github_markup_script_name${test_case_line:+:$test_case_line}"
      +
       	case "$test_case_result" in
       	failure)
      -		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1"
     -+		github_annotation_ error "t/$github_markup_script_name" "${test_case_line:-1}" \
     -+			"failed: $this_test.$test_count $1"
     ++		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1 ($test_case_where)"
       		;;
       	fixed)
      -		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
     @@ t/test-lib-github-workflow-markup.sh: github_annotation_ () {
      -	ok|broken)
      -		# Exit without printing the "ok" or ""broken" tests
      -		return
     -+		github_annotation_ notice "t/$github_markup_script_name" "${test_case_line:-1}" \
     -+			"fixed: $this_test.$test_count $1"
     ++		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1 ($test_case_where)"
       		;;
       	esac
      +

gitgitgadget

From: Harald Nordgren <[email protected]>

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, naming the script it turned up in, the exact line isn't known, only which script:

memory leak logged in t1060 (t1060-object-corruption.sh)

Put the sanitizer report in a log group next to it, so it stays visible.

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.

A failing test already gets its own annotation once its script finishes, but --immediate exits as soon as that test fails, before reaching the code that writes it. Write the annotation first, so turning on --immediate here does not silently drop it.

---
 ci/lib.sh                            |  1 +
 t/test-lib-github-workflow-markup.sh | 10 ++++++++++
 t/test-lib.sh                        |  6 +++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

index c6ccbf8c17..a89f480a78 100755
@@ -382,6 +382,7 @@ linux-leaks|linux-reftable-leaks)
 	export NO_CVS_TESTS=LetsSaveSomeTime
 	export NO_SVN_TESTS=LetsSaveSomeTime
 	export NO_P4_TESTS=LetsSaveSomeTime
+	GIT_TEST_OPTS="$GIT_TEST_OPTS --immediate"
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined

index fa29a62aa3..3fa7859f0b 100644
@@ -28,6 +28,7 @@ start_test_output () {
 	github_markup_output="${GIT_TEST_TEE_OUTPUT_FILE%.out}.markup"
 	>$github_markup_output
 	GIT_TEST_TEE_OFFSET=0
+	github_markup_script_name=${0##*/}
 }
 
 # No need to override start_test_case_output

@@ -53,4 +54,13 @@ finalize_test_case_output () {
 	echo >>$github_markup_output "::endgroup::"
 }
 
+finalize_test_leak_output () {
+	echo >>$github_markup_output \
+		"::error::memory leak logged in $this_test ($github_markup_script_name)"
+
+	echo >>$github_markup_output "::group::leak: $this_test.$test_count"
+	cat "$TEST_RESULTS_SAN_FILE".* >>$github_markup_output
+	echo >>$github_markup_output "::endgroup::"
+}
+
 # No need to override finalize_test_output

index 321c2ba339..a893963f86 100644
@@ -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,6 +823,9 @@ test_failure_ () {
 	say_color error "not ok $test_count - ${pfx:+$pfx }$1"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
+	# Write the annotation before either --immediate exit path below,
+	# both of 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"

@@ -835,7 +839,6 @@ test_failure_ () {
 		check_test_results_san_file_ "$test_failure"
 		_error_exit
 	fi
-	finalize_test_case_output failure "$failure_label" "$@"
 }
 
 test_known_broken_ok_ () {

@@ -1218,6 +1221,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

gitgitgadget

From: Harald Nordgren <[email protected]>

A failing test gets an annotation in the Annotations list on its job's summary page, naming it, for example:

failed: t1060.17 partial clone of corrupted repository

with no indication of where that test lives.

Find the line a test is defined on by searching its script for the test's own description as a fixed string, using the first match, and add the file and line to the annotation's own message text:

failed: t1060.17 partial clone of corrupted repository (t1060-object-corruption.sh:141)

Fall back to naming just the script, with no line, when the description is not found verbatim, which happens when a test builds its description at runtime instead of writing it out literally.

---
 t/test-lib-github-workflow-markup.sh | 29 ++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

index 3fa7859f0b..826c4ac902 100644
@@ -31,23 +31,40 @@ start_test_output () {
 	github_markup_script_name=${0##*/}
 }
 
+find_test_case_line_ () {
+	# A description can contain characters like [ or * that would
+	# corrupt a regex search, so match it literally and take the first
+	# hit. The -- keeps a description starting with "-" from being read
+	# as an option.
+	grep -n -F -- "$1" "$TEST_DIRECTORY/$github_markup_script_name" |
+	head -n 1 | cut -d: -f1
+}
+
 # No need to override start_test_case_output
 
 finalize_test_case_output () {
 	test_case_result=$1
 	shift
+
+	case "$test_case_result" in
+	ok|broken)
+		# Exit without printing the "ok" or "broken" tests
+		return
+		;;
+	esac
+
+	test_case_line=$(find_test_case_line_ "$1")
+	test_case_where="$github_markup_script_name${test_case_line:+:$test_case_line}"
+
 	case "$test_case_result" in
 	failure)
-		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1"
+		echo >>$github_markup_output "::error::failed: $this_test.$test_count $1 ($test_case_where)"
 		;;
 	fixed)
-		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
-		;;
-	ok|broken)
-		# Exit without printing the "ok" or ""broken" tests
-		return
+		echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1 ($test_case_where)"
 		;;
 	esac
+
 	echo >>$github_markup_output "::group::$test_case_result: $this_test.$test_count $*"
 	test-tool >>$github_markup_output path-utils skip-n-bytes \
 		"$GIT_TEST_TEE_OUTPUT_FILE" $GIT_TEST_TEE_OFFSET

gitgitgadget

Hi Harald

This has been sent as v6, but the range-diff looks good and matches the changes listed here. Lets get this merged!

Thanks

Phillip

Sorry, but I somehow missed this topic. Thanks for writing and reviewing. Marked as ready for 'next'.