Re: [PATCH v2 07/13] tests: introduce `test_atexit`

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Mon, Oct 15, 2018 at 03:12:08AM -0700, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> When running the p4 daemon or `git daemon`, we want to kill it at the
> end of the test script.
> 
> So far, we do this "manually".
> 
> However, in the next few commits we want to teach the test suite to
> optionally re-run scripts with different options, therefore we will have
> to have a consistent way to stop daemons.
> 
> 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 `--immediate` option is in effect).

I think killing daemons on failure even with '--immediate' will make
life a bit easier.  I remember several occasions when I tried to debug
a failing test with '--immediate', some daemon stayed alive, and then
the next time I run the test it failed during setup.

Besides 'git daemon' and P4, the httpd tests could benefit from this
as well.


> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 78d8c3783b..d7dd0c1be9 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -891,6 +891,35 @@ 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
> +#	'

I think we should add something like this as well:

# Note that these commands will be run even when a test script run
# with '--immediate' fails.  Be careful with your 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"
> +}
> +
> +test_atexit_handler () {
> +	test : != "$test_atexit_cleanup" || return 0
> +
> +	setup_malloc_check
> +	test_eval_ "$test_atexit_cleanup"
> +	test_atexit_cleanup=:
> +	teardown_malloc_check
> +}

The file 'test-lib-functions.sh' contains helper functions to be used
in tests.  'test_atexit_handler' is not such a function, so I think it
would be better to add it to 'test-lib.sh'.

> +
>  # 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 7ed0013f6d..6f9c1f5300 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -413,6 +413,7 @@ test_external_has_tap=0
>  
>  die () {
>  	code=$?

# Run atexit handlers even when a test script run with
# '--immediate' fails.

> +	test_atexit_handler || code=$?
>  	if test -n "$GIT_EXIT_OK"
>  	then
>  		exit $code
> @@ -826,9 +827,12 @@ write_junit_xml_testcase () {
>  	junit_have_testcase=t
>  }
>  
> +test_atexit_cleanup=:
>  test_done () {
>  	GIT_EXIT_OK=t
>  
> +	test -n "$immediate" || test_atexit_handler

You mentioned elsewhere [1] that you'll remove the condition to run
'test_atexit_handler' unconditionally.  I think this is the right
thing to do, because this condition is responsible for the hanging P4
tests I reported in [2].

So, 'lib-git-p4.sh' stores the pid of the 'p4d' process in the file
"$TRASH_DIRECTORY/p4d.pid" in 'start_p4d', to be read later in
'kill_p4d' to know which process to kill.  When a test script run with
'--immediate' ends successfully, and the atexit handlers are run in
'die' above instead of in 'test_done', then you've got the problem
that 'test_done' removes the trash directory along with the 'p4d.pid'
file before the atexit handlers are run.  With the pidfile gone
'kill_p4d' won't know which process to kill, and the 'p4d' process
will stay alive.

If that condition is removed, then the atexit handlers will be run
before the trash directory is removed, so 'p4d' processes will be
stopped as they should even when an '--immediate' run succeeds.  Good.
Note, however, that in this case 'test_atexit_handler' will be called
twice: first in 'test_done' and then in 'die' as well.  This should be
fine, because 'test_atexit_handler' resets 'test_atexit_cleanup=:', so
the second call will do nothing.  Great.  Perhaps this would be worth
pointing out explicitly in the commit message and/or a comment.

[1] https://public-inbox.org/git/nycvar.QRO.7.76.6.1810161111240.4546@tvgsbejvaqbjf.bet/
[2] https://public-inbox.org/git/20181017232952.GT19800@szeder.dev/

> +
>  	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
>  	then
>  		test -n "$junit_have_testcase" || {
> -- 
> gitgitgadget