{"thread":{"id":"59357","subject":"[PATCH] test: simplify counts aggregation","startedAt":"2023-03-08T09:06:30Z","lastAt":"2023-03-08T12:07:21Z","messageCount":7,"participants":["Felipe Contreras","Ævar Arnfjörð Bjarmason","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473171","messageId":"20230308090536.2562917-1-felipe.contreras@gmail.com","threadId":"59357","inReplyTo":null,"subject":"[PATCH] test: simplify counts aggregation","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-03-08T09:05:36Z","receivedAt":"2023-03-08T09:06:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"When the list of files as input was implemented in 6508eedf67\n(t/aggregate-results: accomodate systems with small max argument list\nlength, 2010-06-01), a much simpler solution wasn't considered.\n\nLet's just pass the pattern as an argument.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n t/Makefile             | 4 +---\n t/aggregate-results.sh | 2 +-\n 2 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 2c2b252240..6bc878558f 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -140,9 +140,7 @@ aggregate-results-and-cleanup: $(T)\n \t$(MAKE) clean\n \n aggregate-results:\n-\tfor f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \\\n-\t\techo \"$$f\"; \\\n-\tdone | '$(SHELL_PATH_SQ)' ./aggregate-results.sh\n+\t'$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'\n \n valgrind:\n \t$(MAKE) GIT_TEST_OPTS=\"$(GIT_TEST_OPTS) --valgrind\"\ndiff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\nindex 7f2b83bdc8..2efc2c37cd 100755\n--- a/t/aggregate-results.sh\n+++ b/t/aggregate-results.sh\n@@ -8,7 +8,7 @@ broken=0\n total=0\n missing_prereq=\n \n-while read file\n+for file in $1\n do\n \twhile read type value\n \tdo\n-- \n2.39.2\n\n"},{"id":"473182","messageId":"230308.86bkl3zjp3.gmgdl@evledraar.gmail.com","threadId":"59357","inReplyTo":"20230308090536.2562917-1-felipe.contreras@gmail.com","subject":"Re: [PATCH] test: simplify counts aggregation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-03-08T09:12:43Z","receivedAt":"2023-03-08T09:17:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 08 2023, Felipe Contreras wrote:\n\n> When the list of files as input was implemented in 6508eedf67\n> (t/aggregate-results: accomodate systems with small max argument list\n> length, 2010-06-01), a much simpler solution wasn't considered.\n>\n> Let's just pass the pattern as an argument.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  t/Makefile             | 4 +---\n>  t/aggregate-results.sh | 2 +-\n>  2 files changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/Makefile b/t/Makefile\n> index 2c2b252240..6bc878558f 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -140,9 +140,7 @@ aggregate-results-and-cleanup: $(T)\n>  \t$(MAKE) clean\n>  \n>  aggregate-results:\n> -\tfor f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \\\n> -\t\techo \"$$f\"; \\\n> -\tdone | '$(SHELL_PATH_SQ)' ./aggregate-results.sh\n> +\t'$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'\n>  \n>  valgrind:\n>  \t$(MAKE) GIT_TEST_OPTS=\"$(GIT_TEST_OPTS) --valgrind\"\n> diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\n> index 7f2b83bdc8..2efc2c37cd 100755\n> --- a/t/aggregate-results.sh\n> +++ b/t/aggregate-results.sh\n> @@ -8,7 +8,7 @@ broken=0\n>  total=0\n>  missing_prereq=\n>  \n> -while read file\n> +for file in $1\n>  do\n>  \twhile read type value\n>  \tdo\n\nThis leaves this code in contrib presumably broken:\n\t\n\tcontrib/subtree/t/Makefile-aggregate-results:\n\tcontrib/subtree/t/Makefile-     for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \\\n\tcontrib/subtree/t/Makefile-             echo \"$$f\"; \\\n\tcontrib/subtree/t/Makefile:     done | '$(SHELL_PATH_SQ)' ../../../t/aggregate-results.sh\n\nBut overall I like this direction, if we can just change that contrib\nMakefile as well to use the new mode the script excepts.\n\nI think we can go even further here, and just pass the\n$(TEST_RESULTS_DIRECTORY_SQ) as an argument to the script, then have it\ndo something like (untested):\n\n\tresults_dir=$1\n\tfor file in \"$results_dir\"/t*-*.counts\n\nWhich I think is a bit more obvious, and since the only task of the\nscript is to do exactly this, there's no reason not to have it do that\nsearch by itself.\n\n"},{"id":"473183","messageId":"20230308092621.M321240@dcvr","threadId":"59357","inReplyTo":"230308.86bkl3zjp3.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] test: simplify counts aggregation","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-03-08T09:26:21Z","receivedAt":"2023-03-08T09:26:43Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"> On Wed, Mar 08 2023, Felipe Contreras wrote:\n> > +++ b/t/Makefile\n> > +\t'$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'\n\n> > +++ b/t/aggregate-results.sh\n> >  \n> > -while read file\n> > +for file in $1\n\nReading aggregate-results.sh alone looks wrong; but your patch is\ncorrect overall because of how it's invoked from the Makefile.\nI think that's too subtle...\n\nÆvar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> I think we can go even further here, and just pass the\n> $(TEST_RESULTS_DIRECTORY_SQ) as an argument to the script\n\nYes.\n"},{"id":"473185","messageId":"CAMP44s2NJefUOzpyq=qbWSi+X-GnVTXaEgT8dM4zMPSzm29yag@mail.gmail.com","threadId":"59357","inReplyTo":"230308.86bkl3zjp3.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] test: simplify counts aggregation","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-03-08T09:56:15Z","receivedAt":"2023-03-08T09:56:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Mar 8, 2023 at 3:16 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n>\n> On Wed, Mar 08 2023, Felipe Contreras wrote:\n>\n> > When the list of files as input was implemented in 6508eedf67\n> > (t/aggregate-results: accomodate systems with small max argument list\n> > length, 2010-06-01), a much simpler solution wasn't considered.\n> >\n> > Let's just pass the pattern as an argument.\n> >\n> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> > ---\n> >  t/Makefile             | 4 +---\n> >  t/aggregate-results.sh | 2 +-\n> >  2 files changed, 2 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/t/Makefile b/t/Makefile\n> > index 2c2b252240..6bc878558f 100644\n> > --- a/t/Makefile\n> > +++ b/t/Makefile\n> > @@ -140,9 +140,7 @@ aggregate-results-and-cleanup: $(T)\n> >       $(MAKE) clean\n> >\n> >  aggregate-results:\n> > -     for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \\\n> > -             echo \"$$f\"; \\\n> > -     done | '$(SHELL_PATH_SQ)' ./aggregate-results.sh\n> > +     '$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'\n> >\n> >  valgrind:\n> >       $(MAKE) GIT_TEST_OPTS=\"$(GIT_TEST_OPTS) --valgrind\"\n> > diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\n> > index 7f2b83bdc8..2efc2c37cd 100755\n> > --- a/t/aggregate-results.sh\n> > +++ b/t/aggregate-results.sh\n> > @@ -8,7 +8,7 @@ broken=0\n> >  total=0\n> >  missing_prereq=\n> >\n> > -while read file\n> > +for file in $1\n> >  do\n> >       while read type value\n> >       do\n>\n> This leaves this code in contrib presumably broken:\n\nRight, I didn't know the \"contrib\" code called that.\n\nEasy fix.\n\n> But overall I like this direction, if we can just change that contrib\n> Makefile as well to use the new mode the script excepts.\n>\n> I think we can go even further here, and just pass the\n> $(TEST_RESULTS_DIRECTORY_SQ) as an argument to the script, then have it\n> do something like (untested):\n>\n>         results_dir=$1\n>         for file in \"$results_dir\"/t*-*.counts\n>\n> Which I think is a bit more obvious, and since the only task of the\n> script is to do exactly this, there's no reason not to have it do that\n> search by itself.\n\nOr just:\n\n    for file in \"${TEST_OUTPUT_DIRECTORY-.}\"/test-results/t*-*.counts\n\nAnd don't pass anything.\n\n-- \nFelipe Contreras\n"},{"id":"473187","messageId":"CAMP44s34bvRP1n3L57sK450ZfLUeVQKVtXDXrE75qM46xaCgcw@mail.gmail.com","threadId":"59357","inReplyTo":"20230308092621.M321240@dcvr","subject":"Re: [PATCH] test: simplify counts aggregation","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-03-08T10:04:59Z","receivedAt":"2023-03-08T10:05:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Mar 8, 2023 at 3:26 AM Eric Wong <e@80x24.org> wrote:\n>\n> > On Wed, Mar 08 2023, Felipe Contreras wrote:\n> > > +++ b/t/Makefile\n> > > +   '$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'\n>\n> > > +++ b/t/aggregate-results.sh\n> > >\n> > > -while read file\n> > > +for file in $1\n>\n> Reading aggregate-results.sh alone looks wrong; but your patch is\n> correct overall because of how it's invoked from the Makefile.\n> I think that's too subtle...\n\nYeap, it's the first time I read it, I don't actually use it, and it's\nbroken for my use case:\n\n    testnum=$(expr \"$file\" : 'test-results/\\(t[0-9]*\\)-')\n\nThat doesn't work when TEST_OUTPUT_DIRECTORY is set (which I always\nhave); should be \".*test-results\".\n\nI don't actually care how many thousands of tests have been run, the\nonly functionality that might be useful to me is the number of fixed\ntests (namely that it's higher than 0). Maybe a summary-results.sh\nmight make more sense.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"473190","messageId":"230308.86ttyvxzjz.gmgdl@evledraar.gmail.com","threadId":"59357","inReplyTo":"CAMP44s2NJefUOzpyq=qbWSi+X-GnVTXaEgT8dM4zMPSzm29yag@mail.gmail.com","subject":"Re: [PATCH] test: simplify counts aggregation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-03-08T11:15:16Z","receivedAt":"2023-03-08T11:17:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 08 2023, Felipe Contreras wrote:\n\n> On Wed, Mar 8, 2023 at 3:16 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>>\n>>\n>> On Wed, Mar 08 2023, Felipe Contreras wrote:\n>>\n>> > When the list of files as input was implemented in 6508eedf67\n>> > (t/aggregate-results: accomodate systems with small max argument list\n>> > length, 2010-06-01), a much simpler solution wasn't considered.\n>> >\n>> > Let's just pass the pattern as an argument.\n>> >\n>> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n>> > ---\n>> >  t/Makefile             | 4 +---\n>> >  t/aggregate-results.sh | 2 +-\n>> >  2 files changed, 2 insertions(+), 4 deletions(-)\n>> >\n>> > diff --git a/t/Makefile b/t/Makefile\n>> > index 2c2b252240..6bc878558f 100644\n>> > --- a/t/Makefile\n>> > +++ b/t/Makefile\n>> > @@ -140,9 +140,7 @@ aggregate-results-and-cleanup: $(T)\n>> >       $(MAKE) clean\n>> >\n>> >  aggregate-results:\n>> > -     for f in '$(TEST_RESULTS_DIRECTORY_SQ)'/t*-*.counts; do \\\n>> > -             echo \"$$f\"; \\\n>> > -     done | '$(SHELL_PATH_SQ)' ./aggregate-results.sh\n>> > +     '$(SHELL_PATH_SQ)' ./aggregate-results.sh '$(TEST_RESULTS_DIRECTORY_SQ)/t*-*.counts'\n>> >\n>> >  valgrind:\n>> >       $(MAKE) GIT_TEST_OPTS=\"$(GIT_TEST_OPTS) --valgrind\"\n>> > diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\n>> > index 7f2b83bdc8..2efc2c37cd 100755\n>> > --- a/t/aggregate-results.sh\n>> > +++ b/t/aggregate-results.sh\n>> > @@ -8,7 +8,7 @@ broken=0\n>> >  total=0\n>> >  missing_prereq=\n>> >\n>> > -while read file\n>> > +for file in $1\n>> >  do\n>> >       while read type value\n>> >       do\n>>\n>> This leaves this code in contrib presumably broken:\n>\n> Right, I didn't know the \"contrib\" code called that.\n>\n> Easy fix.\n>\n>> But overall I like this direction, if we can just change that contrib\n>> Makefile as well to use the new mode the script excepts.\n>>\n>> I think we can go even further here, and just pass the\n>> $(TEST_RESULTS_DIRECTORY_SQ) as an argument to the script, then have it\n>> do something like (untested):\n>>\n>>         results_dir=$1\n>>         for file in \"$results_dir\"/t*-*.counts\n>>\n>> Which I think is a bit more obvious, and since the only task of the\n>> script is to do exactly this, there's no reason not to have it do that\n>> search by itself.\n>\n> Or just:\n>\n>     for file in \"${TEST_OUTPUT_DIRECTORY-.}\"/test-results/t*-*.counts\n>\n> And don't pass anything.\n\nYeah, I think that would work, but at least on an ad-hoc basis I've\nsometimes saved away the \"test-results\" directory\n(e.g. \"test-results.prev\").\n\nI think it would be useful if the script part of our tooling was happy\nto accept any name for such a directory, and then examined its contents.\n\nBut I don't feel strongly about it, and I don't use aggregate-results.sh\nin particular (I always use \"prove\").\n"},{"id":"473191","messageId":"CAMP44s2dZmrt8SDFU+vwV7AX1BJNS3jR-nu0WZJsaEKLgTtKDg@mail.gmail.com","threadId":"59357","inReplyTo":"230308.86ttyvxzjz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] test: simplify counts aggregation","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-03-08T12:07:03Z","receivedAt":"2023-03-08T12:07:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Mar 8, 2023 at 5:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> On Wed, Mar 08 2023, Felipe Contreras wrote:\n\n> > Or just:\n> >\n> >     for file in \"${TEST_OUTPUT_DIRECTORY-.}\"/test-results/t*-*.counts\n> >\n> > And don't pass anything.\n>\n> Yeah, I think that would work, but at least on an ad-hoc basis I've\n> sometimes saved away the \"test-results\" directory\n> (e.g. \"test-results.prev\").\n>\n> I think it would be useful if the script part of our tooling was happy\n> to accept any name for such a directory, and then examined its contents.\n\nThat's not a problem, just pass the directory as an optional argument.\n\n    default_dir=\"${TEST_OUTPUT_DIRECTORY-.}/test-results/\"\n    for file in \"${1-$default_dir}\"/t*-*.counts\n\n> But I don't feel strongly about it, and I don't use aggregate-results.sh\n> in particular (I always use \"prove\").\n\nYeah, me neither (I also use prove).\n\n-- \nFelipe Contreras\n"}]}