t5545: reduced test coverage

t5545: reduced test coverage

[Ramsay Jones]

Hi Junio,

I noticed, when comparing the test-suite output from v2.12.0
versus v2.13.0-rc0 on cygwin, that t5545-push-options.sh was
no longer being run. (Well, now being skipped because it can't
find a web-server). Prior to commit 438fc68462 ("push options:
pass push options to the transport helper", 08-02-2017), this
test file ran three tests on 'local remotes'.

[I used to test with apache on Linux, but during an OS upgrade
(some time ago) I decided not to install apache just for the
git test-suite. (I used to run webgit and cgit, but no more!)]

I suppose it is not a major reduction in test coverage, but I
don't recall any discussion about it and I wondered if it was
by accident or design.

ATB,
Ramsay Jones

Re: t5545: reduced test coverage

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Hi Junio,
>
> I noticed, when comparing the test-suite output from v2.12.0
> versus v2.13.0-rc0 on cygwin, that t5545-push-options.sh was
> no longer being run. (Well, now being skipped because it can't
> find a web-server). Prior to commit 438fc68462 ("push options:
> pass push options to the transport helper", 08-02-2017), this
> test file ran three tests on 'local remotes'.
>
> [I used to test with apache on Linux, but during an OS upgrade
> (some time ago) I decided not to install apache just for the
> git test-suite. (I used to run webgit and cgit, but no more!)]
>
> I suppose it is not a major reduction in test coverage, but I
> don't recall any discussion about it and I wondered if it was
> by accident or design.
>
> ATB,
> Ramsay Jones

I think it was combination of carelessness and not caring about an
incomplete platform environment well enough.  

It appears to me that only a few tests in the entire script wants to
work with HTTP server, so perhaps moving them to the end, together
with the inclusion of lib-httpd and start_httpd that 438fc684 ("push
options: pass push options to the transport helper", 2017-02-08)
introduced, may be sufficient?

Thanks.

[PATCH] t5545: enhance test coverage when no http server is installed

[Stefan Beller]

In commit 438fc68462 ("push options: pass push options to the transport
helper", 08-02-2017), the test coverage was reduced to run no tests at all
if you lack a http server.  Move the http initialization to the end,
such that only http tests are skipped when a http server is missing.

The test in between that tests submodule propagation is safe to run before
the http tests as it makes its own test directories `parent` and
`parent_upstream`.

Noticed-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t5545-push-options.sh | 85 +++++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 42 deletions(-)

diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index f9232f5d0f..90a4b0d2fe 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -3,8 +3,6 @@
 test_description='pushing to a repository using push options'
 
 . ./test-lib.sh
-. "$TEST_DIRECTORY"/lib-httpd.sh
-start_httpd
 
 mk_repo_pair () {
 	rm -rf workbench upstream &&
@@ -102,46 +100,6 @@ test_expect_success 'two push options work' '
 	test_cmp expect upstream/.git/hooks/post-receive.push_options
 '
 
-test_expect_success 'push option denied properly by http server' '
-	test_when_finished "rm -rf test_http_clone" &&
-	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
-	mk_repo_pair &&
-	git -C upstream config receive.advertisePushOptions false &&
-	git -C upstream config http.receivepack true &&
-	cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
-	git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
-	test_commit -C test_http_clone one &&
-	test_must_fail git -C test_http_clone push --push-option=asdf origin master 2>actual &&
-	test_i18ngrep "the receiving end does not support push options" actual &&
-	git -C test_http_clone push origin master
-'
-
-test_expect_success 'push options work properly across http' '
-	test_when_finished "rm -rf test_http_clone" &&
-	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
-	mk_repo_pair &&
-	git -C upstream config receive.advertisePushOptions true &&
-	git -C upstream config http.receivepack true &&
-	cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
-	git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
-
-	test_commit -C test_http_clone one &&
-	git -C test_http_clone push origin master &&
-	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
-	git -C test_http_clone rev-parse --verify master >actual &&
-	test_cmp expect actual &&
-
-	test_commit -C test_http_clone two &&
-	git -C test_http_clone push --push-option=asdf --push-option="more structured text" origin master &&
-	printf "asdf\nmore structured text\n" >expect &&
-	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options &&
-	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/post-receive.push_options &&
-
-	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
-	git -C test_http_clone rev-parse --verify master >actual &&
-	test_cmp expect actual
-'
-
 test_expect_success 'push options and submodules' '
 	test_when_finished "rm -rf parent" &&
 	test_when_finished "rm -rf parent_upstream" &&
@@ -182,6 +140,49 @@ test_expect_success 'push options and submodules' '
 	test_cmp expect parent_upstream/.git/hooks/post-receive.push_options
 '
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'push option denied properly by http server' '
+	test_when_finished "rm -rf test_http_clone" &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions false &&
+	git -C upstream config http.receivepack true &&
+	cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
+	git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
+	test_commit -C test_http_clone one &&
+	test_must_fail git -C test_http_clone push --push-option=asdf origin master 2>actual &&
+	test_i18ngrep "the receiving end does not support push options" actual &&
+	git -C test_http_clone push origin master
+'
+
+test_expect_success 'push options work properly across http' '
+	test_when_finished "rm -rf test_http_clone" &&
+	test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" &&
+	mk_repo_pair &&
+	git -C upstream config receive.advertisePushOptions true &&
+	git -C upstream config http.receivepack true &&
+	cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
+	git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
+
+	test_commit -C test_http_clone one &&
+	git -C test_http_clone push origin master &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
+	git -C test_http_clone rev-parse --verify master >actual &&
+	test_cmp expect actual &&
+
+	test_commit -C test_http_clone two &&
+	git -C test_http_clone push --push-option=asdf --push-option="more structured text" origin master &&
+	printf "asdf\nmore structured text\n" >expect &&
+	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options &&
+	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/post-receive.push_options &&
+
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git rev-parse --verify master >expect &&
+	git -C test_http_clone rev-parse --verify master >actual &&
+	test_cmp expect actual
+'
+
 stop_httpd
 
 test_done
-- 
2.13.0.18.g7d86cc8ba0

Re: t5545: reduced test coverage

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> It appears to me that only a few tests in the entire script wants to
> work with HTTP server, so perhaps moving them to the end, together
> with the inclusion of lib-httpd and start_httpd that 438fc684 ("push
> options: pass push options to the transport helper", 2017-02-08)
> introduced, may be sufficient?

Probably not, as lib-httpd, when it gives up, does the
"skip_all=message; test_done" thing, which makes test_done
to trigger this:

	# Maybe print SKIP message
	if test -n "$skip_all" && test $test_count -gt 0
	then
		error "Can't use skip_all after running some tests"
	fi

So a bit more work is needed, than just moving things around, I am
afraid...

Re: t5545: reduced test coverage

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> It appears to me that only a few tests in the entire script wants to
>> work with HTTP server, so perhaps moving them to the end, together
>> with the inclusion of lib-httpd and start_httpd that 438fc684 ("push
>> options: pass push options to the transport helper", 2017-02-08)
>> introduced, may be sufficient?
>
> Probably not, as lib-httpd, when it gives up, does the
> "skip_all=message; test_done" thing, which makes test_done
> to trigger this:
>
> 	# Maybe print SKIP message
> 	if test -n "$skip_all" && test $test_count -gt 0
> 	then
> 		error "Can't use skip_all after running some tests"
> 	fi
>
> So a bit more work is needed, than just moving things around, I am
> afraid...

And now I am visiting bf4b7219 ("test-lib.sh: Add check for invalid
use of 'skip_all' facility", 2012-09-01), which is yours ;-)

I am wondering what the fallouts would be if we did the following to
test-lib.  We used to say "1..0 # SKIP <why>" when we skip
everything, which is still kept, so that prove can notice why things
are skipped.

When we abort early but after executing any test, we used to error
out, but instead we pretend that the test finished right there, as
it seems that we are not allowed to say "1..4 # SKIP <why>" after
running 4 tests.

 t/test-lib.sh | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 13b5696822..46f29cf112 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -745,20 +745,25 @@ test_done () {
 	fi
 	case "$test_failure" in
 	0)
-		# Maybe print SKIP message
-		if test -n "$skip_all" && test $test_count -gt 0
-		then
-			error "Can't use skip_all after running some tests"
-		fi
-		test -z "$skip_all" || skip_all=" # SKIP $skip_all"
-
 		if test $test_external_has_tap -eq 0
 		then
 			if test $test_remaining -gt 0
 			then
 				say_color pass "# passed all $msg"
 			fi
-			say "1..$test_count$skip_all"
+
+			# Maybe print SKIP message
+			test -z "$skip_all" || skip_all="# SKIP $skip_all"
+			case "$test_count" in
+			0)			    
+				say "1..$test_count${skip_all:+ $skip_all}"
+				;;
+			*)
+				test -z "$skip_all" ||
+				say_color warn "$skip_all"
+				say "1..$test_count"
+				;;
+			esac
 		fi
 
 		test -d "$remove_trash" &&

Re: t5545: reduced test coverage

[Ramsay Jones]

On 17/05/17 05:23, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> It appears to me that only a few tests in the entire script wants to
>>> work with HTTP server, so perhaps moving them to the end, together
>>> with the inclusion of lib-httpd and start_httpd that 438fc684 ("push
>>> options: pass push options to the transport helper", 2017-02-08)
>>> introduced, may be sufficient?
>>
>> Probably not, as lib-httpd, when it gives up, does the
>> "skip_all=message; test_done" thing, which makes test_done
>> to trigger this:
>>
>> 	# Maybe print SKIP message
>> 	if test -n "$skip_all" && test $test_count -gt 0
>> 	then
>> 		error "Can't use skip_all after running some tests"
>> 	fi
>>
>> So a bit more work is needed, than just moving things around, I am
>> afraid...
> 
> And now I am visiting bf4b7219 ("test-lib.sh: Add check for invalid
> use of 'skip_all' facility", 2012-09-01), which is yours ;-)

Indeed. :-D

> I am wondering what the fallouts would be if we did the following to
> test-lib.  We used to say "1..0 # SKIP <why>" when we skip
> everything, which is still kept, so that prove can notice why things
> are skipped.
> 
> When we abort early but after executing any test, we used to error
> out, but instead we pretend that the test finished right there, as
> it seems that we are not allowed to say "1..4 # SKIP <why>" after
> running 4 tests.

Yes, I had this exact 'Huh?' moment back in 2012. I remember spending
quite a long time searching 'TAP specification' documents, in order to
get an answer to this. It was so long ago (and I can't find the links
to those documents now), but I have a vague recollection that prove's
behaviour is correct in allowing '1..0 <skip message>', while throwing
a fit at '1..n <any message>' (for n > 0).

I would probably have simply split the test file into two, but ...

>  t/test-lib.sh | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 13b5696822..46f29cf112 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -745,20 +745,25 @@ test_done () {
>  	fi
>  	case "$test_failure" in
>  	0)
> -		# Maybe print SKIP message
> -		if test -n "$skip_all" && test $test_count -gt 0
> -		then
> -			error "Can't use skip_all after running some tests"
> -		fi
> -		test -z "$skip_all" || skip_all=" # SKIP $skip_all"
> -
>  		if test $test_external_has_tap -eq 0
>  		then
>  			if test $test_remaining -gt 0
>  			then
>  				say_color pass "# passed all $msg"
>  			fi
> -			say "1..$test_count$skip_all"
> +
> +			# Maybe print SKIP message
> +			test -z "$skip_all" || skip_all="# SKIP $skip_all"
> +			case "$test_count" in
> +			0)			    
> +				say "1..$test_count${skip_all:+ $skip_all}"
> +				;;
> +			*)
> +				test -z "$skip_all" ||
> +				say_color warn "$skip_all"
> +				say "1..$test_count"
> +				;;
> +			esac
>  		fi
>  
>  		test -d "$remove_trash" &&

... this looks good to me. (tested on Linux and cygwin).

Thanks!

ATB,
Ramsay Jones

Re: t5545: reduced test coverage

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> I would probably have simply split the test file into two, but ...
>

Hmph, that's a thought.


> ... this looks good to me. (tested on Linux and cygwin).

Thanks.

-- >8 --
Subject: [PATCH] test: allow skipping the remainder

Because TAP output does not like to see the remainder of the test
getting skipped after running one or more tests, bf4b7219
("test-lib.sh: Add check for invalid use of 'skip_all' facility",
2012-09-01) made sure that test_done errors out when this happens.

Instead, loosen the check so that we only pretend that the rest of
the test script did not exist in such a case.  We'd lose a bit of
information (i.e. TAP does not notice that we are skipping some
tests), but not very much (i.e. TAP wasn't told how many tests are
skipped anyway).

This will allow inclusion of lib-httpd.sh in the middle of a test,
which will skip the remainder of the test scripts when tests that
involve web server are declined with GIT_TEST_HTTPD=false, for
example.

Acked-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib.sh | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 13b5696822..30eb743719 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -745,20 +745,25 @@ test_done () {
 	fi
 	case "$test_failure" in
 	0)
-		# Maybe print SKIP message
-		if test -n "$skip_all" && test $test_count -gt 0
-		then
-			error "Can't use skip_all after running some tests"
-		fi
-		test -z "$skip_all" || skip_all=" # SKIP $skip_all"
-
 		if test $test_external_has_tap -eq 0
 		then
 			if test $test_remaining -gt 0
 			then
 				say_color pass "# passed all $msg"
 			fi
-			say "1..$test_count$skip_all"
+
+			# Maybe print SKIP message
+			test -z "$skip_all" || skip_all="# SKIP $skip_all"
+			case "$test_count" in
+			0)
+				say "1..$test_count${skip_all:+ $skip_all}"
+				;;
+			*)
+				test -z "$skip_all" ||
+				say_color warn "$skip_all"
+				say "1..$test_count"
+				;;
+			esac
 		fi
 
 		test -d "$remove_trash" &&
-- 
2.13.0-427-gdfae0f5495