Re: [PATCH] test: simplify counts aggregation

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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