# [PATCH] ci: point leak-sanitizer failures at the actual test and error 39 messages from 2026-09-25 to 2026-10-06. Participants: Harald Nordgren via GitGitGadget, Ben Knoble, Harald Nordgren, Phillip Wood, Junio C Hamano. Thread: https://gitlist.dev/t/66392 ## Harald Nordgren via GitGitGadget, 2026-09-25 18:54 Subject: [PATCH] ci: point leak-sanitizer failures at the actual test and error Message-ID: ``` From: Harald Nordgren 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 Signed-off-by: Harald Nordgren --- 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(-) diff --git a/ci/lib.sh b/ci/lib.sh index c6ccbf8c17..a89f480a78 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -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 diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index fa29a62aa3..4f6460a0ad 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 @@ -35,22 +36,54 @@ 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 echo >>$github_markup_output "::endgroup::" } +finalize_test_leak_output () { + test_leak_summary=$(head -n 40 "$TEST_RESULTS_SAN_FILE".* | + github_escape_message_) + github_annotation_ error "t/$github_markup_script_name" 1 \ + "memory leak logged around $this_test.$test_count%0A%0A$test_leak_summary" +} + # 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 +} + +github_annotation_ () { + echo >>$github_markup_output "::$1 file=$2,line=$3::$4" +} diff --git a/t/test-lib.sh b/t/test-lib.sh index 1f0505e412..a52589c6a2 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 () { @@ -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 ``` ## Ben Knoble, 2026-09-25 20:26 Subject: Re: [PATCH] ci: point leak-sanitizer failures at the actual test and error Message-ID: <09549A0E-D5FF-465C-A933-F144A19D14E4@gmail.com> In-Reply-To: ``` > Le 25 sept. 2026 à 14:59, Harald Nordgren via GitGitGadget a écrit : > > +finalize_test_leak_output () { > + test_leak_summary=$(head -n 40 "$TEST_RESULTS_SAN_FILE".* | > + github_escape_message_) > + github_annotation_ error "t/$github_markup_script_name" 1 \ > + "memory leak logged around $this_test.$test_count%0A%0A$test_leak_summary" > +} > + > # 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 > +} > + > +github_annotation_ () { > + echo >>$github_markup_output "::$1 file=$2,line=$3::$4" > +} Without commenting on the rest, introducing the helpers first might make the important patch easier to read. ``` ## Harald Nordgren, 2026-09-25 21:43 Subject: Re: [PATCH] ci: point leak-sanitizer failures at the actual test and error Message-ID: In-Reply-To: <09549A0E-D5FF-465C-A933-F144A19D14E4@gmail.com> ``` > > +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 > > +} > > + > > +github_annotation_ () { > > + echo >>$github_markup_output "::$1 file=$2,line=$3::$4" > > +} > > Without commenting on the rest, introducing the helpers first might make the important patch easier to read. Good point! Harald ``` ## Phillip Wood, 2026-09-27 15:19 Subject: Re: [PATCH] ci: point leak-sanitizer failures at the actual test and error Message-ID: In-Reply-To: ``` Hi Harald On 25/09/2026 19:54, Harald Nordgren via GitGitGadget wrote: > From: Harald Nordgren > > 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. 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? > Proof that it works: > https://github.com/git/git/actions/runs/35871180948/job/107215430244 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 > > 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(-) > > diff --git a/ci/lib.sh b/ci/lib.sh > index c6ccbf8c17..a89f480a78 100755 > --- a/ci/lib.sh > +++ b/ci/lib.sh > @@ -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 > diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh > index fa29a62aa3..4f6460a0ad 100644 > --- a/t/test-lib-github-workflow-markup.sh > +++ b/t/test-lib-github-workflow-markup.sh > @@ -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 > @@ -35,22 +36,54 @@ 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 > echo >>$github_markup_output "::endgroup::" > } > > +finalize_test_leak_output () { > + test_leak_summary=$(head -n 40 "$TEST_RESULTS_SAN_FILE".* | > + github_escape_message_) > + github_annotation_ error "t/$github_markup_script_name" 1 \ > + "memory leak logged around $this_test.$test_count%0A%0A$test_leak_summary" > +} > + > # 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 > +} > + > +github_annotation_ () { > + echo >>$github_markup_output "::$1 file=$2,line=$3::$4" > +} > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 1f0505e412..a52589c6a2 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 () { > @@ -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 ``` ## Harald Nordgren, 2026-09-27 19:52 Subject: Re: [PATCH] ci: point leak-sanitizer failures at the actual test and error Message-ID: In-Reply-To: ``` > > 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. > > 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? 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. > > Proof that it works: > > https://github.com/git/git/actions/runs/35871180948/job/107215430244 > > 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. I'll take a look at that. > 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. I'll expand the commit message. Harald ``` ## Harald Nordgren via GitGitGadget, 2026-09-28 18:54 Subject: [PATCH v2 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` 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 ## 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 @@ 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 ``` ## Harald Nordgren via GitGitGadget, 2026-09-28 18:54 Subject: [PATCH v2 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- t/test-lib-github-workflow-markup.sh | 41 ++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index 0d54496358..67c5c3461c 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 ``` ## Harald Nordgren via GitGitGadget, 2026-09-28 18:54 Subject: [PATCH v2 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure Message-ID: <46e13a6e77f0c1c23a0dfe6183e8c3dac405da89.1790621693.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- ci/lib.sh | 1 + t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++ t/test-lib.sh | 21 +++++++++++++-------- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/ci/lib.sh b/ci/lib.sh index c6ccbf8c17..a89f480a78 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -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 diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index fa29a62aa3..0d54496358 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 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 - 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 ``` ## Junio C Hamano, 2026-09-28 20:46 Subject: Re: [PATCH v2 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure Message-ID: In-Reply-To: <46e13a6e77f0c1c23a0dfe6183e8c3dac405da89.1790621693.git.gitgitgadget@gmail.com> ``` "Harald Nordgren via GitGitGadget" writes: > From: Harald Nordgren > > 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 ``` ## Junio C Hamano, 2026-09-28 20:53 Subject: Re: [PATCH v2 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: In-Reply-To: ``` "Harald Nordgren via GitGitGadget" writes: > t/test-lib-github-workflow-markup.sh | 41 ++++++++++++++++++++++------ > 1 file changed, 33 insertions(+), 8 deletions(-) > > diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh > index 0d54496358..67c5c3461c 100644 > --- a/t/test-lib-github-workflow-markup.sh > +++ b/t/test-lib-github-workflow-markup.sh > @@ -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' > +} Is it portable to feed a two-letter sequence "\r" to "sed" and expect it to be interpreted as Carriage Return? Implementations of BSD lineage "sed" don't grok it if I recall correctly. > +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 > + All of the above may make sense, but ... > 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::" What is this change about? In the original, all surrounding code has redirection early on the command line, and breaking that pattern is the only difference between the removed and added lines here as far as I can see. > } ``` ## Harald Nordgren, 2026-09-29 07:47 Subject: Re: [PATCH v2 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure Message-ID: In-Reply-To: ``` > > 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? 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. > > 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? I'll take a look at that, it might be a problem. Harald ``` ## Harald Nordgren via GitGitGadget, 2026-09-30 06:09 Subject: [PATCH v3 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` 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 ``` ## Harald Nordgren via GitGitGadget, 2026-09-30 06:09 Subject: [PATCH v3 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure Message-ID: In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- ci/lib.sh | 1 + t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++ t/test-lib.sh | 6 +++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/ci/lib.sh b/ci/lib.sh index c6ccbf8c17..a89f480a78 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -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 diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index fa29a62aa3..0d54496358 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 diff --git a/t/test-lib.sh b/t/test-lib.sh index 1f0505e412..e74a12f1dd 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,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 ``` ## Harald Nordgren via GitGitGadget, 2026-09-30 06:09 Subject: [PATCH v3 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: <750c3605128c268f331b1b9477ca0489ced75543.1790748583.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- t/test-lib-github-workflow-markup.sh | 38 +++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index 0d54496358..66d2ccca18 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 ``` ## Junio C Hamano, 2026-09-30 14:37 Subject: Re: [PATCH v3 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` "Harald Nordgren via GitGitGadget" writes: > 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. 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. ``` ## Phillip Wood, 2026-09-30 14:56 Subject: Re: [PATCH v3 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure Message-ID: <37628f91-eb36-426b-9f6b-b2083f917919@gmail.com> In-Reply-To: ``` Hi Harald On 30/09/2026 07:09, Harald Nordgren via GitGitGadget wrote: > From: Harald Nordgren > > 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. 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. > 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. 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 > 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 > --- > ci/lib.sh | 1 + > t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++ > t/test-lib.sh | 6 +++++- > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/ci/lib.sh b/ci/lib.sh > index c6ccbf8c17..a89f480a78 100755 > --- a/ci/lib.sh > +++ b/ci/lib.sh > @@ -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 > diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh > index fa29a62aa3..0d54496358 100644 > --- a/t/test-lib-github-workflow-markup.sh > +++ b/t/test-lib-github-workflow-markup.sh > @@ -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 > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 1f0505e412..e74a12f1dd 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,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 ``` ## Phillip Wood, 2026-09-30 14:56 Subject: Re: [PATCH v3 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: <5529bccf-eeb1-40f9-ae03-8fa19dc26f5a@gmail.com> In-Reply-To: <750c3605128c268f331b1b9477ca0489ced75543.1790748583.git.gitgitgadget@gmail.com> ``` Hi Harald On 30/09/2026 07:09, Harald Nordgren via GitGitGadget wrote: > From: Harald Nordgren > > 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. 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. > 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. This second sentence doesn't really add anything - you've already said we're searching for a fixed string. > 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. 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. > 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. 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 > Signed-off-by: Harald Nordgren > --- > t/test-lib-github-workflow-markup.sh | 38 +++++++++++++++++++++++----- > 1 file changed, 32 insertions(+), 6 deletions(-) > > diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh > index 0d54496358..66d2ccca18 100644 > --- a/t/test-lib-github-workflow-markup.sh > +++ b/t/test-lib-github-workflow-markup.sh > @@ -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 ``` ## Phillip Wood, 2026-09-30 15:52 Subject: Re: [PATCH v3 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` On 30/09/2026 15:37, Junio C Hamano wrote: > "Harald Nordgren via GitGitGadget" writes: > > With these updates, the patches look good to me. Unless others > spot problems I failed to see, let me mark the topic for 'next'. 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 ``` ## Harald Nordgren, 2026-09-30 18:40 Subject: Re: [PATCH v3 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: In-Reply-To: <5529bccf-eeb1-40f9-ae03-8fa19dc26f5a@gmail.com> ``` On Wed, Sep 30, 2026 at 4:56 PM Phillip Wood wrote: > > Hi Harald > > On 30/09/2026 07:09, Harald Nordgren via GitGitGadget wrote: > > From: Harald Nordgren > > > > 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. > > 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. That's poor wording on my side, I'll clarify. > 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? Yeah, that can be simplified. Harald ``` ## Junio C Hamano, 2026-09-30 18:46 Subject: Re: [PATCH v3 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` Phillip Wood writes: > On 30/09/2026 15:37, Junio C Hamano wrote: >> "Harald Nordgren via GitGitGadget" writes: >> >> With these updates, the patches look good to me. Unless others >> spot problems I failed to see, let me mark the topic for 'next'. > 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 Thanks. ``` ## Harald Nordgren via GitGitGadget, 2026-10-01 18:44 Subject: [PATCH v4 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` 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)`. Signed-off-by: Harald Nordgren @@ 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 ``` ## Harald Nordgren via GitGitGadget, 2026-10-01 18:44 Subject: [PATCH v4 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure Message-ID: <138394b48b2422a54fe6862c8701b8869f9dcb4a.1790880255.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- ci/lib.sh | 1 + t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++ t/test-lib.sh | 6 +++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/ci/lib.sh b/ci/lib.sh index c6ccbf8c17..a89f480a78 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -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 diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index fa29a62aa3..0d54496358 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 diff --git a/t/test-lib.sh b/t/test-lib.sh index 1f0505e412..e74a12f1dd 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,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 ``` ## Harald Nordgren via GitGitGadget, 2026-10-01 18:44 Subject: [PATCH v4 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: <8ec2b53d8265e1219b5f1279cadda2ac44c96ae0.1790880255.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Harald Nordgren 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)`. Signed-off-by: Harald Nordgren --- t/test-lib-github-workflow-markup.sh | 38 +++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index 0d54496358..6fee4dfb22 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 ``` ## Phillip Wood, 2026-10-01 19:49 Subject: Re: [PATCH v4 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: <0e0972b7-65a2-46ce-84a9-7e403620802a@gmail.com> In-Reply-To: <8ec2b53d8265e1219b5f1279cadda2ac44c96ae0.1790880255.git.gitgitgadget@gmail.com> ``` Hi Harald On 01/10/2026 19:44, Harald Nordgren via GitGitGadget wrote: > From: Harald Nordgren > > 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. 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? > 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)`. 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 > Signed-off-by: Harald Nordgren > --- > t/test-lib-github-workflow-markup.sh | 38 +++++++++++++++++++++++----- > 1 file changed, 32 insertions(+), 6 deletions(-) > > diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh > index 0d54496358..6fee4dfb22 100644 > --- a/t/test-lib-github-workflow-markup.sh > +++ b/t/test-lib-github-workflow-markup.sh > @@ -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 ``` ## Junio C Hamano, 2026-10-01 20:11 Subject: Re: [PATCH v4 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: In-Reply-To: <0e0972b7-65a2-46ce-84a9-7e403620802a@gmail.com> ``` Phillip Wood writes: > Hi Harald > > On 01/10/2026 19:44, Harald Nordgren via GitGitGadget wrote: >> From: Harald Nordgren >> >> 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. > > 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? > >> 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. 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. >> 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)`. > > 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 Thanks. ``` ## Harald Nordgren, 2026-10-02 08:04 Subject: Re: [PATCH v4 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: In-Reply-To: <0e0972b7-65a2-46ce-84a9-7e403620802a@gmail.com> ``` > > 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)`. > > 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. 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 ``` ## Harald Nordgren via GitGitGadget, 2026-10-03 08:11 Subject: [PATCH v5 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` 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 ## 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 ``` ## Harald Nordgren via GitGitGadget, 2026-10-03 08:11 Subject: [PATCH v5 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure Message-ID: <851efeec8b363807effe81779ff75552816fdb8f.1791015117.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- ci/lib.sh | 1 + t/test-lib-github-workflow-markup.sh | 16 ++++++++++++++++ t/test-lib.sh | 6 +++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/ci/lib.sh b/ci/lib.sh index c6ccbf8c17..a89f480a78 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -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 diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index fa29a62aa3..0d54496358 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 diff --git a/t/test-lib.sh b/t/test-lib.sh index 321c2ba339..a893963f86 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,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 ``` ## Harald Nordgren via GitGitGadget, 2026-10-03 08:11 Subject: [PATCH v5 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: <46f93a9e16e27966391a1cedb02a156b26fbd77f.1791015117.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- t/test-lib-github-workflow-markup.sh | 30 ++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index 0d54496358..ac8c536231 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 ``` ## Phillip Wood, 2026-10-03 13:03 Subject: Re: [PATCH v5 0/2] ci: link failure and leak annotations to the test script Message-ID: <3587bf44-1b3b-422f-a926-f8481104dfd8@gmail.com> In-Reply-To: ``` Hi Harald On 03/10/2026 09:11, Harald Nordgren via GitGitGadget wrote: > 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 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. > 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). 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 > 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 > > ## 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 > + > ``` ## Phillip Wood, 2026-10-03 19:02 Subject: Re: [PATCH v5 0/2] ci: link failure and leak annotations to the test script Message-ID: <8b873f2e-b395-4044-ab15-f1eab4148447@gmail.com> In-Reply-To: <3587bf44-1b3b-422f-a926-f8481104dfd8@gmail.com> ``` On 03/10/2026 14:03, Phillip Wood wrote: > 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 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 ``` ## Harald Nordgren, 2026-10-04 11:51 Subject: Re: [PATCH v5 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: <8b873f2e-b395-4044-ab15-f1eab4148447@gmail.com> ``` On Sat, Oct 3, 2026 at 9:02 PM Phillip Wood wrote: > > On 03/10/2026 14:03, Phillip Wood wrote: > > 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 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. Yes, unfortunately GitHub won't let us link to a line that was not changed in that PR. > 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. > ... > [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 Is this enough to call this a regression? Then maybe it's not worth doing this part at all. Harald ``` ## Harald Nordgren, 2026-10-04 12:03 Subject: Re: [PATCH v5 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: <3587bf44-1b3b-422f-a926-f8481104dfd8@gmail.com> ``` On Sat, Oct 3, 2026 at 3:03 PM Phillip Wood wrote: > > Hi Harald > > On 03/10/2026 09:11, Harald Nordgren via GitGitGadget wrote: > > 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 > > 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. > > > 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). > > 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. 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 ``` ## Phillip Wood, 2026-10-05 13:23 Subject: Re: [PATCH v5 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` Hi Harald On 04/10/2026 12:51, Harald Nordgren wrote: > On Sat, Oct 3, 2026 at 9:02 PM Phillip Wood wrote: >> >> If I click on the link [2] it does not take me to that test file though, >> because it was not changed. > > Yes, unfortunately GitHub won't let us link to a line that was not > changed in that PR. > >> 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. >> ... >> [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 > > Is this enough to call this a regression? Then maybe it's not worth > doing this part at all. 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 ``` ## Harald Nordgren, 2026-10-05 13:59 Subject: Re: [PATCH v5 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` > > Is this enough to call this a regression? Then maybe it's not worth > > doing this part at all. > > 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. 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 ``` ## Phillip Wood, 2026-10-05 15:11 Subject: Re: [PATCH v5 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` On 05/10/2026 14:59, Harald Nordgren wrote: >>> Is this enough to call this a regression? Then maybe it's not worth >>> doing this part at all. >> >> 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. > > 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? I guess having a bit more detail could be useful, I certainly don't object. Thanks Phillip ``` ## Harald Nordgren via GitGitGadget, 2026-10-06 06:56 Subject: [PATCH v6 0/2] ci: link failure and leak annotations to the test script Message-ID: In-Reply-To: ``` 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 @@ 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 ``` ## Harald Nordgren via GitGitGadget, 2026-10-06 06:56 Subject: [PATCH v6 1/2] ci: annotate leaks and stop a leak-sanitizer script at its first failure Message-ID: <917f373f916945b7a01b151756eba90f55a347fe.1791269798.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- ci/lib.sh | 1 + t/test-lib-github-workflow-markup.sh | 10 ++++++++++ t/test-lib.sh | 6 +++++- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/ci/lib.sh b/ci/lib.sh index c6ccbf8c17..a89f480a78 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -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 diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index fa29a62aa3..3fa7859f0b 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 diff --git a/t/test-lib.sh b/t/test-lib.sh index 321c2ba339..a893963f86 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,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 ``` ## Harald Nordgren via GitGitGadget, 2026-10-06 06:56 Subject: [PATCH v6 2/2] ci: point test failures and fixed known breakages at their file and line Message-ID: In-Reply-To: ``` From: Harald Nordgren 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. Signed-off-by: Harald Nordgren --- t/test-lib-github-workflow-markup.sh | 29 ++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh index 3fa7859f0b..826c4ac902 100644 --- a/t/test-lib-github-workflow-markup.sh +++ b/t/test-lib-github-workflow-markup.sh @@ -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 ```