| Message ID | 20230308090536.2562917-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | test: simplify counts aggregation | expand |
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.
> 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.
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.
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.
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").
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).
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
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(-)