Re: [PATCH] test: simplify counts aggregation

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 8, 2023, 09:12 UTC
Message-ID: <230308.86bkl3zjp3.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230308090536.2562917-1-felipe.contreras@gmail.com>

On Wed, Mar 08 2023, Felipe Contreras wrote:

Show 40 quoted lines

> When the list of files as input was implemented in 6508eedf67
> (t/aggregate-results: accomodate systems with small max argument list
> length, 2010-06-01), a much simpler solution wasn't considered.
>
> Let's just pass the pattern as an argument.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/Makefile             | 4 +---
>  t/aggregate-results.sh | 2 +-
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/t/Makefile b/t/Makefile
> index 2c2b252240..6bc878558f 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -140,9 +140,7 @@ aggregate-results-and-cleanup: $(T)
>  	$(MAKE) clean
>  
>  aggregate-results:
> -	for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \
> -		echo "$$f"; \
> -	done | '$(SHELL_PATH_SQ)' ./aggregate-results.sh
> +	'$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'
>  
>  valgrind:
>  	$(MAKE) GIT_TEST_OPTS="$(GIT_TEST_OPTS) --valgrind"
> diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh
> index 7f2b83bdc8..2efc2c37cd 100755
> --- a/t/aggregate-results.sh
> +++ b/t/aggregate-results.sh
> @@ -8,7 +8,7 @@ broken=0
>  total=0
>  missing_prereq=
>  
> -while read file
> +for file in $1
>  do
>  	while read type value
>  	do

This leaves this code in contrib presumably broken:
	
	contrib/subtree/t/Makefile-aggregate-results:
	contrib/subtree/t/Makefile-     for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \
	contrib/subtree/t/Makefile-             echo "$$f"; \
	contrib/subtree/t/Makefile:     done | '$(SHELL_PATH_SQ)' ../../../t/aggregate-results.sh

But overall I like this direction, if we can just change that contrib Makefile as well to use the new mode the script excepts.

I think we can go even further here, and just pass the $(TEST_RESULTS_DIRECTORY_SQ) as an argument to the script, then have it do something like (untested):

	results_dir=$1
	for file in "$results_dir"/t*-*.counts

Which I think is a bit more obvious, and since the only task of the script is to do exactly this, there's no reason not to have it do that search by itself.