Re: [PATCH] remote-curl: add testing for intelligent retry for HTTP

> Sean McAllister <smcallis@google.com> writes:
>
> > +# generate a random 12 digit string
> > +gen_nonce() {
> > +    test_copy_bytes 12 < /dev/urandom | tr -dc A-Za-z0-9
> > +}
>
> What is the randomness requirement of this application?  IOW, what
> breaks if we just change this to "echo 0123456789ab"?
>
> Or "date | git hash-object --stdin" for that matter?
>
> We'd want to make our tests more predictiable, not less.

>
> > diff --git a/t/lib-httpd/error-ntime.sh b/t/lib-httpd/error-ntime.sh
> > new file mode 100755
> > index 0000000000..e4f91ab816
> > --- /dev/null
> > +++ b/t/lib-httpd/error-ntime.sh
> > @@ -0,0 +1,79 @@
> > +#!/bin/sh
> > +
> > +# Script to simulate a transient error code with Retry-After header set.
> > +#
> > +# PATH_INFO must be of the form /<nonce>/<times>/<retcode>/<retry-after>/<path>
> > +#   (eg: /dc724af1/3/429/10/some/url)
> > +#
> > +# The <nonce> value uniquely identifies the URL, since we're simulating
> > +# a stateful operation using a stateless protocol, we need a way to "namespace"
> > +# URLs so that they don't step on each other.
> > +#
> > +# The first <times> times this endpoint is called, it will return the given
> > +# <retcode>, and if the <retry-after> is non-negative, it will set the
> > +# Retry-After head to that value.
> > +#
> > +# Subsequent calls will return a 302 redirect to <path>.
> > +#
> > +# Supported error codes are 429,502,503, and 504
> > +
> > +print_status() {
> > +      if [ "$1" -eq "302" ]; then printf "Status: 302 Found\n"
> > +    elif [ "$1" -eq "429" ]; then printf "Status: 429 Too Many Requests\n"
> > +    elif [ "$1" -eq "502" ]; then printf "Status: 502 Bad Gateway\n"
> > +    elif [ "$1" -eq "503" ]; then printf "Status: 503 Service Unavailable\n"
> > +    elif [ "$1" -eq "504" ]; then printf "Status: 504 Gateway Timeout\n"
> > +    else
> > +        printf "Status: 500 Internal Server Error\n"
> > +    fi
> > +    printf "Content-Type: text/plain\n"
>
> Style????? (I won't repeat this comment for the rest of this script)
>
> I briefly wondered "oh, are t/lib-httpd/* scripts excempt from the
> coding guidelines?" but a quick look at them tells me that that is
> not the case.
>

> > +}
> > +
> > +# read in path split into cmoponents
> > +IFS='/'
> > +tokens=($PATH_INFO)
> > +
> > +# break out code/retry parts of path
> > +nonce=${tokens[1]}
> > +times=${tokens[2]}
> > +code=${tokens[3]}
> > +retry=${tokens[4]}
>
> You said /bin/sh upfront.  Don't use non-POSIX shell arrays.
>
> > +
> > +# get redirect path
> > +cnt=0
> > +path=""
> > +for ((ii=0; ii < ${#PATH_INFO}; ii++)); do
> > +    if [ "${PATH_INFO:${ii}:1}" == "/" ]; then
> > +        let cnt=${cnt}+1
> > +    fi
> > +    if [ "${cnt}" -eq 5 ]; then
> > +        path="${PATH_INFO:${ii}}"
> > +        break
> > +    fi
> > +done
> > +
> > +# leave a cookie for this request/retry count
> > +state_file="request_${REMOTE_ADDR}_${nonce}_${times}_${code}_${retry}"
> > +
> > +if [ ! -f "$state_file" ]; then
> > +    echo 0 > "$state_file"
> > +fi
> > +
> > +
> > +read cnt < "$state_file"
> > +if [ "$cnt" -lt "$times" ]; then
> > +    let cnt=cnt+1
> > +    echo "$cnt" > "$state_file"
> > +
> > +    # return error
> > +    print_status "$code"
> > +    if [ "$retry" -ge "0" ]; then
> > +        printf "Retry-After: %s\n" "$retry"
> > +    fi
> > +else
> > +    # redirect
> > +    print_status 302
> > +    printf "Location: %s?%s\n" "$path" "${QUERY_STRING}"
> > +fi
> > +
> > +echo
> > diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> > index 7df3c5373a..71d4307001 100755
> > --- a/t/t5601-clone.sh
> > +++ b/t/t5601-clone.sh
> > @@ -756,6 +756,15 @@ test_expect_success 'partial clone using HTTP' '
> >       partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
> >  '
> >
> > +test_expect_success 'partial clone using HTTP with redirect' '
> > +    _NONCE=`gen_nonce` && export _NONCE &&
> > +    curl "$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server" > /dev/null &&
> > +    curl "$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server" > /dev/null &&
> > +    curl "$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server" > /dev/null &&
>
> These lines are not indented with HT?
>
> Don't redirect test output to /dev/null, which is done by test_expect_success
> for us.  >/dev/null makes it less useful to run the test under "-v" option.
>

> > +     partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/error_ntime/${_NONCE}/3/502/10/smart/server"
> > +'
> > +
> > +
> >  # DO NOT add non-httpd-specific tests here, because the last part of this
> >  # test script is only executed when httpd is available and enabled.