[3/3] tests: warn against appending non-httpd-specific tests at the end
diff mbox series

Message ID 20190801155309.15276-4-szeder.dev@gmail.com
State New
Headers show
Series
  • tests: run non-httpd-specific tests before sourcing 'lib-httpd.sh'
Related show

Commit Message

SZEDER Gábor Aug. 1, 2019, 3:53 p.m. UTC
We have a couple of test scripts that are not completely
httpd-specific, but do run a few httpd-specific tests at the end.
These test scripts source 'lib-httpd.sh' somewhere mid-script, which
then skips all the rest of the test script if the dependencies for
running httpd tests are not fulfilled.

As the previous two patches in this series show, already on two
occasions non-httpd-specific tests were appended at the end of such
test scripts, and, consequently, they were skipped as well when httpd
tests couldn't be run.

Add a comment at the end of these test scripts to warn against adding
non-httpd-specific tests at the end, in the hope that they will help
prevent similar issues in the future.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t0410-partial-clone.sh | 3 +++
 t/t5500-fetch-pack.sh    | 3 +++
 t/t5537-fetch-shallow.sh | 3 +++
 t/t5545-push-options.sh  | 3 +++
 t/t5601-clone.sh         | 3 +++
 t/t5616-partial-clone.sh | 3 +++
 t/t5700-protocol-v1.sh   | 3 +++
 t/t5702-protocol-v2.sh   | 3 +++
 8 files changed, 24 insertions(+)

Comments

SZEDER Gábor Aug. 1, 2019, 5:41 p.m. UTC | #1
On Thu, Aug 01, 2019 at 05:53:09PM +0200, SZEDER Gábor wrote:
> Subject: Re: [PATCH 3/3] tests: warn against appending non-httpd-specific
>  tests at the end

This subject line kind of sucks, doesn't it?!

Alas I had a bit of a hard time coming up with something better.  So
far the best (well, least bad) I could think of is:

  t: warn against adding non-httpd-specific tests after sourcing 'lib-httpd'


> We have a couple of test scripts that are not completely
> httpd-specific, but do run a few httpd-specific tests at the end.
> These test scripts source 'lib-httpd.sh' somewhere mid-script, which
> then skips all the rest of the test script if the dependencies for
> running httpd tests are not fulfilled.
> 
> As the previous two patches in this series show, already on two
> occasions non-httpd-specific tests were appended at the end of such
> test scripts, and, consequently, they were skipped as well when httpd
> tests couldn't be run.
> 
> Add a comment at the end of these test scripts to warn against adding
> non-httpd-specific tests at the end, in the hope that they will help
> prevent similar issues in the future.
Junio C Hamano Aug. 1, 2019, 6:18 p.m. UTC | #2
SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Thu, Aug 01, 2019 at 05:53:09PM +0200, SZEDER Gábor wrote:
>> Subject: Re: [PATCH 3/3] tests: warn against appending non-httpd-specific
>>  tests at the end
>
> This subject line kind of sucks, doesn't it?!
>
> Alas I had a bit of a hard time coming up with something better.  So
> far the best (well, least bad) I could think of is:
>
>   t: warn against adding non-httpd-specific tests after sourcing 'lib-httpd'

That reads well.

Thanks.  It must have been tedious to make sure moving tests around
won't break them due to some (hidden) inter-dependency among them.
Very much appreciated.
SZEDER Gábor Aug. 2, 2019, 10:09 a.m. UTC | #3
On Thu, Aug 01, 2019 at 11:18:42AM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > On Thu, Aug 01, 2019 at 05:53:09PM +0200, SZEDER Gábor wrote:
> >> Subject: Re: [PATCH 3/3] tests: warn against appending non-httpd-specific
> >>  tests at the end
> >
> > This subject line kind of sucks, doesn't it?!
> >
> > Alas I had a bit of a hard time coming up with something better.  So
> > far the best (well, least bad) I could think of is:
> >
> >   t: warn against adding non-httpd-specific tests after sourcing 'lib-httpd'
> 
> That reads well.

Ok.  Should I resend, or will you amend? (I see that 'pu' contains the
old subject line).

> Thanks.  It must have been tedious to make sure moving tests around
> won't break them due to some (hidden) inter-dependency among them.
> Very much appreciated.

Luckily, all the moved non-httpd tests and all the httpd tests in
't5510' and 't5703' work in their own directories, meaning that they
neither influence other tests nor are influenced by other tests, with
the exception of that $LOCAL_PRISTINE directory that I noted in the
commit message of patch 2.

I did actually diff-ed the output of the involved tests before and
after these patches, and they were essentially identical (the only
differences were that extra 'rm -rf', a couple of different
timestamps, different commit oids shown by 'git commit' or 'git
fetch', and the occasional races between the trace output and actual
command output).
Junio C Hamano Aug. 2, 2019, 4:37 p.m. UTC | #4
SZEDER Gábor <szeder.dev@gmail.com> writes:

>> >   t: warn against adding non-httpd-specific tests after sourcing 'lib-httpd'
>> 
>> That reads well.
>
> Ok.  Should I resend, or will you amend? (I see that 'pu' contains the
> old subject line).

I do not think the original was too bad, either, but I'll amend;
thanks for being extra careful.

Patch
diff mbox series

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 5bd892f2f7..6415063980 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -518,4 +518,7 @@  test_expect_success 'fetching of missing objects from an HTTP server' '
 	git verify-pack --verbose "$IDX" | grep "$HASH"
 '
 
+# 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.
+
 test_done
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 1c71c0ec77..8210f63d41 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -920,4 +920,7 @@  test_expect_success 'fetch with --filter=blob:limit=0 and HTTP' '
 	fetch_filter_blob_limit_zero "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/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.
+
 test_done
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 66f0b64d39..97a67728ca 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -255,4 +255,7 @@  test_expect_success 'shallow fetches check connectivity before writing shallow f
 	git -C client fsck
 '
 
+# 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.
+
 test_done
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 6d1d59c9b1..04b34c4de1 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -278,4 +278,7 @@  test_expect_success 'push options keep quoted characters intact (http)' '
 	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options
 '
 
+# 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.
+
 test_done
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 37d76808d4..4a3b901f06 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -739,4 +739,7 @@  test_expect_success 'partial clone using HTTP' '
 	partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/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.
+
 test_done
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index b91ef548f8..565254558f 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -417,4 +417,7 @@  test_expect_success 'tolerate server sending REF_DELTA against missing promisor
 	! test -e "$HTTPD_ROOT_PATH/one-time-sed"
 '
 
+# 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.
+
 test_done
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index 7c9511c593..2571eb90b7 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -292,4 +292,7 @@  test_expect_success 'push with http:// using protocol v1' '
 	grep "git< version 1" log
 '
 
+# 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.
+
 test_done
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 011b81d4fc..fbddd0aea9 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -723,4 +723,7 @@  test_expect_success 'when server does not send "ready", expect FLUSH' '
 	test_i18ngrep "expected no other sections to be sent after no .ready." err
 '
 
+# 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.
+
 test_done