diff mbox series

[1/3] t5510-fetch: run non-httpd-specific test before sourcing 'lib-httpd.sh'

Message ID 20190801155309.15276-2-szeder.dev@gmail.com (mailing list archive)
State New, archived
Headers show
Series tests: run non-httpd-specific tests before sourcing 'lib-httpd.sh' | expand

Commit Message

SZEDER Gábor Aug. 1, 2019, 3:53 p.m. UTC 
't5510-fetch.sh' sources 'lib-httpd.sh' near the end to run a
httpd-specific test, but 'lib-httpd.sh' skips all the rest of the test
script if the dependencies for running httpd tests are not fulfilled.
Alas, recently cdbd70c437 (fetch: add --[no-]show-forced-updates
argument, 2019-06-18) appended a non-httpd-specific test at the end,
and this test is then skipped as well when httpd tests can't be run.

Move this new test earlier in the test script, before 'lib-httpd.sh'
is sourced, so it will be run even when httpd tests aren't.

Also add a comment at the end of this test script to warn against
adding non-httpd-specific tests at the end, in the hope that it will
help prevent similar issues in the future.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5510-fetch.sh | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

Comments

Derrick Stolee Aug. 1, 2019, 5:51 p.m. UTC | #1 
On 8/1/2019 11:53 AM, SZEDER Gábor wrote:
> 't5510-fetch.sh' sources 'lib-httpd.sh' near the end to run a
> httpd-specific test, but 'lib-httpd.sh' skips all the rest of the test
> script if the dependencies for running httpd tests are not fulfilled.
> Alas, recently cdbd70c437 (fetch: add --[no-]show-forced-updates
> argument, 2019-06-18) appended a non-httpd-specific test at the end,
> and this test is then skipped as well when httpd tests can't be run.
> 
> Move this new test earlier in the test script, before 'lib-httpd.sh'
> is sourced, so it will be run even when httpd tests aren't.
> 
> Also add a comment at the end of this test script to warn against
> adding non-httpd-specific tests at the end, in the hope that it will
> help prevent similar issues in the future.

I appreciate this comment! Not only will it help authors, it will help
reviewers who only see a slice of the test file during review.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index f2481de577..34b486f1a4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -902,6 +902,29 @@  test_expect_success C_LOCALE_OUTPUT 'fetch compact output' '
 	test_cmp expect actual
 '
 
+test_expect_success '--no-show-forced-updates' '
+	mkdir forced-updates &&
+	(
+		cd forced-updates &&
+		git init &&
+		test_commit 1 &&
+		test_commit 2
+	) &&
+	git clone forced-updates forced-update-clone &&
+	git clone forced-updates no-forced-update-clone &&
+	git -C forced-updates reset --hard HEAD~1 &&
+	(
+		cd forced-update-clone &&
+		git fetch --show-forced-updates origin 2>output &&
+		test_i18ngrep "(forced update)" output
+	) &&
+	(
+		cd no-forced-update-clone &&
+		git fetch --no-show-forced-updates origin 2>output &&
+		test_i18ngrep ! "(forced update)" output
+	)
+'
+
 setup_negotiation_tip () {
 	SERVER="$1"
 	URL="$2"
@@ -978,27 +1001,7 @@  test_expect_success '--negotiation-tip limits "have" lines sent with HTTP protoc
 	check_negotiation_tip
 '
 
-test_expect_success '--no-show-forced-updates' '
-	mkdir forced-updates &&
-	(
-		cd forced-updates &&
-		git init &&
-		test_commit 1 &&
-		test_commit 2
-	) &&
-	git clone forced-updates forced-update-clone &&
-	git clone forced-updates no-forced-update-clone &&
-	git -C forced-updates reset --hard HEAD~1 &&
-	(
-		cd forced-update-clone &&
-		git fetch --show-forced-updates origin 2>output &&
-		test_i18ngrep "(forced update)" output
-	) &&
-	(
-		cd no-forced-update-clone &&
-		git fetch --no-show-forced-updates origin 2>output &&
-		test_i18ngrep ! "(forced update)" output
-	)
-'
+# 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