From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 06 Nov 2015 09:28:03 GMT
Subject: Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout
Message-ID: <CAPig+cQiGqo2rNSC_Lw-WXq0ABp6PRFV-2w1wtK+XccQt2RD=w@mail.gmail.com>
In-Reply-To: <1446800323-2914-4-git-send-email-larsxschneider@gmail.com>

On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschneider@gmail.com> wrote:
> 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
> +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
> +}

I'm confused by this. Patch 2/4 was already calling
retry_until_fail(), but it's introduction seems to be here in patch
3/4. Am I missing something obvious?

>  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" &&
> @@ -153,8 +167,9 @@ kill_p4d() {
>  }
>
>  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() {
> --
> 2.5.1