* t5545: reduced test coverage @ 2017-05-16 15:55 Ramsay Jones 2017-05-17 2:40 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Ramsay Jones @ 2017-05-16 15:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: t5545: reduced test coverage 2017-05-16 15:55 t5545: reduced test coverage Ramsay Jones @ 2017-05-17 2:40 ` Junio C Hamano 2017-05-17 3:11 ` [PATCH] t5545: enhance test coverage when no http server is installed Stefan Beller 2017-05-17 3:56 ` t5545: reduced test coverage Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: Junio C Hamano @ 2017-05-17 2:40 UTC (permalink / raw) To: Ramsay Jones; +Cc: Stefan Beller, GIT Mailing-list 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] t5545: enhance test coverage when no http server is installed 2017-05-17 2:40 ` Junio C Hamano @ 2017-05-17 3:11 ` Stefan Beller 2017-05-17 3:56 ` t5545: reduced test coverage Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Stefan Beller @ 2017-05-17 3:11 UTC (permalink / raw) To: ramsay, gitster; +Cc: git, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: t5545: reduced test coverage 2017-05-17 2:40 ` Junio C Hamano 2017-05-17 3:11 ` [PATCH] t5545: enhance test coverage when no http server is installed Stefan Beller @ 2017-05-17 3:56 ` Junio C Hamano 2017-05-17 4:23 ` Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2017-05-17 3:56 UTC (permalink / raw) To: Ramsay Jones; +Cc: Stefan Beller, GIT Mailing-list 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... ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: t5545: reduced test coverage 2017-05-17 3:56 ` t5545: reduced test coverage Junio C Hamano @ 2017-05-17 4:23 ` Junio C Hamano 2017-05-17 20:48 ` Ramsay Jones 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2017-05-17 4:23 UTC (permalink / raw) To: Ramsay Jones; +Cc: Stefan Beller, GIT Mailing-list 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" && ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: t5545: reduced test coverage 2017-05-17 4:23 ` Junio C Hamano @ 2017-05-17 20:48 ` Ramsay Jones 2017-05-18 2:52 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Ramsay Jones @ 2017-05-17 20:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: Stefan Beller, GIT Mailing-list 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: t5545: reduced test coverage 2017-05-17 20:48 ` Ramsay Jones @ 2017-05-18 2:52 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2017-05-18 2:52 UTC (permalink / raw) To: Ramsay Jones; +Cc: Stefan Beller, GIT Mailing-list 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-05-18 2:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-16 15:55 t5545: reduced test coverage Ramsay Jones 2017-05-17 2:40 ` Junio C Hamano 2017-05-17 3:11 ` [PATCH] t5545: enhance test coverage when no http server is installed Stefan Beller 2017-05-17 3:56 ` t5545: reduced test coverage Junio C Hamano 2017-05-17 4:23 ` Junio C Hamano 2017-05-17 20:48 ` Ramsay Jones 2017-05-18 2:52 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).