[v2,2/7] test-lib: parse some --options earlier
diff mbox series

Message ID 20181209225628.22216-3-szeder.dev@gmail.com
State New
Headers show
Series
  • test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
Related show

Commit Message

SZEDER Gábor Dec. 9, 2018, 10:56 p.m. UTC
'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(-)

Comments

Jeff King Dec. 11, 2018, 11:09 a.m. UTC | #1
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
SZEDER Gábor Dec. 11, 2018, 12:42 p.m. UTC | #2
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
Jeff King Dec. 17, 2018, 9:44 p.m. UTC | #3
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
SZEDER Gábor Dec. 30, 2018, 7:04 p.m. UTC | #4
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.
Jeff King Jan. 3, 2019, 4:53 a.m. UTC | #5
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

Patch
diff mbox series

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