| Message ID | 20190313122419.2210-4-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests: introduce 'test_atexit' | expand |
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" || {
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" || {
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.
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" || {