| Message ID | 20181209225628.22216-3-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests | expand |
On Sun, Dec 09, 2018 at 11:56:23PM +0100, SZEDER Gábor wrote: > 'test-lib.sh' looks for the presence of certain options like '--tee' > and '--verbose-log', so it can execute the test script again to save > its standard output and error. This happens way before the actual > option parsing loop, and the condition looking for these options looks > a bit odd, too. This patch series will add two more options to look > out for, and, in addition, will have to extract these options' stuck > arguments (i.e. '--opt=arg') as well. > > Add a proper option parsing loop to check these select options early > in 'test-lib.sh', making this early option checking more readable and > keeping those later changes in this series simpler. Use a 'for opt in > "$@"' loop to iterate over the options to preserve "$@" intact, so > options like '--verbose-log' can execute the test script again with > all the original options. > > As an alternative, we could parse all options early, but there are > options that do require an _unstuck_ argument, which is tricky to > handle properly in such a for loop, and the resulting complexity is, > in my opinion, worse than having this extra, partial option parsing > loop. In general, I'm not wild about having multiple option-parsing loops that skip the normal left-to-right parsing, since it introduces funny corner cases (like "-foo --bar" which should be the same as "--foo=--bar" instead thinking that "--bar" was passed as an option). But looking at what this is replacing: > -case "$GIT_TEST_TEE_STARTED, $* " in > -done,*) > - # do not redirect again > - ;; > -*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*) your version is easily an order of magnitude less horrible. ;) > t/test-lib.sh | 53 +++++++++++++++++++++++++++++++-------------------- > 1 file changed, 32 insertions(+), 21 deletions(-) This looks good to me overall, though... > +# Parse some options early, taking care to leave $@ intact. > +for opt > +do > + case "$opt" in > + --tee) > + tee=t ;; > + -V|--verbose-log) > + verbose_log=t ;; > + --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind) > + valgrind=memcheck ;; > + --valgrind=*) > + valgrind=${opt#--*=} ;; > + --valgrind-only=*) > + valgrind_only=${opt#--*=} ;; > + *) > + # Other options will be handled later. > + esac > +done > [...] > +elif test -n "$tee" || test -n "$verbose_log" || > + test -n "$valgrind" || test -n "$valgrind_only" Now that we've nicely moved the parsing up, would it make sense to put the annotation for "this option implies --tee" with those options? I.e., set tee=t when we see --verbose-log, which keeps all of the verbose-log logic together? > @@ -336,9 +344,12 @@ do > echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD" > fi > shift ;; > - -V|--verbose-log) > - verbose_log=t > - shift ;; > + --tee|\ > + -V|--verbose-log|\ > + --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\ > + --valgrind=*|\ > + --valgrind-only=*) > + shift ;; # These options were handled already. > *) It's too bad there's not an easy way to selectively remove from the $@ array (which would avoid duplicating this list here). The best I could come up with is: -- >8 -- first=t for i in "$@"; do test -n "$first" && set -- first= case "$i" in --foo) echo "saw foo" ;; *) set -- "$@" "$i" ;; esac done for i in "$@"; do echo "remainder: $i" done -- 8< -- but I won't be surprised if there are portability problems with assigning $@ in the middle of a loop that iterates over it. -Peff
On Tue, Dec 11, 2018 at 06:09:19AM -0500, Jeff King wrote: > On Sun, Dec 09, 2018 at 11:56:23PM +0100, SZEDER Gábor wrote: > > > 'test-lib.sh' looks for the presence of certain options like '--tee' > > and '--verbose-log', so it can execute the test script again to save > > its standard output and error. This happens way before the actual > > option parsing loop, and the condition looking for these options looks > > a bit odd, too. This patch series will add two more options to look > > out for, and, in addition, will have to extract these options' stuck > > arguments (i.e. '--opt=arg') as well. > > > > Add a proper option parsing loop to check these select options early > > in 'test-lib.sh', making this early option checking more readable and > > keeping those later changes in this series simpler. Use a 'for opt in > > "$@"' loop to iterate over the options to preserve "$@" intact, so > > options like '--verbose-log' can execute the test script again with > > all the original options. > > > > As an alternative, we could parse all options early, but there are > > options that do require an _unstuck_ argument, which is tricky to > > handle properly in such a for loop, and the resulting complexity is, > > in my opinion, worse than having this extra, partial option parsing > > loop. > > In general, I'm not wild about having multiple option-parsing loops that > skip the normal left-to-right parsing, since it introduces funny corner > cases (like "-foo --bar" which should be the same as "--foo=--bar" > instead thinking that "--bar" was passed as an option). Yeah, that's already an "issue" in the current implementation as well, though there are no such options that require options as argument. > But looking at what this is replacing: > > > -case "$GIT_TEST_TEE_STARTED, $* " in > > -done,*) > > - # do not redirect again > > - ;; > > -*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*) Anyway, I had another crack at turning the current option parsing loop into a for loop keeping $@ intact, and the results don't look all that bad this time. Note that this diff below only does the while -> for conversion, but leaves the loop where it is, so the changes are easily visible. The important bits are the conditions at the beginning of the loop and after the loop, and the handling of '-r'; the rest is mostly s/shift// and sort-of s/$1/$opt/. Thoughts? Is it better than two loops? I think it's better. diff --git a/t/test-lib.sh b/t/test-lib.sh index 9a3f7930a3..efdb6be3c8 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -264,58 +264,65 @@ test "x$TERM" != "xdumb" && ( ) && color=t -while test "$#" -ne 0 +store_arg_to= +prev_opt= +for opt do - case "$1" in + if test -n "$store_arg_to" + then + eval $store_arg_to=\$opt + store_arg_to= + prev_opt= + continue + fi + case "$opt" in -d|--d|--de|--deb|--debu|--debug) - debug=t; shift ;; + debug=t ;; -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate) - immediate=t; shift ;; + immediate=t ;; -l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests) - GIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;; + GIT_TEST_LONG=t; export GIT_TEST_LONG ;; -r) - shift; test "$#" -ne 0 || { - echo 'error: -r requires an argument' >&2; - exit 1; - } - run_list=$1; shift ;; + store_arg_to=run_list + prev_opt=-r + ;; --run=*) - run_list=${1#--*=}; shift ;; + run_list=${opt#--*=} ;; -h|--h|--he|--hel|--help) - help=t; shift ;; + help=t ;; -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose) - verbose=t; shift ;; + verbose=t ;; --verbose-only=*) - verbose_only=${1#--*=} - shift ;; + verbose_only=${opt#--*=} + ;; -q|--q|--qu|--qui|--quie|--quiet) # Ignore --quiet under a TAP::Harness. Saying how many tests # passed without the ok/not ok details is always an error. - test -z "$HARNESS_ACTIVE" && quiet=t; shift ;; + test -z "$HARNESS_ACTIVE" && quiet=t ;; --with-dashes) - with_dashes=t; shift ;; + with_dashes=t ;; --no-color) - color=; shift ;; + color= ;; --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind) valgrind=memcheck - shift ;; + ;; --valgrind=*) - valgrind=${1#--*=} - shift ;; + valgrind=${opt#--*=} + ;; --valgrind-only=*) - valgrind_only=${1#--*=} - shift ;; + valgrind_only=${opt#--*=} + ;; --tee) - shift ;; # was handled already + ;; # was handled already --root=*) - root=${1#--*=} - shift ;; + root=${opt#--*=} + ;; --chain-lint) GIT_TEST_CHAIN_LINT=1 - shift ;; + ;; --no-chain-lint) GIT_TEST_CHAIN_LINT=0 - shift ;; + ;; -x) # Some test scripts can't be reliably traced with '-x', # unless the test is run with a Bash version supporting @@ -335,15 +342,21 @@ do else echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD" fi - shift ;; + ;; -V|--verbose-log) verbose_log=t - shift ;; + ;; *) - echo "error: unknown test option '$1'" >&2; exit 1 ;; + echo "error: unknown test option '$opt'" >&2; exit 1 ;; esac done +if test -n "$store_arg_to" +then + echo "error: $prev_opt requires an argument" >&2 + exit 1 +fi + if test -n "$valgrind_only" then test -z "$valgrind" && valgrind=memcheck
On Tue, Dec 11, 2018 at 01:42:45PM +0100, SZEDER Gábor wrote: > > But looking at what this is replacing: > > > > > -case "$GIT_TEST_TEE_STARTED, $* " in > > > -done,*) > > > - # do not redirect again > > > - ;; > > > -*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*) > > > Anyway, I had another crack at turning the current option parsing loop > into a for loop keeping $@ intact, and the results don't look all that > bad this time. Note that this diff below only does the while -> for > conversion, but leaves the loop where it is, so the changes are easily > visible. The important bits are the conditions at the beginning of > the loop and after the loop, and the handling of '-r'; the rest is > mostly s/shift// and sort-of s/$1/$opt/. > > Thoughts? Is it better than two loops? I think it's better. It certainly looks better to me. It also makes sense to me to validate the options before forking/logging, though I suppose one could argue the opposite. I wonder why we didn't do it this way in the beginning (i.e., why the tee bits were all handled separately before the parsing phase). I guess just because we have to pass the options down to the sub-process. > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 9a3f7930a3..efdb6be3c8 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -264,58 +264,65 @@ test "x$TERM" != "xdumb" && ( > ) && > color=t > > -while test "$#" -ne 0 > +store_arg_to= > +prev_opt= > +for opt > do > - case "$1" in > + if test -n "$store_arg_to" > + then > + eval $store_arg_to=\$opt > + store_arg_to= > + prev_opt= > + continue > + fi OK, so this is set for the unstuck options, which then pick up the option in the next loop iteration. That's perhaps less gross than my "re-build the options with set --" trick. A simple variable set is enough for "-r". In theory we could make this: if test -n "$handle_unstuck_arg" then eval "$handle_unstuck_arg \$1" fi ... -r) handle_unstuck_arg=handle_opt_r ;; and handle_opt_r() could do whatever it wants. But I don't really foresee us adding a lot of new options (in fact, given that this is just the internal tests, I am tempted to say that we should just make it "-r<arg>" for the sake of simplicity and consistency. But maybe somebody would be annoyed. I have never used "-r" ever myself). -Peff
On Mon, Dec 17, 2018 at 04:44:36PM -0500, Jeff King wrote: > On Tue, Dec 11, 2018 at 01:42:45PM +0100, SZEDER Gábor wrote: > > diff --git a/t/test-lib.sh b/t/test-lib.sh > > index 9a3f7930a3..efdb6be3c8 100644 > > --- a/t/test-lib.sh > > +++ b/t/test-lib.sh > > @@ -264,58 +264,65 @@ test "x$TERM" != "xdumb" && ( > > ) && > > color=t > > > > -while test "$#" -ne 0 > > +store_arg_to= > > +prev_opt= > > +for opt > > do > > - case "$1" in > > + if test -n "$store_arg_to" > > + then > > + eval $store_arg_to=\$opt > > + store_arg_to= > > + prev_opt= > > + continue > > + fi > > OK, so this is set for the unstuck options, which then pick up the > option in the next loop iteration. That's perhaps less gross than my > "re-build the options with set --" trick. > > A simple variable set is enough for "-r". In theory we could make this: > > if test -n "$handle_unstuck_arg" > then > eval "$handle_unstuck_arg \$1" > fi > ... > > -r) > handle_unstuck_arg=handle_opt_r ;; > > and handle_opt_r() could do whatever it wants. But I don't really > foresee us adding a lot of new options Yeah, I would refrain from making it too general and fancy with a callback function for now, when there is only a single option that could use it. > (in fact, given that this is just > the internal tests, I am tempted to say that we should just make it > "-r<arg>" for the sake of simplicity and consistency. But maybe somebody > would be annoyed. I have never used "-r" ever myself). I didn't even know what '-r' does... And I agree that changing it to '-r<arg>' would be the best, but this patch series is about adding '--stress', so changing how '-r' gets its mandatory argument (and potentially annoying someone) is beyond the scope, I would say.
On Sun, Dec 30, 2018 at 08:04:19PM +0100, SZEDER Gábor wrote: > > (in fact, given that this is just > > the internal tests, I am tempted to say that we should just make it > > "-r<arg>" for the sake of simplicity and consistency. But maybe somebody > > would be annoyed. I have never used "-r" ever myself). > > I didn't even know what '-r' does... I had to look it up, too. :) > And I agree that changing it to '-r<arg>' would be the best, but this > patch series is about adding '--stress', so changing how '-r' gets its > mandatory argument (and potentially annoying someone) is beyond the > scope, I would say. OK, I'm fine with that (though once we've built the infrastructure to handle its unstuck form, I don't know if there's much point in changing it, so we can probably just let it live on forever). -Peff
diff --git a/t/test-lib.sh b/t/test-lib.sh index 9a3f7930a3..5577d9dc5a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -71,13 +71,33 @@ then exit 1 fi +# Parse some options early, taking care to leave $@ intact. +for opt +do + case "$opt" in + --tee) + tee=t ;; + -V|--verbose-log) + verbose_log=t ;; + --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind) + valgrind=memcheck ;; + --valgrind=*) + valgrind=${opt#--*=} ;; + --valgrind-only=*) + valgrind_only=${opt#--*=} ;; + *) + # Other options will be handled later. + esac +done + # if --tee was passed, write the output not only to the terminal, but # additionally to the file test-results/$BASENAME.out, too. -case "$GIT_TEST_TEE_STARTED, $* " in -done,*) - # do not redirect again - ;; -*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*) +if test "$GIT_TEST_TEE_STARTED" = "done" +then + : # do not redirect again +elif test -n "$tee" || test -n "$verbose_log" || + test -n "$valgrind" || test -n "$valgrind_only" +then mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results" BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)" @@ -94,8 +114,7 @@ done,*) echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE" test "$(cat "$BASE.exit")" = 0 exit - ;; -esac +fi # For repeatability, reset the environment to known value. # TERM is sanitized below, after saving color control sequences. @@ -296,17 +315,6 @@ do with_dashes=t; shift ;; --no-color) color=; shift ;; - --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind) - valgrind=memcheck - shift ;; - --valgrind=*) - valgrind=${1#--*=} - shift ;; - --valgrind-only=*) - valgrind_only=${1#--*=} - shift ;; - --tee) - shift ;; # was handled already --root=*) root=${1#--*=} shift ;; @@ -336,9 +344,12 @@ do echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD" fi shift ;; - -V|--verbose-log) - verbose_log=t - shift ;; + --tee|\ + -V|--verbose-log|\ + --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\ + --valgrind=*|\ + --valgrind-only=*) + shift ;; # These options were handled already. *) echo "error: unknown test option '$1'" >&2; exit 1 ;; esac
'test-lib.sh' looks for the presence of certain options like '--tee' and '--verbose-log', so it can execute the test script again to save its standard output and error. This happens way before the actual option parsing loop, and the condition looking for these options looks a bit odd, too. This patch series will add two more options to look out for, and, in addition, will have to extract these options' stuck arguments (i.e. '--opt=arg') as well. Add a proper option parsing loop to check these select options early in 'test-lib.sh', making this early option checking more readable and keeping those later changes in this series simpler. Use a 'for opt in "$@"' loop to iterate over the options to preserve "$@" intact, so options like '--verbose-log' can execute the test script again with all the original options. As an alternative, we could parse all options early, but there are options that do require an _unstuck_ argument, which is tricky to handle properly in such a for loop, and the resulting complexity is, in my opinion, worse than having this extra, partial option parsing loop. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- t/test-lib.sh | 53 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 21 deletions(-)