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@xxxxxxxxxxxxx>
>>
>> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>> ---
>> 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
