v5, 6 partsimplement test_might_fail using a refactored test_must_fail

>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>   t/lib-git-p4.sh | 31 +++++++++++++++++++++++--------
>   1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index 7548225..8d6b48f 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -6,6 +6,10 @@
>   # a subdirectory called "$git"
>   TEST_NO_CREATE_REPO=NoThanks
>
> +# Some operations require multiple attempts to be successful. Define
> +# here the maximal retry timeout in seconds.
> +RETRY_TIMEOUT=60
> +
>   . ./test-lib.sh
>
>   if ! test_have_prereq PYTHON
> @@ -121,22 +125,33 @@ p4_add_user() {
>   	EOF
>   }
>
> +retry_until_success() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :

> +    done
> +}
> +
> +retry_until_fail() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :

> +    done
> +}
> +
>   kill_p4d() {
>   	pid=$(cat "$pidfile")
> -	# it had better exist for the first kill
> -	kill $pid &&
> -	for i in 1 2 3 4 5 ; do
> -		kill $pid >/dev/null 2>&1 || break
> -		sleep 1
> -	done &&
> +	retry_until_fail kill $pid
> +	retry_until_fail kill -9 $pid
>   	# complain if it would not die
>   	test_must_fail kill $pid >/dev/null 2>&1 &&
>   	rm -rf "$db" "$cli" "$pidfile"
>   }
>
>   cleanup_git() {
> -	rm -rf "$git" &&
> -	mkdir "$git"
> +	retry_until_success rm -r "$git"
> +	test_must_fail test -d "$git" &&
> +	retry_until_success mkdir "$git"
>   }
>
>   marshal_dump() {
>

> On 15/11/15 13:08, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
> 
> Should there be a sleep in that retry_until_success loop so that it doesn't spin sending signals to p4d?

> 
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>>  t/lib-git-p4.sh | 31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>> 
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index 7548225..8d6b48f 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -6,6 +6,10 @@
>>  # a subdirectory called "$git"
>>  TEST_NO_CREATE_REPO=NoThanks
>> 
>> +# Some operations require multiple attempts to be successful. Define
>> +# here the maximal retry timeout in seconds.
>> +RETRY_TIMEOUT=60
>> +
>>  . ./test-lib.sh
>> 
>>  if ! test_have_prereq PYTHON
>> @@ -121,22 +125,33 @@ p4_add_user() {
>>  	EOF
>>  }
>> 
>> +retry_until_success() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
> 
> sleep here?
> 
>> +    done
>> +}
>> +
>> +retry_until_fail() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
> 
> sleep here?
> 
>> +    done
>> +}
>> +
>>  kill_p4d() {
>>  	pid=$(cat "$pidfile")
>> -	# it had better exist for the first kill
>> -	kill $pid &&
>> -	for i in 1 2 3 4 5 ; do
>> -		kill $pid >/dev/null 2>&1 || break
>> -		sleep 1
>> -	done &&
>> +	retry_until_fail kill $pid
>> +	retry_until_fail kill -9 $pid
>>  	# complain if it would not die
>>  	test_must_fail kill $pid >/dev/null 2>&1 &&
>>  	rm -rf "$db" "$cli" "$pidfile"
>>  }
>> 
>>  cleanup_git() {
>> -	rm -rf "$git" &&
>> -	mkdir "$git"
>> +	retry_until_success rm -r "$git"
>> +	test_must_fail test -d "$git" &&
>> +	retry_until_success mkdir "$git"
>>  }
>> 
>>  marshal_dump() {

>
> On 16 Nov 2015, at 09:36, Luke Diamand <luke@diamand.org> wrote:
>
>> On 15/11/15 13:08, larsxschneider@gmail.com wrote:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> In rare cases kill/cleanup operations in tests fail. Retry these
>>> operations with a timeout to make the test less flaky.
>>
>> Should there be a sleep in that retry_until_success loop so that it doesn't spin sending signals to p4d?
> Agreed. I will add a sleep in the next roll!
>
>
>>
>> Do we need to worry about the time offset being updated (e.g. NTP) while this is running?
> Interesting question! I would consider this an edge case but I can see how it could happen.
> Do you see a way to handle that in an easy way?
>

> From: Lars Schneider <larsxschneider@gmail.com>
>
> In rare cases kill/cleanup operations in tests fail. Retry these
> operations with a timeout to make the test less flaky.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> @@ -121,22 +125,33 @@ p4_add_user() {
>         EOF
>  }
>
> +retry_until_success() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))

> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :
> +    done
> +}
> +
> +retry_until_fail() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :
> +    done
> +}

> On Sun, Nov 15, 2015 at 8:08 AM,  <larsxschneider@gmail.com> wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> @@ -121,22 +125,33 @@ p4_add_user() {
>>        EOF
>> }
>> 
>> +retry_until_success() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> 
> There was some discussion previously[1] about detecting dynamically
> whether 'date +%s' was supported. Was this something that you intended
> to do, or did you decide against it since p4 is not supported on such
> platforms?
> 
> Same question also applies to patch 4/6.

> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/280978/match=lazy+prerequisite
> 
>> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
>> +    done
>> +}
>> +
>> +retry_until_fail() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
>> +    done
>> +}

> On 16 Nov 2015, at 22:14, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Nov 15, 2015 at 8:08 AM,  <larsxschneider@gmail.com> wrote:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> In rare cases kill/cleanup operations in tests fail. Retry these
>>> operations with a timeout to make the test less flaky.
>>>
>>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>>> ---
>>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>>> @@ -121,22 +125,33 @@ p4_add_user() {
>>>        EOF
>>> }
>>>
>>> +retry_until_success() {
>>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>>
>> There was some discussion previously[1] about detecting dynamically
>> whether 'date +%s' was supported. Was this something that you intended
>> to do, or did you decide against it since p4 is not supported on such
>> platforms?
>>
>> Same question also applies to patch 4/6.
>
> While implementing it I thought more about it. P4D is only
> supported on platforms that support the date function. That means
> these tests will only run on platforms that support the date
> function. Consequently I wondered if this would justify the
> slightly more complicated code. However, if you think this change
> would help the patch to get accepted then I will add it.

>>
>> While implementing it I thought more about it. P4D is only
>> supported on platforms that support the date function. That means
>> these tests will only run on platforms that support the date
>> function. Consequently I wondered if this would justify the
>> slightly more complicated code. However, if you think this change
>> would help the patch to get accepted then I will add it.
>
> I don't feel strongly about it, and it's not my call anyhow. Opinions
> of Junio, Peff (as interim maintainer), and Luke weigh much more
> heavily than my own. Punting on dynamic detection of "date +%s" may be
> perfectly acceptable with the attitude that it can be implemented
> later if someone runs across a case where it's actually needed.

>>> While implementing it I thought more about it. P4D is only
>>> supported on platforms that support the date function. That means
>>> these tests will only run on platforms that support the date
>>> function. Consequently I wondered if this would justify the
>>> slightly more complicated code. However, if you think this change
>>> would help the patch to get accepted then I will add it.
>>
>> I don't feel strongly about it, and it's not my call anyhow. Opinions
>> of Junio, Peff (as interim maintainer), and Luke weigh much more
>> heavily than my own. Punting on dynamic detection of "date +%s" may be
>> perfectly acceptable with the attitude that it can be implemented
>> later if someone runs across a case where it's actually needed.
>
> Which other platforms are we talking about here?
>
> https://www.perforce.com/downloads/helix
>
> From there, you can get Solaris10, HP-UX, AIX and various flavours of BSD.
> Solaris supports "date +%s".

>>
>> Which other platforms are we talking about here?
>>
>> https://www.perforce.com/downloads/helix
>>
>> From there, you can get Solaris10, HP-UX, AIX and various flavours of BSD.
>> Solaris supports "date +%s".
>
> The question about "date +%s" portability arose with a suggestion to
> employ it in git-filter-branch[1]. Apparently, the concern about
> Solaris was raised in response to a Stack Overflow question[2] which
> claimed that +%s was not supported by that OS. Unfortunately, however,
> [2] did not indicate to which (older?) versions of Solaris that
> shortcoming applied. If Solaris 10 does support +%s, and the Perforce
> product is only available for Solaris 10, then perhaps concern about
> +%s a non-issue(?).
>
> [1]: http://git.661346.n2.nabble.com/FEATURE-REQUEST-Filter-branch-extend-progress-with-a-simple-estimated-time-remaning-td7638200.html#a7638504
> [2]: http://stackoverflow.com/questions/2445198/get-seconds-since-epoch-in-any-posix-compliant-shell

> From: Lars Schneider <larsxschneider@gmail.com>
>
> In rare cases p4d seems to hang. This watchdog will kill the p4d
> process after 300s in any case. That means each individual git p4 test
> needs to finish before 300s or it will fail.

>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>   t/lib-git-p4.sh | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index 8d6b48f..f2a009c 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -10,6 +10,10 @@ TEST_NO_CREATE_REPO=NoThanks
>   # here the maximal retry timeout in seconds.
>   RETRY_TIMEOUT=60
>
> +# Sometimes p4d seems to hang. Terminate the p4d process automatically after
> +# the defined timeout in seconds.
> +P4D_TIMEOUT=300
> +
>   . ./test-lib.sh
>
>   if ! test_have_prereq PYTHON
> @@ -85,6 +89,19 @@ start_p4d() {
>   	# will be caught with the "kill -0" check below.
>   	i=${P4D_START_PATIENCE:-300}
>   	pid=$(cat "$pidfile")
> +
> +	timeout=$(($(date +%s) + $P4D_TIMEOUT))
> +	while true
> +	do
> +		if test $(date +%s) -gt $timeout
> +		then
> +			kill -9 $pid
> +			exit 1
> +		fi
> +		sleep 1
> +	done &
> +	watchdog_pid=$!
> +
>   	ready=
>   	while test $i -gt 0
>   	do
> @@ -145,7 +162,8 @@ kill_p4d() {
>   	retry_until_fail kill -9 $pid
>   	# complain if it would not die
>   	test_must_fail kill $pid >/dev/null 2>&1 &&
> -	rm -rf "$db" "$cli" "$pidfile"
> +	rm -rf "$db" "$cli" "$pidfile" &&
> +	retry_until_fail kill -9 $watchdog_pid
>   }
>
>   cleanup_git() {
>

>
> You can reproduce the problem by commenting "P4D_TIMEOUT" in
> "lib-git-p4.sh" and running "prove ./t9800-git-p4-basic.sh".
> ---
>   t/lib-git-p4.sh | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index f2a009c..f9c68d4 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -65,6 +65,12 @@ cli="$TRASH_DIRECTORY/cli"
>   git="$TRASH_DIRECTORY/git"
>   pidfile="$TRASH_DIRECTORY/p4d.pid"
>
> +# Sometimes "prove" seems to hang on exit because p4d is still running
> +cleanup() {
> +	kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
> +}
> +trap cleanup EXIT
> +
>   # git p4 submit generates a temp file, which will
>   # not get cleaned up if the submission fails.  Don't
>   # clutter up /tmp on the test machine.
>

> On 15/11/15 13:08, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Sometimes the "prove" test runner hangs on test exit because p4d is
>> still running. Add a trap to always kill "p4d" on test exit.
> 
> With this change, I've started seeing this when running the tests:
> 
> cat: /home/lgd/git/git/t/trash directory.t9819-git-p4-case-folding/p4d.pid: No such file or directory
> 
> Probably just needs the obvious "test -f" adding.
> 
> Other than, all looks good to me. Particularly nice that I can now do:
> 
> $ make T=t98* -j10
> 
> and it actually works!

> 
> 
> 
> 
> 
>> 
>> You can reproduce the problem by commenting "P4D_TIMEOUT" in
>> "lib-git-p4.sh" and running "prove ./t9800-git-p4-basic.sh".
>> ---
>>  t/lib-git-p4.sh | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index f2a009c..f9c68d4 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -65,6 +65,12 @@ cli="$TRASH_DIRECTORY/cli"
>>  git="$TRASH_DIRECTORY/git"
>>  pidfile="$TRASH_DIRECTORY/p4d.pid"
>> 
>> +# Sometimes "prove" seems to hang on exit because p4d is still running
>> +cleanup() {
>> +	kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
>> +}
>> +trap cleanup EXIT
>> +
>>  # git p4 submit generates a temp file, which will
>>  # not get cleaned up if the submission fails.  Don't
>>  # clutter up /tmp on the test machine.
>> 
>