[PATCH] test: simplify counts aggregation

[PATCH] test: simplify counts aggregation

[Felipe Contreras]

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
-- 
2.39.2

Re: [PATCH] test: simplify counts aggregation

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 08 2023, Felipe Contreras wrote:

> 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.

Re: [PATCH] test: simplify counts aggregation

[Eric Wong]

> On Wed, Mar 08 2023, Felipe Contreras wrote:
> > +++ b/t/Makefile
> > +	'$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'

> > +++ b/t/aggregate-results.sh
> >  
> > -while read file
> > +for file in $1

Reading aggregate-results.sh alone looks wrong; but your patch is
correct overall because of how it's invoked from the Makefile.
I think that's too subtle...

Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> I think we can go even further here, and just pass the
> $(TEST_RESULTS_DIRECTORY_SQ) as an argument to the script

Yes.

Re: [PATCH] test: simplify counts aggregation

[Felipe Contreras]

On Wed, Mar 8, 2023 at 3:16 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Wed, Mar 08 2023, Felipe Contreras wrote:
>
> > 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:

Right, I didn't know the "contrib" code called that.

Easy fix.

> 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.

Or just:

    for file in "${TEST_OUTPUT_DIRECTORY-.}"/test-results/t*-*.counts

And don't pass anything.

-- 
Felipe Contreras

Re: [PATCH] test: simplify counts aggregation

[Felipe Contreras]

On Wed, Mar 8, 2023 at 3:26 AM Eric Wong <e@80x24.org> wrote:
>
> > On Wed, Mar 08 2023, Felipe Contreras wrote:
> > > +++ b/t/Makefile
> > > +   '$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'
>
> > > +++ b/t/aggregate-results.sh
> > >
> > > -while read file
> > > +for file in $1
>
> Reading aggregate-results.sh alone looks wrong; but your patch is
> correct overall because of how it's invoked from the Makefile.
> I think that's too subtle...

Yeap, it's the first time I read it, I don't actually use it, and it's
broken for my use case:

    testnum=$(expr "$file" : 'test-results/\(t[0-9]*\)-')

That doesn't work when TEST_OUTPUT_DIRECTORY is set (which I always
have); should be ".*test-results".

I don't actually care how many thousands of tests have been run, the
only functionality that might be useful to me is the number of fixed
tests (namely that it's higher than 0). Maybe a summary-results.sh
might make more sense.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] test: simplify counts aggregation

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 08 2023, Felipe Contreras wrote:

> On Wed, Mar 8, 2023 at 3:16 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>>
>> On Wed, Mar 08 2023, Felipe Contreras wrote:
>>
>> > 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:
>
> Right, I didn't know the "contrib" code called that.
>
> Easy fix.
>
>> 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.
>
> Or just:
>
>     for file in "${TEST_OUTPUT_DIRECTORY-.}"/test-results/t*-*.counts
>
> And don't pass anything.

Yeah, I think that would work, but at least on an ad-hoc basis I've
sometimes saved away the "test-results" directory
(e.g. "test-results.prev").

I think it would be useful if the script part of our tooling was happy
to accept any name for such a directory, and then examined its contents.

But I don't feel strongly about it, and I don't use aggregate-results.sh
in particular (I always use "prove").

Re: [PATCH] test: simplify counts aggregation

[Felipe Contreras]

On Wed, Mar 8, 2023 at 5:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Wed, Mar 08 2023, Felipe Contreras wrote:

> > Or just:
> >
> >     for file in "${TEST_OUTPUT_DIRECTORY-.}"/test-results/t*-*.counts
> >
> > And don't pass anything.
>
> Yeah, I think that would work, but at least on an ad-hoc basis I've
> sometimes saved away the "test-results" directory
> (e.g. "test-results.prev").
>
> I think it would be useful if the script part of our tooling was happy
> to accept any name for such a directory, and then examined its contents.

That's not a problem, just pass the directory as an optional argument.

    default_dir="${TEST_OUTPUT_DIRECTORY-.}/test-results/"
    for file in "${1-$default_dir}"/t*-*.counts

> But I don't feel strongly about it, and I don't use aggregate-results.sh
> in particular (I always use "prove").

Yeah, me neither (I also use prove).

-- 
Felipe Contreras