From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 06 May 2025 13:17:09 GMT
Subject: Re: [PATCH 1/4] t: fix cases where output breaks TAP format
Message-ID: <49ef0fc1-39f1-4771-88e4-440e0924478f@gmail.com>
In-Reply-To: <20250506-pks-meson-tap-v1-1-5aaab2942a4c@pks.im>

Hi Patrick

On 06/05/2025 11:59, Patrick Steinhardt wrote:
> The TAP format does not allow arbitrary output outside of a specific
> test case. If a test suite wants to print any such diagnostic output,
> then this output has to be prefixed with "#" to mark it accordingly.
> A bunch of our tests generate output outside of `test_expect_*`
> testcases anyway without such a mark, which breaks strict TAP parsers.
> 
> Upon further inspection, all of the output generated by such tests is
> rather uninteresting. Refactor them so that we don't break the TAP
> format.

I think there is an argument that these tests are broken and we should 
be running these commands inside test_expect_success(). However this 
patch doesn't make things substantially worse because although we lose 
the output from test_create_repo that probably isn't going to matter. 
The changes to the highlighting prereq look fine too.

Best Wishes

Phillip

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>   t/t1007-hash-object.sh                       |  2 +-
>   t/t4041-diff-submodule-option.sh             |  4 ++--
>   t/t4060-diff-submodule-option-diff-format.sh |  2 +-
>   t/t7401-submodule-summary.sh                 |  4 ++--
>   t/t9500-gitweb-standalone-no-errors.sh       | 14 +++++++-------
>   5 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
> index b3cf53ff8c9..210cce56ec6 100755
> --- a/t/t1007-hash-object.sh
> +++ b/t/t1007-hash-object.sh
> @@ -30,7 +30,7 @@ setup_repo() {
>   
>   test_repo=test
>   push_repo() {
> -	test_create_repo $test_repo
> +	test_create_repo $test_repo >/dev/null
>   	cd $test_repo
>   
>   	setup_repo
> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> index 28f9d83d4c1..31f359ddf1e 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -48,7 +48,7 @@ commit_file () {
>   	git commit "$@" -m "Commit $*" >/dev/null
>   }
>   
> -test_create_repo sm1 &&
> +test_create_repo sm1 >/dev/null &&
>   add_file . foo >/dev/null
>   
>   head1=$(add_file sm1 foo1 foo2)
> @@ -236,7 +236,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' '
>   '
>   
>   rm -f sm1 &&
> -test_create_repo sm1 &&
> +test_create_repo sm1 >/dev/null &&
>   head6=$(add_file sm1 foo6 foo7)
>   fullhead6=$(cd sm1; git rev-parse --verify HEAD)
>   test_expect_success 'nonexistent commit' '
> diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
> index 76b83101d3b..17ef40c0c9f 100755
> --- a/t/t4060-diff-submodule-option-diff-format.sh
> +++ b/t/t4060-diff-submodule-option-diff-format.sh
> @@ -364,7 +364,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' '
>   '
>   
>   rm -f sm1 &&
> -test_create_repo sm1 &&
> +test_create_repo sm1 >/dev/null &&
>   head6=$(add_file sm1 foo6 foo7)
>   test_expect_success 'nonexistent commit' '
>   	git diff-index -p --submodule=diff HEAD >actual &&
> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
> index 9c3cc4cf404..80bbb1b7b5b 100755
> --- a/t/t7401-submodule-summary.sh
> +++ b/t/t7401-submodule-summary.sh
> @@ -38,7 +38,7 @@ commit_file () {
>   	git commit "$@" -m "Commit $*" >/dev/null
>   }
>   
> -test_create_repo sm1 &&
> +test_create_repo sm1 >/dev/null &&
>   add_file . foo >/dev/null
>   
>   head1=$(add_file sm1 foo1 foo2)
> @@ -215,7 +215,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' "
>   "
>   
>   rm -f sm1 &&
> -test_create_repo sm1 &&
> +test_create_repo sm1 >/dev/null &&
>   head6=$(add_file sm1 foo6 foo7)
>   test_expect_success 'nonexistent commit' "
>   	git submodule summary >actual &&
> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
> index 7679780fb87..84196a10896 100755
> --- a/t/t9500-gitweb-standalone-no-errors.sh
> +++ b/t/t9500-gitweb-standalone-no-errors.sh
> @@ -701,13 +701,13 @@ test_expect_success \
>   # syntax highlighting
>   
>   
> -highlight_version=$(highlight --version </dev/null 2>/dev/null)
> -if [ $? -eq 127 ]; then
> -	say "Skipping syntax highlighting tests: 'highlight' not found"
> -elif test -z "$highlight_version"; then
> -	say "Skipping syntax highlighting tests: incorrect 'highlight' found"
> -else
> -	test_set_prereq HIGHLIGHT
> +test_lazy_prereq HIGHLIGHT '
> +	highlight_version=$(highlight --version </dev/null 2>/dev/null) &&
> +	test -n "$highlight_version"
> +'
> +
> +if test_have_prereq HIGHLIGHT
> +then
>   	cat >>gitweb_config.perl <<-\EOF
>   	our $highlight_bin = "highlight";
>   	$feature{'highlight'}{'override'} = 1;
>