[03/11] test-lib: introduce 'test_atexit'
diff mbox series

Message ID 20190313122419.2210-4-szeder.dev@gmail.com
State New
Headers show
Series
  • tests: introduce 'test_atexit'
Related show

Commit Message

SZEDER Gábor March 13, 2019, 12:24 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

When running Apache, 'git daemon', or p4d, we want to kill them at the
end of the test script, otherwise a leftover daemon process will keep
its port open indefinitely, and thus will interfere with subsequent
executions of the same test script.

So far, we stop these daemon processes "manually", i.e.:

  - by registering functions or commands in the trap on EXIT to stop
    the daemon while preserving the last seen exit code before the
    trap (to deal with a failure when run with '--immediate' or with
    interrupts by ctrl-C),

  - and by invoking these functions/commands last thing before
    'test_done' (and sometimes restoring the test framework's default
    trap on EXIT, to prevent the daemons from being killed twice).

On one hand, we do this inconsistently, e.g. 'git p4' tests invoke
different functions in the trap on EXIT and in the last test before
'test_done', and they neither restore the test framework's default trap
on EXIT nor preserve the last seen exit code.  On the other hand, this
is error prone, because, as shown in a previous patch in this series,
any output from the cleanup commands in the trap on EXIT can prevent a
proper cleanup when a test script run with '--verbose-log' and certain
shells, notably 'dash', is interrupted.

Let's introduce 'test_atexit', which is loosely modeled after
'test_when_finished', but has a broader scope: rather than running the
commands after the current test case, run them when the test script
finishes, and also run them when the test is interrupted, or exits
early in case of a failure while the '--immediate' option is in
effect.

When running the cleanup commands at the end of a successful test,
then they will be run in 'test_done' before it removes the trash
directory, i.e. the cleanup commands will still be able to access any
pidfiles or socket files in there.  When running the cleanup commands
after an interrupt or failure with '--immediate', then they will be
run in the trap on EXIT.  In both cases they will be run in
'test_eval_', i.e. both standard error and output of all cleanup
commands will go where they should according to the '-v' or
'--verbose-log' options, and thus won't cause any troubles when
interrupting a test script run with '--verbose-log'.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/README                | 20 ++++++++++++++++++++
 t/t0000-basic.sh        | 18 ++++++++++++++++++
 t/test-lib-functions.sh | 28 ++++++++++++++++++++++++++++
 t/test-lib.sh           | 23 +++++++++++++++++++++++
 4 files changed, 89 insertions(+)

Comments

Junio C Hamano March 14, 2019, 3:21 a.m. UTC | #1
SZEDER Gábor <szeder.dev@gmail.com> writes:

> +test_atexit () {
> +	# We cannot detect when we are in a subshell in general, but by
> +	# doing so on Bash is better than nothing (the test will
> +	# silently pass on other shells).
> +	test "${BASH_SUBSHELL-0}" = 0 ||
> +	error "bug in test script: test_atexit does nothing in a subshell"
> +	test_atexit_cleanup="{ $*
> +		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
> +}

This chaining is modelled after how $test_cleaup is built and
maintained by test_when_finished.  Use of eval_ret makes sense in
that original context as eval_ret _is_ used to keep track of the
result of 'test_eval_ "$1"' in test_run_ that executed the body
of a single test_expect_$something, and $test_cleanup would want to
keep the resulting status from that body when clean-up succeeds (or
otherwise, make that failure to clean-up visible as $eval_ret).

But does it make sense in the context of the whole test script to
try maintaining $eval_ret?

>  # Most tests can use the created repository, but some may need to create more.
>  # Usage: test_create_repo <directory>
>  test_create_repo () {
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index db3875d1e4..b35881696f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -620,6 +620,10 @@ test_external_has_tap=0
>  
>  die () {
>  	code=$?
> +	# This is responsible for running the atexit commands even when a
> +	# test script run with '--immediate' fails, or when the user hits
> +	# ctrl-C, i.e. when 'test_done' is not invoked at all.
> +	test_atexit_handler || code=$?
>  	if test -n "$GIT_EXIT_OK"
>  	then
>  		exit $code
> @@ -1045,9 +1049,28 @@ write_junit_xml_testcase () {
>  	junit_have_testcase=t
>  }
>  
> +test_atexit_cleanup=:
> +test_atexit_handler () {
> +	# In a succeeding test script 'test_atexit_handler' is invoked
> +	# twice: first from 'test_done', then from 'die' in the trap on
> +	# EXIT.

We are guaranteed to still have the trash directory when we are run
in the exit handler after getting interrupted or test_failure_()
exits under the "-i" option, and when test_done() calls us.  What
will cause us trouble is the exit handler at the end of a successful
run after test_done() finishes.  At that point, test_done would have
already cleared the trash directory, so we may not have enough state
to allow us to clean-up at exit.

Clearing the exit trap in test_done after it calls us might be an
alternative, but I think it is equivalent to clearing the
test_atexit_cleanup variable, and it is cleaner, so I think I agree
with the approach this patch uses.

> +	# This condition and resetting 'test_atexit_cleanup' below makes
> +	# sure that the registered cleanup commands are run only once.
> +	test : != "$test_atexit_cleanup" || return 0

I think test_when_finished uses a special value in $test_cleanup in
a similar way but it even skips when there is no point doing the
test_eval_ of the "accumulated" scriptlet when it is empty.

Shouldn't we be doing the same thing, i.e.

	if test -z "$test_atexit_cleanup"
	then
		return 0
	fi
	... do the heavy lifting ...
	test_atexit_cleanup=

That will make the handler truly a no-op when there is no atexit
defined.

> +	setup_malloc_check
> +	test_eval_ "$test_atexit_cleanup"
> +	test_atexit_cleanup=:
> +	teardown_malloc_check
> +}
> +
>  test_done () {
>  	GIT_EXIT_OK=t
>  
> +	# Run the atexit commands _before_ the trash directory is
> +	# removed, so the commands can access pidfiles and socket files.
> +	test_atexit_handler
> +
>  	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
>  	then
>  		test -n "$junit_have_testcase" || {
SZEDER Gábor March 14, 2019, 5:40 p.m. UTC | #2
On Thu, Mar 14, 2019 at 12:21:00PM +0900, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > +test_atexit () {
> > +	# We cannot detect when we are in a subshell in general, but by
> > +	# doing so on Bash is better than nothing (the test will
> > +	# silently pass on other shells).
> > +	test "${BASH_SUBSHELL-0}" = 0 ||
> > +	error "bug in test script: test_atexit does nothing in a subshell"
> > +	test_atexit_cleanup="{ $*
> > +		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
> > +}
> 
> This chaining is modelled after how $test_cleaup is built and
> maintained by test_when_finished.  Use of eval_ret makes sense in
> that original context as eval_ret _is_ used to keep track of the
> result of 'test_eval_ "$1"' in test_run_ that executed the body
> of a single test_expect_$something, and $test_cleanup would want to
> keep the resulting status from that body when clean-up succeeds (or
> otherwise, make that failure to clean-up visible as $eval_ret).
> 
> But does it make sense in the context of the whole test script to
> try maintaining $eval_ret?

Right, it doesn't, as 'die' preserves the last seen exit code, and any
exit codes from the atexit commands are ignored anyway.

> >  # Most tests can use the created repository, but some may need to create more.
> >  # Usage: test_create_repo <directory>
> >  test_create_repo () {
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index db3875d1e4..b35881696f 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -620,6 +620,10 @@ test_external_has_tap=0
> >  
> >  die () {
> >  	code=$?
> > +	# This is responsible for running the atexit commands even when a
> > +	# test script run with '--immediate' fails, or when the user hits
> > +	# ctrl-C, i.e. when 'test_done' is not invoked at all.
> > +	test_atexit_handler || code=$?
> >  	if test -n "$GIT_EXIT_OK"
> >  	then
> >  		exit $code
> > @@ -1045,9 +1049,28 @@ write_junit_xml_testcase () {
> >  	junit_have_testcase=t
> >  }
> >  
> > +test_atexit_cleanup=:
> > +test_atexit_handler () {
> > +	# In a succeeding test script 'test_atexit_handler' is invoked
> > +	# twice: first from 'test_done', then from 'die' in the trap on
> > +	# EXIT.
> 
> We are guaranteed to still have the trash directory when we are run
> in the exit handler after getting interrupted or test_failure_()
> exits under the "-i" option, and when test_done() calls us.  What
> will cause us trouble is the exit handler at the end of a successful
> run after test_done() finishes.  At that point, test_done would have
> already cleared the trash directory, so we may not have enough state
> to allow us to clean-up at exit.
> 
> Clearing the exit trap in test_done after it calls us might be an
> alternative, but I think it is equivalent to clearing the
> test_atexit_cleanup variable, and it is cleaner, so I think I agree
> with the approach this patch uses.
> 
> > +	# This condition and resetting 'test_atexit_cleanup' below makes
> > +	# sure that the registered cleanup commands are run only once.
> > +	test : != "$test_atexit_cleanup" || return 0
> 
> I think test_when_finished uses a special value in $test_cleanup in
> a similar way

That's right.

> but it even skips when there is no point doing the
> test_eval_ of the "accumulated" scriptlet when it is empty.

But this is not, because $test_cleanup is initialized to this special
value and it can never be empty, and indeed 'test_eval_' uses this
condition:

  if test -z "$immediate" || test $eval_ret = 0 ||
     test -n "$expecting_failure" && test "$test_cleanup" != ":"

and it never checks $test_cleanup's emptiness.

> Shouldn't we be doing the same thing, i.e.
> 
> 	if test -z "$test_atexit_cleanup"
> 	then
> 		return 0
> 	fi
> 	... do the heavy lifting ...
> 	test_atexit_cleanup=
> 
> That will make the handler truly a no-op when there is no atexit
> defined.

$test_atexit_cleanup is used the same way as $test_cleanup; it's
initialized to the same special value and can never be empty, so there
is no need to check for its emptiness either.

> > +	setup_malloc_check
> > +	test_eval_ "$test_atexit_cleanup"
> > +	test_atexit_cleanup=:
> > +	teardown_malloc_check
> > +}
> > +
> >  test_done () {
> >  	GIT_EXIT_OK=t
> >  
> > +	# Run the atexit commands _before_ the trash directory is
> > +	# removed, so the commands can access pidfiles and socket files.
> > +	test_atexit_handler
> > +
> >  	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
> >  	then
> >  		test -n "$junit_have_testcase" || {
Junio C Hamano March 18, 2019, 1:50 a.m. UTC | #3
SZEDER Gábor <szeder.dev@gmail.com> writes:

>> but it even skips when there is no point doing the
>> test_eval_ of the "accumulated" scriptlet when it is empty.
>
> But this is not, because $test_cleanup is initialized to this special
> value and it can never be empty, and indeed 'test_eval_' uses this
> condition:
>
>   if test -z "$immediate" || test $eval_ret = 0 ||
>      test -n "$expecting_failure" && test "$test_cleanup" != ":"
>
> and it never checks $test_cleanup's emptiness.

Yeah, I used "empty" not in the literal sense; I know why the "empty
in spirit" setting is a single colon for $test_cleanup.

I just did not realize that this new variable was using that exact
pattern and using ":" as the empty in spirit, and that was where my
fuzzy wordibng came from.

So in short, I misread the code, and part of that is because I was
misled by the comment:

> +	# This condition and resetting 'test_atexit_cleanup' below makes
> +	# sure that the registered cleanup commands are run only once.
> +	test : != "$test_atexit_cleanup" || return 0

It over-stresses the "run only once", but the true value of this is
that it avoids running an "empty in spirit" clean-up sequence.  

The avoidance of double execution merely takes advantage of this
implementation detail by "resetting" the variable is better
explained where the "resetting" happens (i.e. "we reset the variable
to the 'no-command' state, as we've run all of them here, but just
before the process finally exits, the helper will be called and we
do not want to run these commands again when it happens").

Thanks.

Patch
diff mbox series

diff --git a/t/README b/t/README
index 886bbec5bc..af510d236a 100644
--- a/t/README
+++ b/t/README
@@ -862,6 +862,26 @@  library for your script to use.
 		...
 	'
 
+ - test_atexit <script>
+
+   Prepend <script> to a list of commands to run unconditionally to
+   clean up before the test script exits, e.g. to stop a daemon:
+
+	test_expect_success 'test git daemon' '
+		git daemon &
+		daemon_pid=$! &&
+		test_atexit 'kill $daemon_pid' &&
+		hello world
+	'
+
+   The commands will be executed before the trash directory is removed,
+   i.e. the atexit commands will still be able to access any pidfiles or
+   socket files.
+
+   Note that these commands will be run even when a test script run
+   with '--immediate' fails.  Be careful with your atexit commands to
+   minimize any changes to the failed state.
+
  - test_write_lines <lines>
 
    Write <lines> on standard output, one line per argument.
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index b6566003dd..c03054c538 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -825,6 +825,24 @@  test_expect_success 'tests clean up even on failures' "
 	EOF
 "
 
+test_expect_success 'test_atexit is run' "
+	test_must_fail run_sub_test_lib_test \
+		atexit-cleanup 'Run atexit commands' -i <<-\\EOF &&
+	test_expect_success 'tests clean up even after a failure' '
+		> ../../clean-atexit &&
+		test_atexit rm ../../clean-atexit &&
+		> ../../also-clean-atexit &&
+		test_atexit rm ../../also-clean-atexit &&
+		> ../../dont-clean-atexit &&
+		(exit 1)
+	'
+	test_done
+	EOF
+	test_path_is_file dont-clean-atexit &&
+	test_path_is_missing clean-atexit &&
+	test_path_is_missing also-clean-atexit
+"
+
 test_expect_success 'test_oid setup' '
 	test_oid_init
 '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 80402a428f..6a50dba390 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -934,6 +934,34 @@  test_when_finished () {
 		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }
 
+# This function can be used to schedule some commands to be run
+# unconditionally at the end of the test script, e.g. to stop a daemon:
+#
+#	test_expect_success 'test git daemon' '
+#		git daemon &
+#		daemon_pid=$! &&
+#		test_atexit 'kill $daemon_pid' &&
+#		hello world
+#	'
+#
+# The commands will be executed before the trash directory is removed,
+# i.e. the atexit commands will still be able to access any pidfiles or
+# socket files.
+#
+# Note that these commands will be run even when a test script run
+# with '--immediate' fails.  Be careful with your atexit commands to
+# minimize any changes to the failed state.
+
+test_atexit () {
+	# We cannot detect when we are in a subshell in general, but by
+	# doing so on Bash is better than nothing (the test will
+	# silently pass on other shells).
+	test "${BASH_SUBSHELL-0}" = 0 ||
+	error "bug in test script: test_atexit does nothing in a subshell"
+	test_atexit_cleanup="{ $*
+		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
+}
+
 # Most tests can use the created repository, but some may need to create more.
 # Usage: test_create_repo <directory>
 test_create_repo () {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index db3875d1e4..b35881696f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -620,6 +620,10 @@  test_external_has_tap=0
 
 die () {
 	code=$?
+	# This is responsible for running the atexit commands even when a
+	# test script run with '--immediate' fails, or when the user hits
+	# ctrl-C, i.e. when 'test_done' is not invoked at all.
+	test_atexit_handler || code=$?
 	if test -n "$GIT_EXIT_OK"
 	then
 		exit $code
@@ -1045,9 +1049,28 @@  write_junit_xml_testcase () {
 	junit_have_testcase=t
 }
 
+test_atexit_cleanup=:
+test_atexit_handler () {
+	# In a succeeding test script 'test_atexit_handler' is invoked
+	# twice: first from 'test_done', then from 'die' in the trap on
+	# EXIT.
+	# This condition and resetting 'test_atexit_cleanup' below makes
+	# sure that the registered cleanup commands are run only once.
+	test : != "$test_atexit_cleanup" || return 0
+
+	setup_malloc_check
+	test_eval_ "$test_atexit_cleanup"
+	test_atexit_cleanup=:
+	teardown_malloc_check
+}
+
 test_done () {
 	GIT_EXIT_OK=t
 
+	# Run the atexit commands _before_ the trash directory is
+	# removed, so the commands can access pidfiles and socket files.
+	test_atexit_handler
+
 	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
 	then
 		test -n "$junit_have_testcase" || {