* [PATCH v2 1/7] test-lib: translate SIGTERM and SIGHUP to an exit
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
@ 2018-12-09 22:56 ` SZEDER Gábor
2018-12-11 10:57 ` Jeff King
2018-12-09 22:56 ` [PATCH v2 2/7] test-lib: parse some --options earlier SZEDER Gábor
` (7 subsequent siblings)
8 siblings, 1 reply; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
Right now if a test script receives SIGTERM or SIGHUP (e.g., because a
test was hanging and the user 'kill'-ed it or simply closed the
terminal window the test was running in), the shell exits immediately.
This can be annoying if the test script did any global setup, like
starting apache or git-daemon, as it will not have an opportunity to
clean up after itself. A subsequent run of the test won't be able to
start its own daemon, and will either fail or skip the tests.
Instead, let's trap SIGTERM and SIGHUP as well to make sure we do a
clean shutdown, and just chain it to a normal exit (which will trigger
any cleanup).
This patch follows suit of da706545f7 (t: translate SIGINT to an exit,
2015-03-13), and even stole its commit message as well.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f1faa24b2..9a3f7930a3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -476,7 +476,7 @@ die () {
GIT_EXIT_OK=
trap 'die' EXIT
-trap 'exit $?' INT
+trap 'exit $?' INT TERM HUP
# The user-facing functions are loaded from a separate file so that
# test_perf subshells can have them too
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v2 1/7] test-lib: translate SIGTERM and SIGHUP to an exit
2018-12-09 22:56 ` [PATCH v2 1/7] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
@ 2018-12-11 10:57 ` Jeff King
0 siblings, 0 replies; 67+ messages in thread
From: Jeff King @ 2018-12-11 10:57 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano
On Sun, Dec 09, 2018 at 11:56:22PM +0100, SZEDER Gábor wrote:
> Right now if a test script receives SIGTERM or SIGHUP (e.g., because a
> test was hanging and the user 'kill'-ed it or simply closed the
> terminal window the test was running in), the shell exits immediately.
> This can be annoying if the test script did any global setup, like
> starting apache or git-daemon, as it will not have an opportunity to
> clean up after itself. A subsequent run of the test won't be able to
> start its own daemon, and will either fail or skip the tests.
>
> Instead, let's trap SIGTERM and SIGHUP as well to make sure we do a
> clean shutdown, and just chain it to a normal exit (which will trigger
> any cleanup).
>
> This patch follows suit of da706545f7 (t: translate SIGINT to an exit,
> 2015-03-13), and even stole its commit message as well.
No wonder it was so nicely explained. ;)
I think this is quite a reasonable thing to do. Since we're trying to
clean up, in theory we would like to hook any signal death, but these
three are the common ones in practice. We handle QUIT and PIPE as well
in our C code; the latter isn't an issue here. SIGQUIT is a possibility,
I guess, but seems rather unlikely.
-Peff
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v2 2/7] test-lib: parse some --options earlier
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 1/7] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
@ 2018-12-09 22:56 ` SZEDER Gábor
2018-12-11 11:09 ` Jeff King
2018-12-09 22:56 ` [PATCH v2 3/7] test-lib: consolidate naming of test-results paths SZEDER Gábor
` (6 subsequent siblings)
8 siblings, 1 reply; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
'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(-)
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
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v2 2/7] test-lib: parse some --options earlier
2018-12-09 22:56 ` [PATCH v2 2/7] test-lib: parse some --options earlier SZEDER Gábor
@ 2018-12-11 11:09 ` Jeff King
2018-12-11 12:42 ` SZEDER Gábor
0 siblings, 1 reply; 67+ messages in thread
From: Jeff King @ 2018-12-11 11:09 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH v2 2/7] test-lib: parse some --options earlier
2018-12-11 11:09 ` Jeff King
@ 2018-12-11 12:42 ` SZEDER Gábor
2018-12-17 21:44 ` Jeff King
0 siblings, 1 reply; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-11 12:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
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
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v2 2/7] test-lib: parse some --options earlier
2018-12-11 12:42 ` SZEDER Gábor
@ 2018-12-17 21:44 ` Jeff King
2018-12-30 19:04 ` SZEDER Gábor
0 siblings, 1 reply; 67+ messages in thread
From: Jeff King @ 2018-12-17 21:44 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v2 2/7] test-lib: parse some --options earlier
2018-12-17 21:44 ` Jeff King
@ 2018-12-30 19:04 ` SZEDER Gábor
2019-01-03 4:53 ` Jeff King
0 siblings, 1 reply; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
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.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v2 2/7] test-lib: parse some --options earlier
2018-12-30 19:04 ` SZEDER Gábor
@ 2019-01-03 4:53 ` Jeff King
0 siblings, 0 replies; 67+ messages in thread
From: Jeff King @ 2019-01-03 4:53 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v2 3/7] test-lib: consolidate naming of test-results paths
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 1/7] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 2/7] test-lib: parse some --options earlier SZEDER Gábor
@ 2018-12-09 22:56 ` SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 4/7] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
` (5 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
There are two places where we strip off any leading path components
and the '.sh' suffix from the test script's pathname, and there are
four places where we construct the name of the 't/test-results'
directory or the name of various test-specific files in there. The
last patch in this series will add even more.
Factor these out into helper variables to avoid repeating ourselves.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5577d9dc5a..09c77cbd1b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -90,6 +90,10 @@ do
esac
done
+TEST_NAME="$(basename "$0" .sh)"
+TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
+TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
@@ -98,12 +102,11 @@ then
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)"
+ mkdir -p "$TEST_RESULTS_DIR"
# Make this filename available to the sub-process in case it is using
# --verbose-log.
- GIT_TEST_TEE_OUTPUT_FILE=$BASE.out
+ GIT_TEST_TEE_OUTPUT_FILE=$TEST_RESULTS_BASE.out
export GIT_TEST_TEE_OUTPUT_FILE
# Truncate before calling "tee -a" to get rid of the results
@@ -111,8 +114,8 @@ then
>"$GIT_TEST_TEE_OUTPUT_FILE"
(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
- echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
- test "$(cat "$BASE.exit")" = 0
+ echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
+ test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
exit
fi
@@ -829,12 +832,9 @@ test_done () {
if test -z "$HARNESS_ACTIVE"
then
- test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
- mkdir -p "$test_results_dir"
- base=${0##*/}
- test_results_path="$test_results_dir/${base%.sh}.counts"
+ mkdir -p "$TEST_RESULTS_DIR"
- cat >"$test_results_path" <<-EOF
+ cat >"$TEST_RESULTS_BASE.counts" <<-EOF
total $test_count
success $test_success
fixed $test_fixed
@@ -1040,7 +1040,7 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$(basename "$0" .sh)"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v2 4/7] test-lib: set $TRASH_DIRECTORY earlier
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (2 preceding siblings ...)
2018-12-09 22:56 ` [PATCH v2 3/7] test-lib: consolidate naming of test-results paths SZEDER Gábor
@ 2018-12-09 22:56 ` SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 5/7] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
` (4 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
A later patch in this series will need to know the path to the trash
directory early in 'test-lib.sh', but $TRASH_DIRECTORY is set much
later. Furthermore, the path to the trash directory depends on the
'--root=<path>' option, which, too, is parsed too late.
Move parsing '--root=...' to the early option parsing loop, and set
$TRASH_DIRECTORY where the other test-specific path variables are set.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 09c77cbd1b..ea1cd34013 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -85,6 +85,8 @@ do
valgrind=${opt#--*=} ;;
--valgrind-only=*)
valgrind_only=${opt#--*=} ;;
+ --root=*)
+ root=${opt#--*=} ;;
*)
# Other options will be handled later.
esac
@@ -93,6 +95,12 @@ done
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
+test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
+case "$TRASH_DIRECTORY" in
+/*) ;; # absolute path is good
+ *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
+esac
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
@@ -318,9 +326,6 @@ do
with_dashes=t; shift ;;
--no-color)
color=; shift ;;
- --root=*)
- root=${1#--*=}
- shift ;;
--chain-lint)
GIT_TEST_CHAIN_LINT=1
shift ;;
@@ -351,7 +356,8 @@ do
-V|--verbose-log|\
--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
--valgrind=*|\
- --valgrind-only=*)
+ --valgrind-only=*|\
+ --root=*)
shift ;; # These options were handled already.
*)
echo "error: unknown test option '$1'" >&2; exit 1 ;;
@@ -1040,12 +1046,6 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
-test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
-case "$TRASH_DIRECTORY" in
-/*) ;; # absolute path is good
- *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
-esac
rm -fr "$TRASH_DIRECTORY" || {
GIT_EXIT_OK=t
echo >&5 "FATAL: Cannot prepare test area"
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v2 5/7] test-lib: extract Bash version check for '-x' tracing
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (3 preceding siblings ...)
2018-12-09 22:56 ` [PATCH v2 4/7] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
@ 2018-12-09 22:56 ` SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 6/7] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
` (3 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
Some of our test scripts can't be reliably run with '-x' tracing
enabled unless executed with a Bash version supporting BASH_XTRACEFD
(since v4.1), and we have a lengthy condition to disable tracing if
such a test script is not executed with a suitable Bash version.
Move this check out from the option parsing loop, so other options can
imply '-x' by setting 'trace=t', without missing this Bash version
check.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ea1cd34013..d55d158580 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -333,24 +333,7 @@ do
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
- # BASH_XTRACEFD (introduced in Bash v4.1). Check whether
- # this test is marked as such, and ignore '-x' if it
- # isn't executed with a suitable Bash version.
- if test -z "$test_untraceable" || {
- test -n "$BASH_VERSION" && {
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- }
- }
- then
- trace=t
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- fi
+ trace=t
shift ;;
--tee|\
-V|--verbose-log|\
@@ -373,6 +356,24 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ if test -n "$BASH_VERSION" && {
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ }
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
if test -n "$trace" && test -z "$verbose_log"
then
verbose=t
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v2 6/7] test-lib-functions: introduce the 'test_set_port' helper function
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (4 preceding siblings ...)
2018-12-09 22:56 ` [PATCH v2 5/7] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
@ 2018-12-09 22:56 ` SZEDER Gábor
2018-12-09 22:56 ` [PATCH v2 7/7] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
` (2 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
Several test scripts run daemons like 'git-daemon' or Apache, and
communicate with them through TCP sockets. To have unique ports where
these daemons are accessible, the ports are usually the number of the
corresponding test scripts, unless the user overrides them via
environment variables, and thus all those tests and test libs contain
more or less the same bit of one-liner boilerplate code to find out
the port. The last patch in this series will make this a bit more
complicated.
Factor out finding the port for a daemon into the common helper
function 'test_set_port' to avoid repeating ourselves.
Take special care of test scripts with "low" numbers:
- Test numbers below 1024 would result in a port that's only usable
as root, so set their port to '10000 + test-nr' to make sure it
doesn't interfere with other tests in the test suite. This makes
the hardcoded port number in 't0410-partial-clone.sh' unnecessary,
remove it.
- The shell's arithmetic evaluation interprets numbers with leading
zeros as octal values, which means that test number below 1000 and
containing the digits 8 or 9 will trigger an error. Remove all
leading zeros from the test numbers to prevent this.
Note that the 'git p4' tests are unlike the other tests involving
daemons in that:
- 'lib-git-p4.sh' doesn't use the test's number for unique port as
is, but does a bit of additional arithmetic on top [1].
- The port is not overridable via an environment variable.
With this patch even 'git p4' tests will use the test's number as
default port, and it will be overridable via the P4DPORT environment
variable.
[1] Commit fc00233071 (git-p4 tests: refactor and cleanup, 2011-08-22)
introduced that "unusual" unique port computation without
explaining why it was necessary (as opposed to simply using the
test number as is). It seems to be just unnecessary complication,
and in any case that commit came way before the "test nr as unique
port" got "standardized" for other daemons in commits c44132fcf3
(tests: auto-set git-daemon port, 2014-02-10), 3bb486e439 (tests:
auto-set LIB_HTTPD_PORT from test name, 2014-02-10), and
bf9d7df950 (t/lib-git-svn.sh: improve svnserve tests with parallel
make test, 2017-12-01).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +--------
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 36 ++++++++++++++++++++++++++++++++++++
7 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index f98de95c15..41eb1e3ae8 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -28,7 +28,7 @@ then
test_skip_or_die $GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
fi
-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}
+test_set_port LIB_GIT_DAEMON_PORT
GIT_DAEMON_PID=
GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index c27599474c..b3be3ba011 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -53,14 +53,7 @@ time_in_seconds () {
(cd / && "$PYTHON_PATH" -c 'import time; print(int(time.time()))')
}
-# Try to pick a unique port: guess a large number, then hope
-# no more than one of each test is running.
-#
-# This does not handle the case where somebody else is running the
-# same tests and has chosen the same ports.
-testid=${this_test#t}
-git_p4_test_start=9800
-P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
+test_set_port P4DPORT
P4PORT=localhost:$P4DPORT
P4CLIENT=client
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index a8130f9119..f3b478c307 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -13,6 +13,7 @@ fi
GIT_DIR=$PWD/.git
GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn
SVN_TREE=$GIT_SVN_DIR/svn-tree
+test_set_port SVNSERVE_PORT
svn >/dev/null 2>&1
if test $? -ne 1
@@ -119,7 +120,6 @@ require_svnserve () {
}
start_svnserve () {
- SVNSERVE_PORT=${SVNSERVE_PORT-${this_test#t}}
svnserve --listen-port $SVNSERVE_PORT \
--root "$rawsvnrepo" \
--listen-once \
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a8729f8232..e465116ef9 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -82,7 +82,7 @@ case $(uname) in
esac
LIB_HTTPD_PATH=${LIB_HTTPD_PATH-"$DEFAULT_HTTPD_PATH"}
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-${this_test#t}}
+test_set_port LIB_HTTPD_PORT
TEST_PATH="$TEST_DIRECTORY"/lib-httpd
HTTPD_ROOT_PATH="$PWD"/httpd
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..0aca8d7588 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -480,7 +480,6 @@ test_expect_success 'gc stops traversal when a missing but promised object is re
! grep "$TREE_HASH" out
'
-LIB_HTTPD_PORT=12345 # default port, 410, cannot be used as non-root
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..cd9e60632d 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -260,7 +260,7 @@ test_lazy_prereq GIT_DAEMON '
# This test spawns a daemon, so run it only if the user would be OK with
# testing with git-daemon.
test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
- JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+ test_set_port JGIT_DAEMON_PORT &&
JGIT_DAEMON_PID= &&
git init --bare empty.git &&
>empty.git/git-daemon-export-ok &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6b3bbf99e4..3746327bde 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1263,3 +1263,39 @@ test_oid () {
fi &&
eval "printf '%s' \"\${$var}\""
}
+
+# Choose a port number based on the test script's number and store it in
+# the given variable name, unless that variable already contains a number.
+test_set_port () {
+ local var=$1 port
+
+ if test $# -ne 1 || test -z "$var"
+ then
+ BUG "test_set_port requires a variable name"
+ fi
+
+ eval port=\$$var
+ case "$port" in
+ "")
+ # No port is set in the given env var, use the test
+ # number as port number instead.
+ # Remove not only the leading 't', but all leading zeros
+ # as well, so the arithmetic below won't (mis)interpret
+ # a test number like '0123' as an octal value.
+ port=${this_test#${this_test%%[1-9]*}}
+ if test "${port:-0}" -lt 1024
+ then
+ # root-only port, use a larger one instead.
+ port=$(($port + 10000))
+ fi
+
+ eval $var=$port
+ ;;
+ *[^0-9]*)
+ error >&7 "invalid port number: $port"
+ ;;
+ *)
+ # The user has specified the port.
+ ;;
+ esac
+}
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v2 7/7] test-lib: add the '--stress' option to run a test repeatedly under load
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (5 preceding siblings ...)
2018-12-09 22:56 ` [PATCH v2 6/7] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
@ 2018-12-09 22:56 ` SZEDER Gábor
2018-12-10 1:34 ` [PATCH] fixup! " SZEDER Gábor
2018-12-11 11:16 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests Jeff King
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
8 siblings, 1 reply; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
Unfortunately, we have a few flaky tests, whose failures tend to be
hard to reproduce. We've found that the best we can do to reproduce
such a failure is to run the test repeatedly while the machine is
under load, and wait in the hope that the load creates enough variance
in the timing of the test's commands that a failure is evenually
triggered. I have a command to do that, and I noticed that two other
contributors have rolled their own scripts to do the same, all
choosing slightly different approaches.
To help reproduce failures in flaky tests, introduce the '--stress'
option to run a test script repeatedly in multiple parallel jobs until
one of them fails, thereby using the test script itself to increase
the load on the machine.
The number of parallel jobs is determined by, in order of precedence:
the number specified as '--stress=<N>', or the value of the
GIT_TEST_STRESS_LOAD environment variable, or twice the number of
available processors (as reported by the 'getconf' utility), or 8.
Make '--stress' imply '--verbose -x --immediate' to get the most
information about rare failures; there is really no point in spending
all the extra effort to reproduce such a failure, and then not know
which command failed and why.
To prevent the several parallel invocations of the same test from
interfering with each other:
- Include the parallel job's number in the name of the trash
directory and the various output files under 't/test-results/' as
a '.stress-<Nr>' suffix.
- Add the parallel job's number to the port number specified by the
user or to the test number, so even tests involving daemons
listening on a TCP socket can be stressed.
- Redirect each parallel test run's output to
't/test-results/$TEST_NAME.stress-<nr>.out', because dumping the
output of several parallel running tests to the terminal would
create a big ugly mess.
For convenience, print the output of the failed test job at the end,
and rename its trash directory to end with the '.stress-failed'
suffix, so it's easy to find in a predictable path. If, in an
unlikely case, more than one jobs were to fail nearly at the same
time, then print the output of all failed jobs, and rename the trash
directory of only the last one (i.e. with the highest job number), as
it is the trash directory of the test whose output will be at the
bottom of the user's terminal.
Based on Jeff King's 'stress' script.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/README | 19 +++++++-
t/test-lib-functions.sh | 7 ++-
t/test-lib.sh | 103 ++++++++++++++++++++++++++++++++++++++--
3 files changed, 123 insertions(+), 6 deletions(-)
diff --git a/t/README b/t/README
index 28711cc508..11ce7675e3 100644
--- a/t/README
+++ b/t/README
@@ -186,6 +186,22 @@ appropriately before running "make".
this feature by setting the GIT_TEST_CHAIN_LINT environment
variable to "1" or "0", respectively.
+--stress::
+--stress=<N>::
+ Run the test script repeatedly in multiple parallel jobs until
+ one of them fails. Useful for reproducing rare failures in
+ flaky tests. The number of parallel jobs is, in order of
+ precedence: <N>, or the value of the GIT_TEST_STRESS_LOAD
+ environment variable, or twice the number of available
+ processors (as shown by the 'getconf' utility), or 8.
+ Implies `--verbose -x --immediate` to get the most information
+ about the failure. Note that the verbose output of each test
+ job is saved to 't/test-results/$TEST_NAME.stress-<nr>.out',
+ and only the output of the failed test job is shown on the
+ terminal. The names of the trash directories get a
+ '.stress-<nr>' suffix, and the trash directory of the failed
+ test job is renamed to end with a '.stress-failed' suffix.
+
You can also set the GIT_TEST_INSTALLED environment variable to
the bindir of an existing git installation to test that installation.
You still need to have built this git sandbox, from which various
@@ -425,7 +441,8 @@ This test harness library does the following things:
- Creates an empty test directory with an empty .git/objects database
and chdir(2) into it. This directory is 't/trash
directory.$test_name_without_dotsh', with t/ subject to change by
- the --root option documented above.
+ the --root option documented above, and a '.stress-<N>' suffix
+ appended by the --stress option.
- Defines standard test helper functions for your scripts to
use. These functions are designed to make all scripts behave
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3746327bde..ee3435c6df 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1288,8 +1288,6 @@ test_set_port () {
# root-only port, use a larger one instead.
port=$(($port + 10000))
fi
-
- eval $var=$port
;;
*[^0-9]*)
error >&7 "invalid port number: $port"
@@ -1298,4 +1296,9 @@ test_set_port () {
# The user has specified the port.
;;
esac
+
+ # Make sure that parallel '--stress' test jobs get different
+ # ports.
+ port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
+ eval $var=$port
}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d55d158580..e405191164 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -87,21 +87,110 @@ do
valgrind_only=${opt#--*=} ;;
--root=*)
root=${opt#--*=} ;;
+ --stress)
+ stress=t ;;
+ --stress=*)
+ stress=${opt#--*=} ;;
*)
# Other options will be handled later.
esac
done
+TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
-TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
+TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX"
+TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
*) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
esac
+# If --stress was passed, run this test repeatedly in several parallel loops.
+if test "$GIT_TEST_STRESS_STARTED" = "done"
+then
+ : # Don't stress test again.
+elif test -n "$stress"
+then
+ if test "$stress" != t
+ then
+ job_count=$stress
+ elif test -n "$GIT_TEST_STRESS_LOAD"
+ then
+ job_count="$GIT_TEST_STRESS_LOAD"
+ elif job_count=$(getconf _NPROCESSORS_ONLN 2>/dev/null) &&
+ test -n "$job_count"
+ then
+ job_count=$((2 * $job_count))
+ else
+ job_count=8
+ fi
+
+ mkdir -p "$TEST_RESULTS_DIR"
+ stressfail="$TEST_RESULTS_BASE.stress-failed"
+ rm -f "$stressfail"
+
+ stress_exit=0
+ trap '
+ kill $job_pids 2>/dev/null
+ wait
+ stress_exit=1
+ ' TERM INT HUP
+
+ job_pids=
+ job_nr=0
+ while test $job_nr -lt "$job_count"
+ do
+ (
+ GIT_TEST_STRESS_STARTED=done
+ GIT_TEST_STRESS_JOB_NR=$job_nr
+ export GIT_TEST_STRESS_STARTED GIT_TEST_STRESS_JOB_NR
+
+ trap '
+ kill $test_pid 2>/dev/null
+ wait
+ exit 1
+ ' TERM INT
+
+ cnt=0
+ while ! test -e "$stressfail"
+ do
+ $TEST_SHELL_PATH "$0" "$@" >"$TEST_RESULTS_BASE.stress-$job_nr.out" 2>&1 &
+ test_pid=$!
+
+ if wait $test_pid
+ then
+ printf "OK %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
+ else
+ echo $GIT_TEST_STRESS_JOB_NR >>"$stressfail"
+ printf "FAIL %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
+ fi
+ cnt=$(($cnt + 1))
+ done
+ ) &
+ job_pids="$job_pids $!"
+ job_nr=$(($job_nr + 1))
+ done
+
+ wait
+
+ if test -f "$stressfail"
+ then
+ echo "Log(s) of failed test run(s):"
+ for failed_job_nr in $(sort -n "$stressfail")
+ do
+ echo "Contents of '$TEST_RESULTS_BASE.stress-$failed_job_nr.out':"
+ cat "$TEST_RESULTS_BASE.stress-$failed_job_nr.out"
+ done
+ rm -rf "$TRASH_DIRECTORY.stress-failed"
+ # Move the last one.
+ mv "$TRASH_DIRECTORY.stress-$failed_job_nr" "$TRASH_DIRECTORY.stress-failed"
+ fi
+
+ exit $stress_exit
+fi
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
@@ -340,7 +429,8 @@ do
--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
--valgrind=*|\
--valgrind-only=*|\
- --root=*)
+ --root=*|\
+ --stress|--stress=*)
shift ;; # These options were handled already.
*)
echo "error: unknown test option '$1'" >&2; exit 1 ;;
@@ -379,6 +469,13 @@ then
verbose=t
fi
+if test -n "$stress"
+then
+ verbose=t
+ trace=t
+ immediate=t
+fi
+
if test -n "$color"
then
# Save the color control sequences now rather than run tput
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH] fixup! test-lib: add the '--stress' option to run a test repeatedly under load
2018-12-09 22:56 ` [PATCH v2 7/7] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
@ 2018-12-10 1:34 ` SZEDER Gábor
0 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-10 1:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
---
Erm. 'trace=t' must be set before checking whether the shell
supports BASH_XTRACEFD.
t/test-lib.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e405191164..3e9916b39b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -446,6 +446,13 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$stress"
+then
+ verbose=t
+ trace=t
+ immediate=t
+fi
+
if test -n "$trace" && test -n "$test_untraceable"
then
# '-x' tracing requested, but this test script can't be reliably
@@ -469,13 +476,6 @@ then
verbose=t
fi
-if test -n "$stress"
-then
- verbose=t
- trace=t
- immediate=t
-fi
-
if test -n "$color"
then
# Save the color control sequences now rather than run tput
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (6 preceding siblings ...)
2018-12-09 22:56 ` [PATCH v2 7/7] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
@ 2018-12-11 11:16 ` Jeff King
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
8 siblings, 0 replies; 67+ messages in thread
From: Jeff King @ 2018-12-11 11:16 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano
On Sun, Dec 09, 2018 at 11:56:21PM +0100, SZEDER Gábor wrote:
> This patch series tries to make reproducing rare failures in flaky
> tests easier: it adds the '--stress' option to our test library to run
> the test script repeatedly in multiple parallel jobs, in the hope that
> the increased load creates enough variance in the timing of the test's
> commands that such a failure is eventually triggered.
>
> Notable changes since v1:
>
> - Made it more Peff-friendly :), namely it will now show the log of
> the failed test and will rename its trash directory.
>
> Furthermore, '--stress' will now imply '--verbose -x --immediate'.
:) Thanks. I do sympathize with the notion of keeping orthogonal things
orthogonal, but I hope that this will make the tool a little more
pleasant to use out of the box. We can always tweak the behavior later,
too. It's not like this is something user-facing that we've promised as
a scripting interface.
> - Improved abort handling based on the discussion of the previous
> version. (As a result, the patch is so heavily modified, that
> 'range-diff' with default parameters consideres it a different
> patch; increasing the creation factor then results in one big ugly
> diff of a diff, so I left it as-is.)
Yeah, this all looked good to me.
> - Added a few new patches, mostly preparatory refactorings, though
> the first one might be considered a bugfix.
I left a few minor comments, but these all looked good to me.
-Peff
^ permalink raw reply [flat|nested] 67+ messages in thread* [PATCH v3 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
2018-12-09 22:56 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (7 preceding siblings ...)
2018-12-11 11:16 ` [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests Jeff King
@ 2018-12-30 19:16 ` SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 1/8] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
` (8 more replies)
8 siblings, 9 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
To recap: this patch series tries to make reproducing rare failures in
flaky tests easier: it adds the '--stress' option to our test library
to run the test script repeatedly in multiple parallel jobs, in the
hope that the increased load creates enough variance in the timing of
the test's commands that such a failure is eventually triggered.
Changes since v2:
- Most importantly, parse all options before handling '--tee',
'--verbose-log' or '--stress'. This constitutes the bulk of the
range diff.
- Add a bit of sanity check for the argument of '--stress=123'.
- Minor commit message updates.
v2 was quickly followed up with a fixup! patch; the range-diff below
includes that fixup! already squashed in.
Previous versions:
v2: https://public-inbox.org/git/20181209225628.22216-1-szeder.dev@gmail.com/T/#u
v1: https://public-inbox.org/git/20181204163457.15717-1-szeder.dev@gmail.com/T/
SZEDER Gábor (8):
test-lib: translate SIGTERM and SIGHUP to an exit
test-lib: parse options in a for loop to keep $@ intact
test-lib: parse command line options earlier
test-lib: consolidate naming of test-results paths
test-lib: set $TRASH_DIRECTORY earlier
test-lib: extract Bash version check for '-x' tracing
test-lib-functions: introduce the 'test_set_port' helper function
test-lib: add the '--stress' option to run a test repeatedly under
load
t/README | 19 ++-
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +-
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 39 +++++
t/test-lib.sh | 360 ++++++++++++++++++++++++++-------------
9 files changed, 303 insertions(+), 133 deletions(-)
Range-diff:
1: 3a5c926167 = 1: 48a2b19218 test-lib: translate SIGTERM and SIGHUP to an exit
2: 8eee8d7fba < -: ---------- test-lib: parse some --options earlier
-: ---------- > 2: adc5e8a86e test-lib: parse options in a for loop to keep $@ intact
-: ---------- > 3: 89748074db test-lib: parse command line options earlier
3: dd20ae5e9a ! 4: 5a6d17f54a test-lib: consolidate naming of test-results paths
@@ -16,8 +16,8 @@
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
- esac
- done
+ verbose=t
+ fi
+TEST_NAME="$(basename "$0" .sh)"
+TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
@@ -27,8 +27,8 @@
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
@@
- elif test -n "$tee" || test -n "$verbose_log" ||
- test -n "$valgrind" || test -n "$valgrind_only"
+ : # do not redirect again
+ elif test -n "$tee"
then
- mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
- BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
4: f6e56c91c4 ! 5: cd7626b782 test-lib: set $TRASH_DIRECTORY earlier
@@ -15,15 +15,6 @@
diff --git a/t/test-lib.sh b/t/test-lib.sh
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
-@@
- valgrind=${opt#--*=} ;;
- --valgrind-only=*)
- valgrind_only=${opt#--*=} ;;
-+ --root=*)
-+ root=${opt#--*=} ;;
- *)
- # Other options will be handled later.
- esac
@@
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
@@ -37,26 +28,6 @@
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
-@@
- with_dashes=t; shift ;;
- --no-color)
- color=; shift ;;
-- --root=*)
-- root=${1#--*=}
-- shift ;;
- --chain-lint)
- GIT_TEST_CHAIN_LINT=1
- shift ;;
-@@
- -V|--verbose-log|\
- --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
- --valgrind=*|\
-- --valgrind-only=*)
-+ --valgrind-only=*|\
-+ --root=*)
- shift ;; # These options were handled already.
- *)
- echo "error: unknown test option '$1'" >&2; exit 1 ;;
@@
fi
5: 99ecd2902e ! 6: 90296ac776 test-lib: extract Bash version check for '-x' tracing
@@ -2,23 +2,30 @@
test-lib: extract Bash version check for '-x' tracing
- Some of our test scripts can't be reliably run with '-x' tracing
- enabled unless executed with a Bash version supporting BASH_XTRACEFD
- (since v4.1), and we have a lengthy condition to disable tracing if
- such a test script is not executed with a suitable Bash version.
+ One of our test scripts, 't1510-repo-setup.sh' [1], still can't be
+ reliably run with '-x' tracing enabled, unless it's executed with a
+ Bash version supporting BASH_XTRACEFD (since v4.1). We have a lengthy
+ condition to check the version of the shell running the test script,
+ and disable tracing if it's not executed with a suitable Bash version
+ [2].
Move this check out from the option parsing loop, so other options can
imply '-x' by setting 'trace=t', without missing this Bash version
check.
+ [1] 5827506928 (t1510-repo-setup: mark as untraceable with '-x',
+ 2018-02-24)
+ [2] 5fc98e79fc (t: add means to disable '-x' tracing for individual
+ test scripts, 2018-02-24)
+
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
diff --git a/t/test-lib.sh b/t/test-lib.sh
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
- GIT_TEST_CHAIN_LINT=0
- shift ;;
+ --no-chain-lint)
+ GIT_TEST_CHAIN_LINT=0 ;;
-x)
- # Some test scripts can't be reliably traced with '-x',
- # unless the test is run with a Bash version supporting
@@ -38,10 +45,11 @@
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- fi
-+ trace=t
- shift ;;
- --tee|\
- -V|--verbose-log|\
+- ;;
++ trace=t ;;
+ -V|--verbose-log)
+ verbose_log=t
+ tee=t
@@
test -z "$verbose_log" && verbose=t
fi
6: dcf7d2a397 ! 7: a4a3e7cefa test-lib-functions: introduce the 'test_set_port' helper function
@@ -178,7 +178,7 @@
+
+ eval $var=$port
+ ;;
-+ *[^0-9]*)
++ *[^0-9]*|0*)
+ error >&7 "invalid port number: $port"
+ ;;
+ *)
7: f27f2fc1d0 ! 8: b0dcff0d3f test-lib: add the '--stress' option to run a test repeatedly under load
@@ -4,12 +4,12 @@
Unfortunately, we have a few flaky tests, whose failures tend to be
hard to reproduce. We've found that the best we can do to reproduce
- such a failure is to run the test repeatedly while the machine is
- under load, and wait in the hope that the load creates enough variance
- in the timing of the test's commands that a failure is evenually
- triggered. I have a command to do that, and I noticed that two other
- contributors have rolled their own scripts to do the same, all
- choosing slightly different approaches.
+ such a failure is to run the test script repeatedly while the machine
+ is under load, and wait in the hope that the load creates enough
+ variance in the timing of the test's commands that a failure is
+ evenually triggered. I have a command to do that, and I noticed that
+ two other contributors have rolled their own scripts to do the same,
+ all choosing slightly different approaches.
To help reproduce failures in flaky tests, introduce the '--stress'
option to run a test script repeatedly in multiple parallel jobs until
@@ -37,19 +37,21 @@
user or to the test number, so even tests involving daemons
listening on a TCP socket can be stressed.
- - Redirect each parallel test run's output to
+ - Redirect each parallel test run's verbose output to
't/test-results/$TEST_NAME.stress-<nr>.out', because dumping the
output of several parallel running tests to the terminal would
create a big ugly mess.
For convenience, print the output of the failed test job at the end,
and rename its trash directory to end with the '.stress-failed'
- suffix, so it's easy to find in a predictable path. If, in an
- unlikely case, more than one jobs were to fail nearly at the same
- time, then print the output of all failed jobs, and rename the trash
- directory of only the last one (i.e. with the highest job number), as
- it is the trash directory of the test whose output will be at the
- bottom of the user's terminal.
+ suffix, so it's easy to find in a predictable path (OTOH, all absolute
+ paths recorded in the trash directory become invalid; we'll see
+ whether this causes any issues in practice). If, in an unlikely case,
+ more than one jobs were to fail nearly at the same time, then print
+ the output of all failed jobs, and rename the trash directory of only
+ the last one (i.e. with the highest job number), as it is the trash
+ directory of the test whose output will be at the bottom of the user's
+ terminal.
Based on Jeff King's 'stress' script.
@@ -102,7 +104,7 @@
-
- eval $var=$port
;;
- *[^0-9]*)
+ *[^0-9]*|0*)
error >&7 "invalid port number: $port"
@@
# The user has specified the port.
@@ -119,17 +121,42 @@
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
- valgrind_only=${opt#--*=} ;;
- --root=*)
- root=${opt#--*=} ;;
+ verbose_log=t
+ tee=t
+ ;;
+ --stress)
+ stress=t ;;
+ --stress=*)
-+ stress=${opt#--*=} ;;
++ stress=${opt#--*=}
++ case "$stress" in
++ *[^0-9]*|0*|"")
++ echo "error: --stress=<N> requires the number of jobs to run" >&2
++ exit 1
++ ;;
++ *) # Good.
++ ;;
++ esac
++ ;;
*)
- # Other options will be handled later.
+ echo "error: unknown test option '$opt'" >&2; exit 1 ;;
esac
- done
+@@
+ test -z "$verbose_log" && verbose=t
+ fi
+
++if test -n "$stress"
++then
++ verbose=t
++ trace=t
++ immediate=t
++fi
++
+ if test -n "$trace" && test -n "$test_untraceable"
+ then
+ # '-x' tracing requested, but this test script can't be reliably
+@@
+ verbose=t
+ fi
+TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
TEST_NAME="$(basename "$0" .sh)"
@@ -231,27 +258,3 @@
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
-@@
- --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
- --valgrind=*|\
- --valgrind-only=*|\
-- --root=*)
-+ --root=*|\
-+ --stress|--stress=*)
- shift ;; # These options were handled already.
- *)
- echo "error: unknown test option '$1'" >&2; exit 1 ;;
-@@
- verbose=t
- fi
-
-+if test -n "$stress"
-+then
-+ verbose=t
-+ trace=t
-+ immediate=t
-+fi
-+
- if test -n "$color"
- then
- # Save the color control sequences now rather than run tput
--
2.20.1.151.gec613c4b75
^ permalink raw reply [flat|nested] 67+ messages in thread* [PATCH v3 1/8] test-lib: translate SIGTERM and SIGHUP to an exit
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
@ 2018-12-30 19:16 ` SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 2/8] test-lib: parse options in a for loop to keep $@ intact SZEDER Gábor
` (7 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
Right now if a test script receives SIGTERM or SIGHUP (e.g., because a
test was hanging and the user 'kill'-ed it or simply closed the
terminal window the test was running in), the shell exits immediately.
This can be annoying if the test script did any global setup, like
starting apache or git-daemon, as it will not have an opportunity to
clean up after itself. A subsequent run of the test won't be able to
start its own daemon, and will either fail or skip the tests.
Instead, let's trap SIGTERM and SIGHUP as well to make sure we do a
clean shutdown, and just chain it to a normal exit (which will trigger
any cleanup).
This patch follows suit of da706545f7 (t: translate SIGINT to an exit,
2015-03-13), and even stole its commit message as well.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f1faa24b2..9a3f7930a3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -476,7 +476,7 @@ die () {
GIT_EXIT_OK=
trap 'die' EXIT
-trap 'exit $?' INT
+trap 'exit $?' INT TERM HUP
# The user-facing functions are loaded from a separate file so that
# test_perf subshells can have them too
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 2/8] test-lib: parse options in a for loop to keep $@ intact
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 1/8] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
@ 2018-12-30 19:16 ` SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 3/8] test-lib: parse command line options earlier SZEDER Gábor
` (6 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
'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, and to do so it needs the original
command line options the test was invoked with.
The next patch is about to move the option parsing loop earlier in
'test-lib.sh', but it is implemented using 'shift' in a while loop,
effecively destroying "$@" by the end of the option parsing. Not
good.
As a preparatory step, turn that option parsing loop into a 'for opt
in "$@"' loop to preserve "$@" intact while iterating over the
options, and taking extra care to handle the '-r' option's required
argument (or the lack thereof).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 77 ++++++++++++++++++++++++++++-----------------------
1 file changed, 42 insertions(+), 35 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9a3f7930a3..ed7267962f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -264,58 +264,59 @@ 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
+ ;;
--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=memcheck ;;
--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 ;;
+ GIT_TEST_CHAIN_LINT=1 ;;
--no-chain-lint)
- GIT_TEST_CHAIN_LINT=0
- shift ;;
+ GIT_TEST_CHAIN_LINT=0 ;;
-x)
# Some test scripts can't be reliably traced with '-x',
# unless the test is run with a Bash version supporting
@@ -335,14 +336,20 @@ do
else
echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
fi
- shift ;;
+ ;;
-V|--verbose-log)
- verbose_log=t
- shift ;;
+ verbose_log=t ;;
*)
- echo "error: unknown test option '$1'" >&2; exit 1 ;;
+ echo "error: unknown test option '$opt'" >&2; exit 1 ;;
esac
+
+ prev_opt=$opt
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
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 3/8] test-lib: parse command line options earlier
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 1/8] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 2/8] test-lib: parse options in a for loop to keep $@ intact SZEDER Gábor
@ 2018-12-30 19:16 ` SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 4/8] test-lib: consolidate naming of test-results paths SZEDER Gábor
` (5 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
'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. It looks for '--valgrind' as well, to
set up some Valgrind-specific stuff. These all happen before the
actual option parsing loop, and the conditions looking for these
options look a bit odd, too. They are not completely correct, either,
because in a bogus invocation like './t1234-foo.sh -r --tee' they
recognize '--tee', although it should be handled as the required
argument of the '-r' option. This patch series will add two more
options to look out for early, and, in addition, will have to extract
these options' stuck arguments (i.e. '--opt=arg') as well.
So let's move the option parsing loop and the couple of related
conditions following it earlier in 'test-lib.sh', before the place
where the test script is executed again for '--tee' and its friends.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 228 ++++++++++++++++++++++++++------------------------
1 file changed, 119 insertions(+), 109 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ed7267962f..fcc04afdff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -71,13 +71,125 @@ then
exit 1
fi
+# Parse options while taking care to leave $@ intact, so we will still
+# have all the original command line options when executing the test
+# script again for '--tee' and '--verbose-log' below.
+store_arg_to=
+prev_opt=
+for opt
+do
+ 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 ;;
+ -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
+ 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 ;;
+ -r)
+ store_arg_to=run_list
+ ;;
+ --run=*)
+ run_list=${opt#--*=} ;;
+ -h|--h|--he|--hel|--help)
+ help=t ;;
+ -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
+ verbose=t ;;
+ --verbose-only=*)
+ 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 ;;
+ --with-dashes)
+ with_dashes=t ;;
+ --no-color)
+ color= ;;
+ --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
+ valgrind=memcheck
+ tee=t
+ ;;
+ --valgrind=*)
+ valgrind=${opt#--*=}
+ tee=t
+ ;;
+ --valgrind-only=*)
+ valgrind_only=${opt#--*=}
+ tee=t
+ ;;
+ --tee)
+ tee=t ;;
+ --root=*)
+ root=${opt#--*=} ;;
+ --chain-lint)
+ GIT_TEST_CHAIN_LINT=1 ;;
+ --no-chain-lint)
+ GIT_TEST_CHAIN_LINT=0 ;;
+ -x)
+ # Some test scripts can't be reliably traced with '-x',
+ # unless the test is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1). Check whether
+ # this test is marked as such, and ignore '-x' if it
+ # isn't executed with a suitable Bash version.
+ if test -z "$test_untraceable" || {
+ test -n "$BASH_VERSION" && {
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ }
+ }
+ then
+ trace=t
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ fi
+ ;;
+ -V|--verbose-log)
+ verbose_log=t
+ tee=t
+ ;;
+ *)
+ echo "error: unknown test option '$opt'" >&2; exit 1 ;;
+ esac
+
+ prev_opt=$opt
+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
+ test -z "$verbose" && verbose_only="$valgrind_only"
+elif test -n "$valgrind"
+then
+ test -z "$verbose_log" && verbose=t
+fi
+
+if test -n "$trace" && test -z "$verbose_log"
+then
+ verbose=t
+fi
+
# 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"
+then
mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
@@ -94,8 +206,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.
@@ -193,7 +304,7 @@ fi
# Add libc MALLOC and MALLOC_PERTURB test
# only if we are not executing the test with valgrind
-if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
+if test -n "$valgrind" ||
test -n "$TEST_NO_MALLOC_CHECK"
then
setup_malloc_check () {
@@ -264,107 +375,6 @@ test "x$TERM" != "xdumb" && (
) &&
color=t
-store_arg_to=
-prev_opt=
-for opt
-do
- 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 ;;
- -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
- 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 ;;
- -r)
- store_arg_to=run_list
- ;;
- --run=*)
- run_list=${opt#--*=} ;;
- -h|--h|--he|--hel|--help)
- help=t ;;
- -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
- verbose=t ;;
- --verbose-only=*)
- 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 ;;
- --with-dashes)
- with_dashes=t ;;
- --no-color)
- color= ;;
- --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
- valgrind=memcheck ;;
- --valgrind=*)
- valgrind=${opt#--*=} ;;
- --valgrind-only=*)
- valgrind_only=${opt#--*=} ;;
- --tee)
- ;; # was handled already
- --root=*)
- root=${opt#--*=} ;;
- --chain-lint)
- GIT_TEST_CHAIN_LINT=1 ;;
- --no-chain-lint)
- GIT_TEST_CHAIN_LINT=0 ;;
- -x)
- # Some test scripts can't be reliably traced with '-x',
- # unless the test is run with a Bash version supporting
- # BASH_XTRACEFD (introduced in Bash v4.1). Check whether
- # this test is marked as such, and ignore '-x' if it
- # isn't executed with a suitable Bash version.
- if test -z "$test_untraceable" || {
- test -n "$BASH_VERSION" && {
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- }
- }
- then
- trace=t
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- fi
- ;;
- -V|--verbose-log)
- verbose_log=t ;;
- *)
- echo "error: unknown test option '$opt'" >&2; exit 1 ;;
- esac
-
- prev_opt=$opt
-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
- test -z "$verbose" && verbose_only="$valgrind_only"
-elif test -n "$valgrind"
-then
- test -z "$verbose_log" && verbose=t
-fi
-
-if test -n "$trace" && test -z "$verbose_log"
-then
- verbose=t
-fi
-
if test -n "$color"
then
# Save the color control sequences now rather than run tput
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 4/8] test-lib: consolidate naming of test-results paths
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
` (2 preceding siblings ...)
2018-12-30 19:16 ` [PATCH v3 3/8] test-lib: parse command line options earlier SZEDER Gábor
@ 2018-12-30 19:16 ` SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 5/8] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
` (4 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
There are two places where we strip off any leading path components
and the '.sh' suffix from the test script's pathname, and there are
four places where we construct the name of the 't/test-results'
directory or the name of various test-specific files in there. The
last patch in this series will add even more.
Factor these out into helper variables to avoid repeating ourselves.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index fcc04afdff..41457d1dcf 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -183,6 +183,10 @@ then
verbose=t
fi
+TEST_NAME="$(basename "$0" .sh)"
+TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
+TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
@@ -190,12 +194,11 @@ then
: # do not redirect again
elif test -n "$tee"
then
- mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
- BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
+ mkdir -p "$TEST_RESULTS_DIR"
# Make this filename available to the sub-process in case it is using
# --verbose-log.
- GIT_TEST_TEE_OUTPUT_FILE=$BASE.out
+ GIT_TEST_TEE_OUTPUT_FILE=$TEST_RESULTS_BASE.out
export GIT_TEST_TEE_OUTPUT_FILE
# Truncate before calling "tee -a" to get rid of the results
@@ -203,8 +206,8 @@ then
>"$GIT_TEST_TEE_OUTPUT_FILE"
(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
- echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
- test "$(cat "$BASE.exit")" = 0
+ echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
+ test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
exit
fi
@@ -835,12 +838,9 @@ test_done () {
if test -z "$HARNESS_ACTIVE"
then
- test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
- mkdir -p "$test_results_dir"
- base=${0##*/}
- test_results_path="$test_results_dir/${base%.sh}.counts"
+ mkdir -p "$TEST_RESULTS_DIR"
- cat >"$test_results_path" <<-EOF
+ cat >"$TEST_RESULTS_BASE.counts" <<-EOF
total $test_count
success $test_success
fixed $test_fixed
@@ -1046,7 +1046,7 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$(basename "$0" .sh)"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 5/8] test-lib: set $TRASH_DIRECTORY earlier
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
` (3 preceding siblings ...)
2018-12-30 19:16 ` [PATCH v3 4/8] test-lib: consolidate naming of test-results paths SZEDER Gábor
@ 2018-12-30 19:16 ` SZEDER Gábor
2018-12-30 22:44 ` SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 6/8] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
` (3 subsequent siblings)
8 siblings, 1 reply; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
A later patch in this series will need to know the path to the trash
directory early in 'test-lib.sh', but $TRASH_DIRECTORY is set much
later. Furthermore, the path to the trash directory depends on the
'--root=<path>' option, which, too, is parsed too late.
Move parsing '--root=...' to the early option parsing loop, and set
$TRASH_DIRECTORY where the other test-specific path variables are set.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 41457d1dcf..2b88ba2de1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -186,6 +186,12 @@ fi
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
+test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
+case "$TRASH_DIRECTORY" in
+/*) ;; # absolute path is good
+ *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
+esac
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
@@ -1046,12 +1052,6 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
-test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
-case "$TRASH_DIRECTORY" in
-/*) ;; # absolute path is good
- *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
-esac
rm -fr "$TRASH_DIRECTORY" || {
GIT_EXIT_OK=t
echo >&5 "FATAL: Cannot prepare test area"
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v3 5/8] test-lib: set $TRASH_DIRECTORY earlier
2018-12-30 19:16 ` [PATCH v3 5/8] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
@ 2018-12-30 22:44 ` SZEDER Gábor
2018-12-30 22:48 ` [PATCH v3.1 " SZEDER Gábor
0 siblings, 1 reply; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 22:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
On Sun, Dec 30, 2018 at 08:16:26PM +0100, SZEDER Gábor wrote:
> A later patch in this series will need to know the path to the trash
> directory early in 'test-lib.sh', but $TRASH_DIRECTORY is set much
> later. Furthermore, the path to the trash directory depends on the
> '--root=<path>' option, which, too, is parsed too late.
>
> Move parsing '--root=...' to the early option parsing loop, and set
> $TRASH_DIRECTORY where the other test-specific path variables are set.
Sigh.
After moving the whole option parsing loop, I should have updated this
commit message as well.
Hang on for a sec...
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> t/test-lib.sh | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 41457d1dcf..2b88ba2de1 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -186,6 +186,12 @@ fi
> TEST_NAME="$(basename "$0" .sh)"
> TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
> TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
> +TRASH_DIRECTORY="trash directory.$TEST_NAME"
> +test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
> +case "$TRASH_DIRECTORY" in
> +/*) ;; # absolute path is good
> + *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
> +esac
>
> # if --tee was passed, write the output not only to the terminal, but
> # additionally to the file test-results/$BASENAME.out, too.
> @@ -1046,12 +1052,6 @@ then
> fi
>
> # Test repository
> -TRASH_DIRECTORY="trash directory.$TEST_NAME"
> -test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
> -case "$TRASH_DIRECTORY" in
> -/*) ;; # absolute path is good
> - *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
> -esac
> rm -fr "$TRASH_DIRECTORY" || {
> GIT_EXIT_OK=t
> echo >&5 "FATAL: Cannot prepare test area"
> --
> 2.20.1.151.gec613c4b75
>
^ permalink raw reply [flat|nested] 67+ messages in thread* [PATCH v3.1 5/8] test-lib: set $TRASH_DIRECTORY earlier
2018-12-30 22:44 ` SZEDER Gábor
@ 2018-12-30 22:48 ` SZEDER Gábor
0 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 22:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
A later patch in this series will need to know the path to the trash
directory early in 'test-lib.sh', but $TRASH_DIRECTORY is set much
later.
Set $TRASH_DIRECTORY earlier, where the other test-specific path
variables are set.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
Updated the commit message, the diff is the same.
t/test-lib.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 41457d1dcf..2b88ba2de1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -186,6 +186,12 @@ fi
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
+test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
+case "$TRASH_DIRECTORY" in
+/*) ;; # absolute path is good
+ *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
+esac
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
@@ -1046,12 +1052,6 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
-test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
-case "$TRASH_DIRECTORY" in
-/*) ;; # absolute path is good
- *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
-esac
rm -fr "$TRASH_DIRECTORY" || {
GIT_EXIT_OK=t
echo >&5 "FATAL: Cannot prepare test area"
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [PATCH v3 6/8] test-lib: extract Bash version check for '-x' tracing
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
` (4 preceding siblings ...)
2018-12-30 19:16 ` [PATCH v3 5/8] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
@ 2018-12-30 19:16 ` SZEDER Gábor
2018-12-31 17:14 ` Carlo Arenas
2018-12-30 19:16 ` [PATCH v3 7/8] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
` (2 subsequent siblings)
8 siblings, 1 reply; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
One of our test scripts, 't1510-repo-setup.sh' [1], still can't be
reliably run with '-x' tracing enabled, unless it's executed with a
Bash version supporting BASH_XTRACEFD (since v4.1). We have a lengthy
condition to check the version of the shell running the test script,
and disable tracing if it's not executed with a suitable Bash version
[2].
Move this check out from the option parsing loop, so other options can
imply '-x' by setting 'trace=t', without missing this Bash version
check.
[1] 5827506928 (t1510-repo-setup: mark as untraceable with '-x',
2018-02-24)
[2] 5fc98e79fc (t: add means to disable '-x' tracing for individual
test scripts, 2018-02-24)
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2b88ba2de1..7d77a26d44 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -134,25 +134,7 @@ do
--no-chain-lint)
GIT_TEST_CHAIN_LINT=0 ;;
-x)
- # Some test scripts can't be reliably traced with '-x',
- # unless the test is run with a Bash version supporting
- # BASH_XTRACEFD (introduced in Bash v4.1). Check whether
- # this test is marked as such, and ignore '-x' if it
- # isn't executed with a suitable Bash version.
- if test -z "$test_untraceable" || {
- test -n "$BASH_VERSION" && {
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- }
- }
- then
- trace=t
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- fi
- ;;
+ trace=t ;;
-V|--verbose-log)
verbose_log=t
tee=t
@@ -178,6 +160,24 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ if test -n "$BASH_VERSION" && {
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ }
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
if test -n "$trace" && test -z "$verbose_log"
then
verbose=t
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 7/8] test-lib-functions: introduce the 'test_set_port' helper function
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
` (5 preceding siblings ...)
2018-12-30 19:16 ` [PATCH v3 6/8] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
@ 2018-12-30 19:16 ` SZEDER Gábor
2018-12-30 19:16 ` [PATCH v3 8/8] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
Several test scripts run daemons like 'git-daemon' or Apache, and
communicate with them through TCP sockets. To have unique ports where
these daemons are accessible, the ports are usually the number of the
corresponding test scripts, unless the user overrides them via
environment variables, and thus all those tests and test libs contain
more or less the same bit of one-liner boilerplate code to find out
the port. The last patch in this series will make this a bit more
complicated.
Factor out finding the port for a daemon into the common helper
function 'test_set_port' to avoid repeating ourselves.
Take special care of test scripts with "low" numbers:
- Test numbers below 1024 would result in a port that's only usable
as root, so set their port to '10000 + test-nr' to make sure it
doesn't interfere with other tests in the test suite. This makes
the hardcoded port number in 't0410-partial-clone.sh' unnecessary,
remove it.
- The shell's arithmetic evaluation interprets numbers with leading
zeros as octal values, which means that test number below 1000 and
containing the digits 8 or 9 will trigger an error. Remove all
leading zeros from the test numbers to prevent this.
Note that the 'git p4' tests are unlike the other tests involving
daemons in that:
- 'lib-git-p4.sh' doesn't use the test's number for unique port as
is, but does a bit of additional arithmetic on top [1].
- The port is not overridable via an environment variable.
With this patch even 'git p4' tests will use the test's number as
default port, and it will be overridable via the P4DPORT environment
variable.
[1] Commit fc00233071 (git-p4 tests: refactor and cleanup, 2011-08-22)
introduced that "unusual" unique port computation without
explaining why it was necessary (as opposed to simply using the
test number as is). It seems to be just unnecessary complication,
and in any case that commit came way before the "test nr as unique
port" got "standardized" for other daemons in commits c44132fcf3
(tests: auto-set git-daemon port, 2014-02-10), 3bb486e439 (tests:
auto-set LIB_HTTPD_PORT from test name, 2014-02-10), and
bf9d7df950 (t/lib-git-svn.sh: improve svnserve tests with parallel
make test, 2017-12-01).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +--------
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 36 ++++++++++++++++++++++++++++++++++++
7 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index f98de95c15..41eb1e3ae8 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -28,7 +28,7 @@ then
test_skip_or_die $GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
fi
-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}
+test_set_port LIB_GIT_DAEMON_PORT
GIT_DAEMON_PID=
GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index c27599474c..b3be3ba011 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -53,14 +53,7 @@ time_in_seconds () {
(cd / && "$PYTHON_PATH" -c 'import time; print(int(time.time()))')
}
-# Try to pick a unique port: guess a large number, then hope
-# no more than one of each test is running.
-#
-# This does not handle the case where somebody else is running the
-# same tests and has chosen the same ports.
-testid=${this_test#t}
-git_p4_test_start=9800
-P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
+test_set_port P4DPORT
P4PORT=localhost:$P4DPORT
P4CLIENT=client
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index a8130f9119..f3b478c307 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -13,6 +13,7 @@ fi
GIT_DIR=$PWD/.git
GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn
SVN_TREE=$GIT_SVN_DIR/svn-tree
+test_set_port SVNSERVE_PORT
svn >/dev/null 2>&1
if test $? -ne 1
@@ -119,7 +120,6 @@ require_svnserve () {
}
start_svnserve () {
- SVNSERVE_PORT=${SVNSERVE_PORT-${this_test#t}}
svnserve --listen-port $SVNSERVE_PORT \
--root "$rawsvnrepo" \
--listen-once \
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a8729f8232..e465116ef9 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -82,7 +82,7 @@ case $(uname) in
esac
LIB_HTTPD_PATH=${LIB_HTTPD_PATH-"$DEFAULT_HTTPD_PATH"}
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-${this_test#t}}
+test_set_port LIB_HTTPD_PORT
TEST_PATH="$TEST_DIRECTORY"/lib-httpd
HTTPD_ROOT_PATH="$PWD"/httpd
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..0aca8d7588 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -480,7 +480,6 @@ test_expect_success 'gc stops traversal when a missing but promised object is re
! grep "$TREE_HASH" out
'
-LIB_HTTPD_PORT=12345 # default port, 410, cannot be used as non-root
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..cd9e60632d 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -260,7 +260,7 @@ test_lazy_prereq GIT_DAEMON '
# This test spawns a daemon, so run it only if the user would be OK with
# testing with git-daemon.
test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
- JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+ test_set_port JGIT_DAEMON_PORT &&
JGIT_DAEMON_PID= &&
git init --bare empty.git &&
>empty.git/git-daemon-export-ok &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6b3bbf99e4..4459bdda13 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1263,3 +1263,39 @@ test_oid () {
fi &&
eval "printf '%s' \"\${$var}\""
}
+
+# Choose a port number based on the test script's number and store it in
+# the given variable name, unless that variable already contains a number.
+test_set_port () {
+ local var=$1 port
+
+ if test $# -ne 1 || test -z "$var"
+ then
+ BUG "test_set_port requires a variable name"
+ fi
+
+ eval port=\$$var
+ case "$port" in
+ "")
+ # No port is set in the given env var, use the test
+ # number as port number instead.
+ # Remove not only the leading 't', but all leading zeros
+ # as well, so the arithmetic below won't (mis)interpret
+ # a test number like '0123' as an octal value.
+ port=${this_test#${this_test%%[1-9]*}}
+ if test "${port:-0}" -lt 1024
+ then
+ # root-only port, use a larger one instead.
+ port=$(($port + 10000))
+ fi
+
+ eval $var=$port
+ ;;
+ *[^0-9]*|0*)
+ error >&7 "invalid port number: $port"
+ ;;
+ *)
+ # The user has specified the port.
+ ;;
+ esac
+}
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 8/8] test-lib: add the '--stress' option to run a test repeatedly under load
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
` (6 preceding siblings ...)
2018-12-30 19:16 ` [PATCH v3 7/8] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
@ 2018-12-30 19:16 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2018-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
Unfortunately, we have a few flaky tests, whose failures tend to be
hard to reproduce. We've found that the best we can do to reproduce
such a failure is to run the test script repeatedly while the machine
is under load, and wait in the hope that the load creates enough
variance in the timing of the test's commands that a failure is
evenually triggered. I have a command to do that, and I noticed that
two other contributors have rolled their own scripts to do the same,
all choosing slightly different approaches.
To help reproduce failures in flaky tests, introduce the '--stress'
option to run a test script repeatedly in multiple parallel jobs until
one of them fails, thereby using the test script itself to increase
the load on the machine.
The number of parallel jobs is determined by, in order of precedence:
the number specified as '--stress=<N>', or the value of the
GIT_TEST_STRESS_LOAD environment variable, or twice the number of
available processors (as reported by the 'getconf' utility), or 8.
Make '--stress' imply '--verbose -x --immediate' to get the most
information about rare failures; there is really no point in spending
all the extra effort to reproduce such a failure, and then not know
which command failed and why.
To prevent the several parallel invocations of the same test from
interfering with each other:
- Include the parallel job's number in the name of the trash
directory and the various output files under 't/test-results/' as
a '.stress-<Nr>' suffix.
- Add the parallel job's number to the port number specified by the
user or to the test number, so even tests involving daemons
listening on a TCP socket can be stressed.
- Redirect each parallel test run's verbose output to
't/test-results/$TEST_NAME.stress-<nr>.out', because dumping the
output of several parallel running tests to the terminal would
create a big ugly mess.
For convenience, print the output of the failed test job at the end,
and rename its trash directory to end with the '.stress-failed'
suffix, so it's easy to find in a predictable path (OTOH, all absolute
paths recorded in the trash directory become invalid; we'll see
whether this causes any issues in practice). If, in an unlikely case,
more than one jobs were to fail nearly at the same time, then print
the output of all failed jobs, and rename the trash directory of only
the last one (i.e. with the highest job number), as it is the trash
directory of the test whose output will be at the bottom of the user's
terminal.
Based on Jeff King's 'stress' script.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/README | 19 ++++++-
t/test-lib-functions.sh | 7 ++-
t/test-lib.sh | 109 +++++++++++++++++++++++++++++++++++++++-
3 files changed, 130 insertions(+), 5 deletions(-)
diff --git a/t/README b/t/README
index 28711cc508..11ce7675e3 100644
--- a/t/README
+++ b/t/README
@@ -186,6 +186,22 @@ appropriately before running "make".
this feature by setting the GIT_TEST_CHAIN_LINT environment
variable to "1" or "0", respectively.
+--stress::
+--stress=<N>::
+ Run the test script repeatedly in multiple parallel jobs until
+ one of them fails. Useful for reproducing rare failures in
+ flaky tests. The number of parallel jobs is, in order of
+ precedence: <N>, or the value of the GIT_TEST_STRESS_LOAD
+ environment variable, or twice the number of available
+ processors (as shown by the 'getconf' utility), or 8.
+ Implies `--verbose -x --immediate` to get the most information
+ about the failure. Note that the verbose output of each test
+ job is saved to 't/test-results/$TEST_NAME.stress-<nr>.out',
+ and only the output of the failed test job is shown on the
+ terminal. The names of the trash directories get a
+ '.stress-<nr>' suffix, and the trash directory of the failed
+ test job is renamed to end with a '.stress-failed' suffix.
+
You can also set the GIT_TEST_INSTALLED environment variable to
the bindir of an existing git installation to test that installation.
You still need to have built this git sandbox, from which various
@@ -425,7 +441,8 @@ This test harness library does the following things:
- Creates an empty test directory with an empty .git/objects database
and chdir(2) into it. This directory is 't/trash
directory.$test_name_without_dotsh', with t/ subject to change by
- the --root option documented above.
+ the --root option documented above, and a '.stress-<N>' suffix
+ appended by the --stress option.
- Defines standard test helper functions for your scripts to
use. These functions are designed to make all scripts behave
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 4459bdda13..92cf8f812c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1288,8 +1288,6 @@ test_set_port () {
# root-only port, use a larger one instead.
port=$(($port + 10000))
fi
-
- eval $var=$port
;;
*[^0-9]*|0*)
error >&7 "invalid port number: $port"
@@ -1298,4 +1296,9 @@ test_set_port () {
# The user has specified the port.
;;
esac
+
+ # Make sure that parallel '--stress' test jobs get different
+ # ports.
+ port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
+ eval $var=$port
}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7d77a26d44..16f74bd524 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -139,6 +139,19 @@ do
verbose_log=t
tee=t
;;
+ --stress)
+ stress=t ;;
+ --stress=*)
+ stress=${opt#--*=}
+ case "$stress" in
+ *[^0-9]*|0*|"")
+ echo "error: --stress=<N> requires the number of jobs to run" >&2
+ exit 1
+ ;;
+ *) # Good.
+ ;;
+ esac
+ ;;
*)
echo "error: unknown test option '$opt'" >&2; exit 1 ;;
esac
@@ -160,6 +173,13 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$stress"
+then
+ verbose=t
+ trace=t
+ immediate=t
+fi
+
if test -n "$trace" && test -n "$test_untraceable"
then
# '-x' tracing requested, but this test script can't be reliably
@@ -183,16 +203,101 @@ then
verbose=t
fi
+TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
-TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
+TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX"
+TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
*) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
esac
+# If --stress was passed, run this test repeatedly in several parallel loops.
+if test "$GIT_TEST_STRESS_STARTED" = "done"
+then
+ : # Don't stress test again.
+elif test -n "$stress"
+then
+ if test "$stress" != t
+ then
+ job_count=$stress
+ elif test -n "$GIT_TEST_STRESS_LOAD"
+ then
+ job_count="$GIT_TEST_STRESS_LOAD"
+ elif job_count=$(getconf _NPROCESSORS_ONLN 2>/dev/null) &&
+ test -n "$job_count"
+ then
+ job_count=$((2 * $job_count))
+ else
+ job_count=8
+ fi
+
+ mkdir -p "$TEST_RESULTS_DIR"
+ stressfail="$TEST_RESULTS_BASE.stress-failed"
+ rm -f "$stressfail"
+
+ stress_exit=0
+ trap '
+ kill $job_pids 2>/dev/null
+ wait
+ stress_exit=1
+ ' TERM INT HUP
+
+ job_pids=
+ job_nr=0
+ while test $job_nr -lt "$job_count"
+ do
+ (
+ GIT_TEST_STRESS_STARTED=done
+ GIT_TEST_STRESS_JOB_NR=$job_nr
+ export GIT_TEST_STRESS_STARTED GIT_TEST_STRESS_JOB_NR
+
+ trap '
+ kill $test_pid 2>/dev/null
+ wait
+ exit 1
+ ' TERM INT
+
+ cnt=0
+ while ! test -e "$stressfail"
+ do
+ $TEST_SHELL_PATH "$0" "$@" >"$TEST_RESULTS_BASE.stress-$job_nr.out" 2>&1 &
+ test_pid=$!
+
+ if wait $test_pid
+ then
+ printf "OK %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
+ else
+ echo $GIT_TEST_STRESS_JOB_NR >>"$stressfail"
+ printf "FAIL %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
+ fi
+ cnt=$(($cnt + 1))
+ done
+ ) &
+ job_pids="$job_pids $!"
+ job_nr=$(($job_nr + 1))
+ done
+
+ wait
+
+ if test -f "$stressfail"
+ then
+ echo "Log(s) of failed test run(s):"
+ for failed_job_nr in $(sort -n "$stressfail")
+ do
+ echo "Contents of '$TEST_RESULTS_BASE.stress-$failed_job_nr.out':"
+ cat "$TEST_RESULTS_BASE.stress-$failed_job_nr.out"
+ done
+ rm -rf "$TRASH_DIRECTORY.stress-failed"
+ # Move the last one.
+ mv "$TRASH_DIRECTORY.stress-$failed_job_nr" "$TRASH_DIRECTORY.stress-failed"
+ fi
+
+ exit $stress_exit
+fi
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
2018-12-30 19:16 ` [PATCH v3 0/8] " SZEDER Gábor
` (7 preceding siblings ...)
2018-12-30 19:16 ` [PATCH v3 8/8] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 1/8] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
` (8 more replies)
8 siblings, 9 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
To recap: this patch series tries to make reproducing rare failures in
flaky tests easier: it adds the '--stress' option to our test library
to run the test script repeatedly in multiple parallel jobs, in the
hope that the increased load creates enough variance in the timing of
the test's commands that such a failure is eventually triggered.
There is an issue with v3: by parsing all command line options before
re-executing the test script for '--tee' or '--verbose-log', we not
only parse the options twice (which is harmless), but check the
shell's version for untraceable test scripts twice as well.
Consequently, the warning about ignoring '-x' are issued twice:
$ ./t1510-repo-setup.sh -x --tee
warning: ignoring -x; './t1510-repo-setup.sh' is untraceable without BASH_XTRACEFD
warning: ignoring -x; './t1510-repo-setup.sh' is untraceable without BASH_XTRACEFD
ok 1 - #0: nonbare repo, no explicit configuration
<...>
Furthermore, when TEST_SHELL_PATH is not /bin/sh but is set to Bash
v4.1 or later, then the above command issues one warning as it's run
with /bin/sh from the shebang line, but then re-execs itself with Bash
and does print the '-x' tracing output as well.
This version of the patch series fixes the above issue by checking the
shell's version after '--tee' and '--verbose-log' re-execue the test
script. To do so it makes more sense to move the 'test-lib: extract
Bash version check for '-x' tracing' patch to the beginning of the
series. This move, however, made the range-diff quite noisy, hence
the interdiff below, as it describes the changes since v3 much better.
It's based on 'sg/test-bash-version-fix'.
SZEDER Gábor (8):
test-lib: translate SIGTERM and SIGHUP to an exit
test-lib: extract Bash version check for '-x' tracing
test-lib: parse options in a for loop to keep $@ intact
test-lib: parse command line options earlier
test-lib: consolidate naming of test-results paths
test-lib: set $TRASH_DIRECTORY earlier
test-lib-functions: introduce the 'test_set_port' helper function
test-lib: add the '--stress' option to run a test repeatedly under
load
t/README | 19 +-
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +-
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 39 +++++
t/test-lib.sh | 365 ++++++++++++++++++++++++++-------------
9 files changed, 308 insertions(+), 133 deletions(-)
Interdiff:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3f0b0c07df..a1abb1177a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -180,29 +180,6 @@ then
immediate=t
fi
-if test -n "$trace" && test -n "$test_untraceable"
-then
- # '-x' tracing requested, but this test script can't be reliably
- # traced, unless it is run with a Bash version supporting
- # BASH_XTRACEFD (introduced in Bash v4.1).
- if test -n "$BASH_VERSION" && eval '
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- '
- then
- : Executed by a Bash version supporting BASH_XTRACEFD. Good.
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- trace=
- fi
-fi
-if test -n "$trace" && test -z "$verbose_log"
-then
- verbose=t
-fi
-
TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
@@ -322,6 +299,34 @@ then
exit
fi
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ #
+ # Perform this version check _after_ the test script was
+ # potentially re-executed with $TEST_SHELL_PATH for '--tee' or
+ # '--verbose-log', so the right shell is checked and the
+ # warning is issued only once.
+ if test -n "$BASH_VERSION" && eval '
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ '
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
+if test -n "$trace" && test -z "$verbose_log"
+then
+ verbose=t
+fi
+
# For repeatability, reset the environment to known value.
# TERM is sanitized below, after saving color control sequences.
LANG=C
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 1/8] test-lib: translate SIGTERM and SIGHUP to an exit
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 2/8] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
` (7 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
Right now if a test script receives SIGTERM or SIGHUP (e.g., because a
test was hanging and the user 'kill'-ed it or simply closed the
terminal window the test was running in), the shell exits immediately.
This can be annoying if the test script did any global setup, like
starting apache or git-daemon, as it will not have an opportunity to
clean up after itself. A subsequent run of the test won't be able to
start its own daemon, and will either fail or skip the tests.
Instead, let's trap SIGTERM and SIGHUP as well to make sure we do a
clean shutdown, and just chain it to a normal exit (which will trigger
any cleanup).
This patch follows suit of da706545f7 (t: translate SIGINT to an exit,
2015-03-13), and even stole its commit message as well.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c34831a4de..4c3744cce4 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -476,7 +476,7 @@ die () {
GIT_EXIT_OK=
trap 'die' EXIT
-trap 'exit $?' INT
+trap 'exit $?' INT TERM HUP
# The user-facing functions are loaded from a separate file so that
# test_perf subshells can have them too
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 2/8] test-lib: extract Bash version check for '-x' tracing
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 1/8] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 3/8] test-lib: parse options in a for loop to keep $@ intact SZEDER Gábor
` (6 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
One of our test scripts, 't1510-repo-setup.sh' [1], still can't be
reliably run with '-x' tracing enabled, unless it's executed with a
Bash version supporting BASH_XTRACEFD (since v4.1). We have a lengthy
condition to check the version of the shell running the test script,
and disable tracing if it's not executed with a suitable Bash version
[2].
Move this check out from the option parsing loop, so other options can
imply '-x' by setting 'trace=t', without missing this Bash version
check.
[1] 5827506928 (t1510-repo-setup: mark as untraceable with '-x',
2018-02-24)
[2] 5fc98e79fc (t: add means to disable '-x' tracing for individual
test scripts, 2018-02-24)
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4c3744cce4..1f02e2e25b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -317,24 +317,7 @@ do
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
- # BASH_XTRACEFD (introduced in Bash v4.1). Check whether
- # this test is marked as such, and ignore '-x' if it
- # isn't executed with a suitable Bash version.
- if test -z "$test_untraceable" || {
- test -n "$BASH_VERSION" && eval '
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- '
- }
- then
- trace=t
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- fi
+ trace=t
shift ;;
-V|--verbose-log)
verbose_log=t
@@ -353,6 +336,24 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ if test -n "$BASH_VERSION" && eval '
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ '
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
if test -n "$trace" && test -z "$verbose_log"
then
verbose=t
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 3/8] test-lib: parse options in a for loop to keep $@ intact
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 1/8] test-lib: translate SIGTERM and SIGHUP to an exit SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 2/8] test-lib: extract Bash version check for '-x' tracing SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 4/8] test-lib: parse command line options earlier SZEDER Gábor
` (5 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
'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, and to do so it needs the original
command line options the test was invoked with.
The next patch is about to move the option parsing loop earlier in
'test-lib.sh', but it is implemented using 'shift' in a while loop,
effecively destroying "$@" by the end of the option parsing. Not
good.
As a preparatory step, turn that option parsing loop into a 'for opt
in "$@"' loop to preserve "$@" intact while iterating over the
options, and taking extra care to handle the '-r' option's required
argument (or the lack thereof).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 78 +++++++++++++++++++++++++++------------------------
1 file changed, 42 insertions(+), 36 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1f02e2e25b..3cf59a92f0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -264,68 +264,74 @@ 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
+ ;;
--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=memcheck ;;
--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 ;;
+ GIT_TEST_CHAIN_LINT=1 ;;
--no-chain-lint)
- GIT_TEST_CHAIN_LINT=0
- shift ;;
+ GIT_TEST_CHAIN_LINT=0 ;;
-x)
- trace=t
- shift ;;
+ trace=t ;;
-V|--verbose-log)
- verbose_log=t
- shift ;;
+ verbose_log=t ;;
*)
- echo "error: unknown test option '$1'" >&2; exit 1 ;;
+ echo "error: unknown test option '$opt'" >&2; exit 1 ;;
esac
+
+ prev_opt=$opt
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
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 4/8] test-lib: parse command line options earlier
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (2 preceding siblings ...)
2019-01-05 1:08 ` [PATCH v4 3/8] test-lib: parse options in a for loop to keep $@ intact SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 5/8] test-lib: consolidate naming of test-results paths SZEDER Gábor
` (4 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
'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. It looks for '--valgrind' as well, to
set up some Valgrind-specific stuff. These all happen before the
actual option parsing loop, and the conditions looking for these
options look a bit odd, too. They are not completely correct, either,
because in a bogus invocation like './t1234-foo.sh -r --tee' they
recognize '--tee', although it should be handled as the required
argument of the '-r' option. This patch series will add two more
options to look out for early, and, in addition, will have to extract
these options' stuck arguments (i.e. '--opt=arg') as well.
So let's move the option parsing loop and the couple of related
conditions following it earlier in 'test-lib.sh', before the place
where the test script is executed again for '--tee' and its friends.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 233 +++++++++++++++++++++++++++-----------------------
1 file changed, 124 insertions(+), 109 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3cf59a92f0..14ccb60838 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -71,13 +71,102 @@ then
exit 1
fi
+# Parse options while taking care to leave $@ intact, so we will still
+# have all the original command line options when executing the test
+# script again for '--tee' and '--verbose-log' below.
+store_arg_to=
+prev_opt=
+for opt
+do
+ 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 ;;
+ -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
+ 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 ;;
+ -r)
+ store_arg_to=run_list
+ ;;
+ --run=*)
+ run_list=${opt#--*=} ;;
+ -h|--h|--he|--hel|--help)
+ help=t ;;
+ -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
+ verbose=t ;;
+ --verbose-only=*)
+ 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 ;;
+ --with-dashes)
+ with_dashes=t ;;
+ --no-color)
+ color= ;;
+ --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
+ valgrind=memcheck
+ tee=t
+ ;;
+ --valgrind=*)
+ valgrind=${opt#--*=}
+ tee=t
+ ;;
+ --valgrind-only=*)
+ valgrind_only=${opt#--*=}
+ tee=t
+ ;;
+ --tee)
+ tee=t ;;
+ --root=*)
+ root=${opt#--*=} ;;
+ --chain-lint)
+ GIT_TEST_CHAIN_LINT=1 ;;
+ --no-chain-lint)
+ GIT_TEST_CHAIN_LINT=0 ;;
+ -x)
+ trace=t ;;
+ -V|--verbose-log)
+ verbose_log=t
+ tee=t
+ ;;
+ *)
+ echo "error: unknown test option '$opt'" >&2; exit 1 ;;
+ esac
+
+ prev_opt=$opt
+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
+ test -z "$verbose" && verbose_only="$valgrind_only"
+elif test -n "$valgrind"
+then
+ test -z "$verbose_log" && verbose=t
+fi
+
# 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"
+then
mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
@@ -94,8 +183,35 @@ done,*)
echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
test "$(cat "$BASE.exit")" = 0
exit
- ;;
-esac
+fi
+
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ #
+ # Perform this version check _after_ the test script was
+ # potentially re-executed with $TEST_SHELL_PATH for '--tee' or
+ # '--verbose-log', so the right shell is checked and the
+ # warning is issued only once.
+ if test -n "$BASH_VERSION" && eval '
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ '
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
+if test -n "$trace" && test -z "$verbose_log"
+then
+ verbose=t
+fi
# For repeatability, reset the environment to known value.
# TERM is sanitized below, after saving color control sequences.
@@ -193,7 +309,7 @@ fi
# Add libc MALLOC and MALLOC_PERTURB test
# only if we are not executing the test with valgrind
-if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
+if test -n "$valgrind" ||
test -n "$TEST_NO_MALLOC_CHECK"
then
setup_malloc_check () {
@@ -264,107 +380,6 @@ test "x$TERM" != "xdumb" && (
) &&
color=t
-store_arg_to=
-prev_opt=
-for opt
-do
- 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 ;;
- -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
- 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 ;;
- -r)
- store_arg_to=run_list
- ;;
- --run=*)
- run_list=${opt#--*=} ;;
- -h|--h|--he|--hel|--help)
- help=t ;;
- -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
- verbose=t ;;
- --verbose-only=*)
- 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 ;;
- --with-dashes)
- with_dashes=t ;;
- --no-color)
- color= ;;
- --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
- valgrind=memcheck ;;
- --valgrind=*)
- valgrind=${opt#--*=} ;;
- --valgrind-only=*)
- valgrind_only=${opt#--*=} ;;
- --tee)
- ;; # was handled already
- --root=*)
- root=${opt#--*=} ;;
- --chain-lint)
- GIT_TEST_CHAIN_LINT=1 ;;
- --no-chain-lint)
- GIT_TEST_CHAIN_LINT=0 ;;
- -x)
- trace=t ;;
- -V|--verbose-log)
- verbose_log=t ;;
- *)
- echo "error: unknown test option '$opt'" >&2; exit 1 ;;
- esac
-
- prev_opt=$opt
-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
- test -z "$verbose" && verbose_only="$valgrind_only"
-elif test -n "$valgrind"
-then
- test -z "$verbose_log" && verbose=t
-fi
-
-if test -n "$trace" && test -n "$test_untraceable"
-then
- # '-x' tracing requested, but this test script can't be reliably
- # traced, unless it is run with a Bash version supporting
- # BASH_XTRACEFD (introduced in Bash v4.1).
- if test -n "$BASH_VERSION" && eval '
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- '
- then
- : Executed by a Bash version supporting BASH_XTRACEFD. Good.
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- trace=
- fi
-fi
-if test -n "$trace" && test -z "$verbose_log"
-then
- verbose=t
-fi
-
if test -n "$color"
then
# Save the color control sequences now rather than run tput
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 5/8] test-lib: consolidate naming of test-results paths
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (3 preceding siblings ...)
2019-01-05 1:08 ` [PATCH v4 4/8] test-lib: parse command line options earlier SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 6/8] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
` (3 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
There are two places where we strip off any leading path components
and the '.sh' suffix from the test script's pathname, and there are
four places where we construct the name of the 't/test-results'
directory or the name of various test-specific files in there. The
last patch in this series will add even more.
Factor these out into helper variables to avoid repeating ourselves.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 14ccb60838..5720292641 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -160,6 +160,10 @@ then
test -z "$verbose_log" && verbose=t
fi
+TEST_NAME="$(basename "$0" .sh)"
+TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
+TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
@@ -167,12 +171,11 @@ then
: # do not redirect again
elif test -n "$tee"
then
- mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
- BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
+ mkdir -p "$TEST_RESULTS_DIR"
# Make this filename available to the sub-process in case it is using
# --verbose-log.
- GIT_TEST_TEE_OUTPUT_FILE=$BASE.out
+ GIT_TEST_TEE_OUTPUT_FILE=$TEST_RESULTS_BASE.out
export GIT_TEST_TEE_OUTPUT_FILE
# Truncate before calling "tee -a" to get rid of the results
@@ -180,8 +183,8 @@ then
>"$GIT_TEST_TEE_OUTPUT_FILE"
(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
- echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
- test "$(cat "$BASE.exit")" = 0
+ echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
+ test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
exit
fi
@@ -840,12 +843,9 @@ test_done () {
if test -z "$HARNESS_ACTIVE"
then
- test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
- mkdir -p "$test_results_dir"
- base=${0##*/}
- test_results_path="$test_results_dir/${base%.sh}.counts"
+ mkdir -p "$TEST_RESULTS_DIR"
- cat >"$test_results_path" <<-EOF
+ cat >"$TEST_RESULTS_BASE.counts" <<-EOF
total $test_count
success $test_success
fixed $test_fixed
@@ -1051,7 +1051,7 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$(basename "$0" .sh)"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 6/8] test-lib: set $TRASH_DIRECTORY earlier
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (4 preceding siblings ...)
2019-01-05 1:08 ` [PATCH v4 5/8] test-lib: consolidate naming of test-results paths SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 7/8] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
` (2 subsequent siblings)
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
A later patch in this series will need to know the path to the trash
directory early in 'test-lib.sh', but $TRASH_DIRECTORY is set much
later.
Set $TRASH_DIRECTORY earlier, where the other test-specific path
variables are set.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5720292641..0e9ac9118d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -163,6 +163,12 @@ fi
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
+test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
+case "$TRASH_DIRECTORY" in
+/*) ;; # absolute path is good
+ *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
+esac
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
@@ -1051,12 +1057,6 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
-test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
-case "$TRASH_DIRECTORY" in
-/*) ;; # absolute path is good
- *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
-esac
rm -fr "$TRASH_DIRECTORY" || {
GIT_EXIT_OK=t
echo >&5 "FATAL: Cannot prepare test area"
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 7/8] test-lib-functions: introduce the 'test_set_port' helper function
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (5 preceding siblings ...)
2019-01-05 1:08 ` [PATCH v4 6/8] test-lib: set $TRASH_DIRECTORY earlier SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-05 1:08 ` [PATCH v4 8/8] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
2019-01-07 8:49 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests Jeff King
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
Several test scripts run daemons like 'git-daemon' or Apache, and
communicate with them through TCP sockets. To have unique ports where
these daemons are accessible, the ports are usually the number of the
corresponding test scripts, unless the user overrides them via
environment variables, and thus all those tests and test libs contain
more or less the same bit of one-liner boilerplate code to find out
the port. The last patch in this series will make this a bit more
complicated.
Factor out finding the port for a daemon into the common helper
function 'test_set_port' to avoid repeating ourselves.
Take special care of test scripts with "low" numbers:
- Test numbers below 1024 would result in a port that's only usable
as root, so set their port to '10000 + test-nr' to make sure it
doesn't interfere with other tests in the test suite. This makes
the hardcoded port number in 't0410-partial-clone.sh' unnecessary,
remove it.
- The shell's arithmetic evaluation interprets numbers with leading
zeros as octal values, which means that test number below 1000 and
containing the digits 8 or 9 will trigger an error. Remove all
leading zeros from the test numbers to prevent this.
Note that the 'git p4' tests are unlike the other tests involving
daemons in that:
- 'lib-git-p4.sh' doesn't use the test's number for unique port as
is, but does a bit of additional arithmetic on top [1].
- The port is not overridable via an environment variable.
With this patch even 'git p4' tests will use the test's number as
default port, and it will be overridable via the P4DPORT environment
variable.
[1] Commit fc00233071 (git-p4 tests: refactor and cleanup, 2011-08-22)
introduced that "unusual" unique port computation without
explaining why it was necessary (as opposed to simply using the
test number as is). It seems to be just unnecessary complication,
and in any case that commit came way before the "test nr as unique
port" got "standardized" for other daemons in commits c44132fcf3
(tests: auto-set git-daemon port, 2014-02-10), 3bb486e439 (tests:
auto-set LIB_HTTPD_PORT from test name, 2014-02-10), and
bf9d7df950 (t/lib-git-svn.sh: improve svnserve tests with parallel
make test, 2017-12-01).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +--------
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 36 ++++++++++++++++++++++++++++++++++++
7 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index f98de95c15..41eb1e3ae8 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -28,7 +28,7 @@ then
test_skip_or_die $GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
fi
-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}
+test_set_port LIB_GIT_DAEMON_PORT
GIT_DAEMON_PID=
GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index c27599474c..b3be3ba011 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -53,14 +53,7 @@ time_in_seconds () {
(cd / && "$PYTHON_PATH" -c 'import time; print(int(time.time()))')
}
-# Try to pick a unique port: guess a large number, then hope
-# no more than one of each test is running.
-#
-# This does not handle the case where somebody else is running the
-# same tests and has chosen the same ports.
-testid=${this_test#t}
-git_p4_test_start=9800
-P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
+test_set_port P4DPORT
P4PORT=localhost:$P4DPORT
P4CLIENT=client
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index a8130f9119..f3b478c307 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -13,6 +13,7 @@ fi
GIT_DIR=$PWD/.git
GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn
SVN_TREE=$GIT_SVN_DIR/svn-tree
+test_set_port SVNSERVE_PORT
svn >/dev/null 2>&1
if test $? -ne 1
@@ -119,7 +120,6 @@ require_svnserve () {
}
start_svnserve () {
- SVNSERVE_PORT=${SVNSERVE_PORT-${this_test#t}}
svnserve --listen-port $SVNSERVE_PORT \
--root "$rawsvnrepo" \
--listen-once \
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a8729f8232..e465116ef9 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -82,7 +82,7 @@ case $(uname) in
esac
LIB_HTTPD_PATH=${LIB_HTTPD_PATH-"$DEFAULT_HTTPD_PATH"}
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-${this_test#t}}
+test_set_port LIB_HTTPD_PORT
TEST_PATH="$TEST_DIRECTORY"/lib-httpd
HTTPD_ROOT_PATH="$PWD"/httpd
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..0aca8d7588 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -480,7 +480,6 @@ test_expect_success 'gc stops traversal when a missing but promised object is re
! grep "$TREE_HASH" out
'
-LIB_HTTPD_PORT=12345 # default port, 410, cannot be used as non-root
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..cd9e60632d 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -260,7 +260,7 @@ test_lazy_prereq GIT_DAEMON '
# This test spawns a daemon, so run it only if the user would be OK with
# testing with git-daemon.
test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
- JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+ test_set_port JGIT_DAEMON_PORT &&
JGIT_DAEMON_PID= &&
git init --bare empty.git &&
>empty.git/git-daemon-export-ok &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6b3bbf99e4..4459bdda13 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1263,3 +1263,39 @@ test_oid () {
fi &&
eval "printf '%s' \"\${$var}\""
}
+
+# Choose a port number based on the test script's number and store it in
+# the given variable name, unless that variable already contains a number.
+test_set_port () {
+ local var=$1 port
+
+ if test $# -ne 1 || test -z "$var"
+ then
+ BUG "test_set_port requires a variable name"
+ fi
+
+ eval port=\$$var
+ case "$port" in
+ "")
+ # No port is set in the given env var, use the test
+ # number as port number instead.
+ # Remove not only the leading 't', but all leading zeros
+ # as well, so the arithmetic below won't (mis)interpret
+ # a test number like '0123' as an octal value.
+ port=${this_test#${this_test%%[1-9]*}}
+ if test "${port:-0}" -lt 1024
+ then
+ # root-only port, use a larger one instead.
+ port=$(($port + 10000))
+ fi
+
+ eval $var=$port
+ ;;
+ *[^0-9]*|0*)
+ error >&7 "invalid port number: $port"
+ ;;
+ *)
+ # The user has specified the port.
+ ;;
+ esac
+}
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 8/8] test-lib: add the '--stress' option to run a test repeatedly under load
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (6 preceding siblings ...)
2019-01-05 1:08 ` [PATCH v4 7/8] test-lib-functions: introduce the 'test_set_port' helper function SZEDER Gábor
@ 2019-01-05 1:08 ` SZEDER Gábor
2019-01-07 8:49 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests Jeff King
8 siblings, 0 replies; 67+ messages in thread
From: SZEDER Gábor @ 2019-01-05 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, SZEDER Gábor
Unfortunately, we have a few flaky tests, whose failures tend to be
hard to reproduce. We've found that the best we can do to reproduce
such a failure is to run the test script repeatedly while the machine
is under load, and wait in the hope that the load creates enough
variance in the timing of the test's commands that a failure is
evenually triggered. I have a command to do that, and I noticed that
two other contributors have rolled their own scripts to do the same,
all choosing slightly different approaches.
To help reproduce failures in flaky tests, introduce the '--stress'
option to run a test script repeatedly in multiple parallel jobs until
one of them fails, thereby using the test script itself to increase
the load on the machine.
The number of parallel jobs is determined by, in order of precedence:
the number specified as '--stress=<N>', or the value of the
GIT_TEST_STRESS_LOAD environment variable, or twice the number of
available processors (as reported by the 'getconf' utility), or 8.
Make '--stress' imply '--verbose -x --immediate' to get the most
information about rare failures; there is really no point in spending
all the extra effort to reproduce such a failure, and then not know
which command failed and why.
To prevent the several parallel invocations of the same test from
interfering with each other:
- Include the parallel job's number in the name of the trash
directory and the various output files under 't/test-results/' as
a '.stress-<Nr>' suffix.
- Add the parallel job's number to the port number specified by the
user or to the test number, so even tests involving daemons
listening on a TCP socket can be stressed.
- Redirect each parallel test run's verbose output to
't/test-results/$TEST_NAME.stress-<nr>.out', because dumping the
output of several parallel running tests to the terminal would
create a big ugly mess.
For convenience, print the output of the failed test job at the end,
and rename its trash directory to end with the '.stress-failed'
suffix, so it's easy to find in a predictable path (OTOH, all absolute
paths recorded in the trash directory become invalid; we'll see
whether this causes any issues in practice). If, in an unlikely case,
more than one jobs were to fail nearly at the same time, then print
the output of all failed jobs, and rename the trash directory of only
the last one (i.e. with the highest job number), as it is the trash
directory of the test whose output will be at the bottom of the user's
terminal.
Based on Jeff King's 'stress' script.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/README | 19 ++++++-
t/test-lib-functions.sh | 7 ++-
t/test-lib.sh | 109 +++++++++++++++++++++++++++++++++++++++-
3 files changed, 130 insertions(+), 5 deletions(-)
diff --git a/t/README b/t/README
index 28711cc508..11ce7675e3 100644
--- a/t/README
+++ b/t/README
@@ -186,6 +186,22 @@ appropriately before running "make".
this feature by setting the GIT_TEST_CHAIN_LINT environment
variable to "1" or "0", respectively.
+--stress::
+--stress=<N>::
+ Run the test script repeatedly in multiple parallel jobs until
+ one of them fails. Useful for reproducing rare failures in
+ flaky tests. The number of parallel jobs is, in order of
+ precedence: <N>, or the value of the GIT_TEST_STRESS_LOAD
+ environment variable, or twice the number of available
+ processors (as shown by the 'getconf' utility), or 8.
+ Implies `--verbose -x --immediate` to get the most information
+ about the failure. Note that the verbose output of each test
+ job is saved to 't/test-results/$TEST_NAME.stress-<nr>.out',
+ and only the output of the failed test job is shown on the
+ terminal. The names of the trash directories get a
+ '.stress-<nr>' suffix, and the trash directory of the failed
+ test job is renamed to end with a '.stress-failed' suffix.
+
You can also set the GIT_TEST_INSTALLED environment variable to
the bindir of an existing git installation to test that installation.
You still need to have built this git sandbox, from which various
@@ -425,7 +441,8 @@ This test harness library does the following things:
- Creates an empty test directory with an empty .git/objects database
and chdir(2) into it. This directory is 't/trash
directory.$test_name_without_dotsh', with t/ subject to change by
- the --root option documented above.
+ the --root option documented above, and a '.stress-<N>' suffix
+ appended by the --stress option.
- Defines standard test helper functions for your scripts to
use. These functions are designed to make all scripts behave
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 4459bdda13..92cf8f812c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1288,8 +1288,6 @@ test_set_port () {
# root-only port, use a larger one instead.
port=$(($port + 10000))
fi
-
- eval $var=$port
;;
*[^0-9]*|0*)
error >&7 "invalid port number: $port"
@@ -1298,4 +1296,9 @@ test_set_port () {
# The user has specified the port.
;;
esac
+
+ # Make sure that parallel '--stress' test jobs get different
+ # ports.
+ port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
+ eval $var=$port
}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0e9ac9118d..a1abb1177a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -139,6 +139,19 @@ do
verbose_log=t
tee=t
;;
+ --stress)
+ stress=t ;;
+ --stress=*)
+ stress=${opt#--*=}
+ case "$stress" in
+ *[^0-9]*|0*|"")
+ echo "error: --stress=<N> requires the number of jobs to run" >&2
+ exit 1
+ ;;
+ *) # Good.
+ ;;
+ esac
+ ;;
*)
echo "error: unknown test option '$opt'" >&2; exit 1 ;;
esac
@@ -160,16 +173,108 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$stress"
+then
+ verbose=t
+ trace=t
+ immediate=t
+fi
+
+TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
-TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
+TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX"
+TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
*) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
esac
+# If --stress was passed, run this test repeatedly in several parallel loops.
+if test "$GIT_TEST_STRESS_STARTED" = "done"
+then
+ : # Don't stress test again.
+elif test -n "$stress"
+then
+ if test "$stress" != t
+ then
+ job_count=$stress
+ elif test -n "$GIT_TEST_STRESS_LOAD"
+ then
+ job_count="$GIT_TEST_STRESS_LOAD"
+ elif job_count=$(getconf _NPROCESSORS_ONLN 2>/dev/null) &&
+ test -n "$job_count"
+ then
+ job_count=$((2 * $job_count))
+ else
+ job_count=8
+ fi
+
+ mkdir -p "$TEST_RESULTS_DIR"
+ stressfail="$TEST_RESULTS_BASE.stress-failed"
+ rm -f "$stressfail"
+
+ stress_exit=0
+ trap '
+ kill $job_pids 2>/dev/null
+ wait
+ stress_exit=1
+ ' TERM INT HUP
+
+ job_pids=
+ job_nr=0
+ while test $job_nr -lt "$job_count"
+ do
+ (
+ GIT_TEST_STRESS_STARTED=done
+ GIT_TEST_STRESS_JOB_NR=$job_nr
+ export GIT_TEST_STRESS_STARTED GIT_TEST_STRESS_JOB_NR
+
+ trap '
+ kill $test_pid 2>/dev/null
+ wait
+ exit 1
+ ' TERM INT
+
+ cnt=0
+ while ! test -e "$stressfail"
+ do
+ $TEST_SHELL_PATH "$0" "$@" >"$TEST_RESULTS_BASE.stress-$job_nr.out" 2>&1 &
+ test_pid=$!
+
+ if wait $test_pid
+ then
+ printf "OK %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
+ else
+ echo $GIT_TEST_STRESS_JOB_NR >>"$stressfail"
+ printf "FAIL %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
+ fi
+ cnt=$(($cnt + 1))
+ done
+ ) &
+ job_pids="$job_pids $!"
+ job_nr=$(($job_nr + 1))
+ done
+
+ wait
+
+ if test -f "$stressfail"
+ then
+ echo "Log(s) of failed test run(s):"
+ for failed_job_nr in $(sort -n "$stressfail")
+ do
+ echo "Contents of '$TEST_RESULTS_BASE.stress-$failed_job_nr.out':"
+ cat "$TEST_RESULTS_BASE.stress-$failed_job_nr.out"
+ done
+ rm -rf "$TRASH_DIRECTORY.stress-failed"
+ # Move the last one.
+ mv "$TRASH_DIRECTORY.stress-$failed_job_nr" "$TRASH_DIRECTORY.stress-failed"
+ fi
+
+ exit $stress_exit
+fi
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
--
2.20.1.151.gec613c4b75
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
2019-01-05 1:08 ` [PATCH v4 0/8] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests SZEDER Gábor
` (7 preceding siblings ...)
2019-01-05 1:08 ` [PATCH v4 8/8] test-lib: add the '--stress' option to run a test repeatedly under load SZEDER Gábor
@ 2019-01-07 8:49 ` Jeff King
8 siblings, 0 replies; 67+ messages in thread
From: Jeff King @ 2019-01-07 8:49 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git
On Sat, Jan 05, 2019 at 02:08:51AM +0100, SZEDER Gábor wrote:
> To recap: this patch series tries to make reproducing rare failures in
> flaky tests easier: it adds the '--stress' option to our test library
> to run the test script repeatedly in multiple parallel jobs, in the
> hope that the increased load creates enough variance in the timing of
> the test's commands that such a failure is eventually triggered.
Thanks, I looked over this version (glossing over the bits that hadn't
really changed since v2) and it all looks good to me.
-Peff
^ permalink raw reply [flat|nested] 67+ messages in thread