Re: [PATCH 11/11] test-lib: clear watchman watches at test completion

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

On Thu, Nov 21, 2019 at 10:20:26PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/test-lib-functions.sh | 15 +++++++++++++++
>  t/test-lib.sh           |  2 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index e0b3f28d3a..03573caf42 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1475,3 +1475,18 @@ test_set_port () {
>  	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
>  	eval $var=$port
>  }
> +
> +test_clear_watchman () {
> +	if test $GIT_TEST_FSMONITOR -ne ""

In the rare cases when this function is invoked (see below) this
condition triggers an error from the shell running test script:

  - when the variable is not set, because of the lack of quotes around
    the variable name:

      $ ./t5570-git-daemon.sh 
      [....]
      ok 21 - hostname interpolation works after LF-stripping
      ./t5570-git-daemon.sh: 1482: test: -ne: unexpected operator
      # passed all 21 test(s)
      1..21

  - when the variable is set, because the '-ne' operator does integer
    comparison:

      $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh
      [...]
      ok 21 - hostname interpolation works after LF-stripping
      ./t5570-git-daemon.sh: 1482: test: Illegal number: /home/szeder/src/git/t/t7519/fsmonitor-none
      # failed 1 among 21 test(s)
      1..21

Please use 'if test -n "$GIT_TEST_FSMONITOR"' instead.

> +	then
> +		watchman watch-list |

Then with the above fixed, trying to run 'watchman' triggers another
error if it's not installed:

  $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh 
  [...]
  ok 21 - hostname interpolation works after LF-stripping
  ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found
  # failed 1 among 21 test(s)

I think we need an additional condition to run this only if
't7519/fsmonitor-watchman' is used in the tests.

> +			grep "$TRASH_DIRECTORY" |
> +			sed "s/\t\"//g" |
> +			sed "s/\",//g" >repo-list
> +
> +		for repo in $(cat repo-list)
> +		do
> +			watchman watch-del "$repo"
> +		done
> +	fi
> +}
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 30b07e310f..067a432ea5 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1072,6 +1072,8 @@ test_atexit_handler () {
>  	# sure that the registered cleanup commands are run only once.
>  	test : != "$test_atexit_cleanup" || return 0
>  
> +	test_clear_watchman

I'm not sure where to put this call, but this is definitely not the
right place for it.  See that 'return 0' above in the context?  That's
where the test_atexit_handler function returns early when no atexit
handler commands are set, i.e. in all test scripts that don't involve
some kind of daemons, thus this call is not invoked in the majority of
test scripts.

Simply moving this call before that early return is not good, because
then it would be invoked twice.

An option would be to register this call as an atexit command
somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON
is restored, perhaps).  That way it would be invoked most of the time,
and it would be invoked only once, but I'm not sure how it would work
out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the
middle for the remainder of the test script.  However, register the
atexit command only if GIT_TEST_FSMONITOR is set (to something
watchman-specific), so it won't be invoked at all if
GIT_TEST_FSMONITOR is not set, and thus it won't generate additional
test output and trace.

I don't have a better idea.

> +
>  	setup_malloc_check
>  	test_eval_ "$test_atexit_cleanup"
>  	test_atexit_cleanup=:
> -- 
> gitgitgadget