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

From: Derrick Stolee <stolee@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <dstolee@microsoft.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 11/11] test-lib: clear watchman watches at test completion
Date: Mon, 9 Dec 2019 09:12:37 -0500	[thread overview]
Message-ID: <93325ecb-0bbe-e7fb-3b60-8fff81768f8f@gmail.com> (raw)
In-Reply-To: <20191122010645.GX23183@szeder.dev>

On 11/21/2019 8:06 PM, SZEDER Gábor wrote:

Thanks for this message. Sorry I'm so late getting back to it.

> 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.

Thanks for the pointers.

>> +	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.

The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR,
and that can only use watchman (currently). Barring wanting to unset the
variable if it was set on purpose in a test script, the other options do
not actually return correct values to make use of the feature.

>> +			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.

Ah, I misunderstood the point of test_atexit_handler.

> 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.

Shouldn't it be sufficient to add it into test_done? If the test fails,
then we could leave watches open, but that's no worse than we had without
this test_clear_watchman method.

Thanks,
-Stolee