From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 30 Sep 2026 14:56:39 GMT
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 <haraldnordgren@gmail.com>
> 
> 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 <haraldnordgren@gmail.com>
> ---
>   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