Re: [PATCH] t5562: skip if NO_CURL is enabled

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, max@max630.net, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] t5562: skip if NO_CURL is enabled
Date: Mon, 19 Nov 2018 11:42:55 +0100	[thread overview]
Message-ID: <87a7m51h74.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20181119101535.16538-1-carenas@gmail.com>

On Mon, Nov 19 2018, Carlo Marcelo Arenas Belón wrote:

> 6c213e863a ("http-backend: respect CONTENT_LENGTH for receive-pack", 2018-07-27)
> introduced all tests but without a check for CURL support from git.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/t5562-http-backend-content-length.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> index b24d8b05a4..7594899471 100755
> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -3,6 +3,12 @@
>  test_description='test git-http-backend respects CONTENT_LENGTH'
>  . ./test-lib.sh

This seems like the wrong fix for whatever bug you're encountering. I
just built with NO_CURL and:

    $ ./t5561-http-backend.sh
    1..0 # SKIP skipping test, git built without http support
    $ ./t5562-http-backend-content-length.sh
    ok 1 - setup
    ok 2 - setup, compression related
    ok 3 - fetch plain
    ok 4 - fetch plain truncated
    ok 5 - fetch plain empty
    ok 6 - fetch gzipped
    ok 7 - fetch gzipped truncated
    ok 8 - fetch gzipped empty
    ok 9 - push plain
    ok 10 - push plain truncated
    ok 11 - push plain empty
    ok 12 - push gzipped
    ok 13 - push gzipped truncated
    ok 14 - push gzipped empty
    ok 15 - CONTENT_LENGTH overflow ssite_t
    ok 16 - empty CONTENT_LENGTH
    # passed all 16 test(s)
    1..16

So all these test pass.

Of courses I still have curl on my system, but I don't see the curl(1)
utility used in the test, and my git at this point can't operate on
https?:// URLs, so what error are you getting? Can you paste the test
output with -x -v?

> +if test -n "$NO_CURL"
> +then
> +	skip_all='skipping test, git built without http support'
> +	test_done
> +fi
> +
>  test_lazy_prereq GZIP 'gzip --version'
>
>  verify_http_result() {

If we do end up needing this after all it seems better to do something
like:

    diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
    index a8729f8232..adad654277 100644
    --- a/t/lib-httpd.sh
    +++ b/t/lib-httpd.sh
    @@ -30,11 +30,7 @@
     # Copyright (c) 2008 Clemens Buchacher <drizzd@aon.at>
     #

    -if test -n "$NO_CURL"
    -then
    -       skip_all='skipping test, git built without http support'
    -       test_done
    -fi
    +. "$TEST_DIRECTORY"/lib-no-curl.sh

     if test -n "$NO_EXPAT" && test -n "$LIB_HTTPD_DAV"
     then
    diff --git a/t/lib-no-curl.sh b/t/lib-no-curl.sh
    new file mode 100644
    index 0000000000..014947aa2d
    --- /dev/null
    +++ b/t/lib-no-curl.sh
    @@ -0,0 +1,5 @@
    +if test -n "$NO_CURL"
    +then
    +       skip_all='skipping test, git built without http support'
    +       test_done
    +fi
    diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
    index b24d8b05a4..cffb460673 100755
    --- a/t/t5562-http-backend-content-length.sh
    +++ b/t/t5562-http-backend-content-length.sh
    @@ -2,6 +2,7 @@

     test_description='test git-http-backend respects CONTENT_LENGTH'
     . ./test-lib.sh
    +. ./lib-no-curl.sh

     test_lazy_prereq GZIP 'gzip --version'

Not really a problem with your patch, we have lots of this copy/pasting
all over the place already. I.e. stuff like:

    if test -n "$X"
    then
    	skip_all="$Y"
    	test_done
    fi

or:

    if ! test_have_prereq "$X"
    then
    	skip_all="$Y"
    	test_done
    fi

Maybe we should make more use of test_lazy_prereq and factor all that
into a new helper like:

    test_have_prereq_or_skip_all "$X" "$Y"

Which could be put at the top of these various tests...