Re: [PATCH] test: simplify counts aggregation

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Mar 8, 2023, 09:56 UTC
Message-ID: <CAMP44s2NJefUOzpyq=qbWSi+X-GnVTXaEgT8dM4zMPSzm29yag@mail.gmail.com>
In-Reply-To: <230308.86bkl3zjp3.gmgdl@evledraar.gmail.com>

On Wed, Mar 8, 2023 at 3:16 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 46 quoted lines

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

Show 13 quoted lines

> 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