t5541: don't call start_httpd after sourcing lib-terminal.sh

Since 83d842dc8 "make test" using prove fails for some setups in t5541 with:

"Parse errors: No plan found in TAP output"

Running t5541 on its own fails with:

"error: Can't use skip_all after running some tests"

This happens because "start_httpd" (which determines if the test is to be skipped) is called after sourcing lib-terminal.sh (which sets up the terminal using test_expect_success).

Fix that by calling "start_httpd" before sourcing lib-terminal.sh.

Since recently t5541 fails for me on master and pu. I'm not sure what detail in my setup causes this breakage (I have httpd installed and it is running), but this patch fixes it for me.

One option would be to _always_ define test_terminal....

Something like the patch below (looks like we should be using $PERL_PATH instead of "perl", too).

t/lib-terminal.sh +19 -18

index 9a2dca5..55b708f 100644
@@ -1,35 +1,36 @@
 # Helpers for terminal output tests.
 
-test_expect_success PERL 'set up terminal for tests' '
+# Catch tests which should depend on TTY but forgot to. There's no need
+# to check that TTY is set here. If the test declared it and we are running
+# it, then it is set.
+test_terminal() {
+	if ! test_declared_prereq TTY
+	then
+		echo >&4 "test_terminal: need to declare TTY prerequisite"
+		return 127
+	fi
+	perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
+}
+
+test_lazy_prereq TTY '
+	test_have_prereq PERL &&
+
 	# Reading from the pty master seems to get stuck _sometimes_
 	# on Mac OS X 10.5.0, using Perl 5.10.0 or 5.8.9.
 	#
 	# Reproduction recipe: run
 	#
 	#	i=0
 	#	while ./test-terminal.perl echo hi $i
 	#	do
 	#		: $((i = $i + 1))
 	#	done
 	#
 	# After 2000 iterations or so it hangs.
 	# https://rt.cpan.org/Ticket/Display.html?id=65692
 	#
-	if test "$(uname -s)" = Darwin
-	then
-		:
-	elif
-		perl "$TEST_DIRECTORY"/test-terminal.perl \
-			sh -c "test -t 1 && test -t 2"
-	then
-		test_set_prereq TTY &&
-		test_terminal () {
-			if ! test_declared_prereq TTY
-			then
-				echo >&4 "test_terminal: need to declare TTY prerequisite"
-				return 127
-			fi
-			perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
-		}
-	fi
+	test "$(uname -s)" != Darwin &&
+
+	perl "$TEST_DIRECTORY"/test-terminal.perl \
+		sh -c "test -t 1 && test -t 2"
 '

Quoted 2 lines

Something like the patch below (looks like we should be using $PERL_PATH instead of "perl", too).

;-) Also a SP between test_terminal and (), perhaps.

Quoted 3 lines

Quoted 3 lines

Something like the patch below (looks like we should be using $PERL_PATH instead of "perl", too).

Actually, we don't need to do this, as of 94221d2 (t: use perl instead of "$PERL_PATH" where applicable, 2013-10-28). If only the author of that commit were here to correct me...

Quoted 1 line

;-) Also a SP between test_terminal and (), perhaps.

Fixed below. Here it is with a commit message.

-- >8 --
Subject: t/lib-terminal: make TTY a lazy prerequisite

When lib-terminal.sh is sourced by a test script, we immediately set up the TTY prerequisite. We do so inside a test_expect_success, because that nicely isolates any generated output.

However, this early test can interfere with a script that later wants to skip all tests (e.g., t5541 then goes on to set up the httpd server, and wants to skip_all if that fails). TAP output doesn't let us skip everything after we have already run at least one test.

We could fix this by reordering the inclusion of lib-terminal.sh in t5541 to go after the httpd setup. That solves this case, but we might eventually hit a case with circular dependencies, where either lib-*.sh include might want to skip_all after the other has run a test. So instead, let's just remove the ordering constraint entirely by doing the setup inside a test_lazy_prereq construct, rather than in a regular test. We never cared about the test outcome anyway (it was written to always succeed).

Note that in addition to setting up the prerequisite, the current test also defines test_terminal. Since we can't affect the environment from a lazy_prereq, we have to hoist that out. We previously depended on it _not_ being defined when the TTY prereq isn't set as a way to ensure that tests properly declare their dependency on TTY. However, we still cover the case (see the in-code comment for details).

Reported-by Jens Lehmann <[email protected]>

Signed-off-by Jeff King <[email protected]>

---
 t/lib-terminal.sh | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

t/lib-terminal.sh +19 -18

index 9a2dca5..5184549 100644
@@ -1,6 +1,20 @@
 # Helpers for terminal output tests.
 
-test_expect_success PERL 'set up terminal for tests' '
+# Catch tests which should depend on TTY but forgot to. There's no need
+# to aditionally check that the TTY prereq is set here.  If the test declared
+# it and we are running the test, then it must have been set.
+test_terminal () {
+	if ! test_declared_prereq TTY
+	then
+		echo >&4 "test_terminal: need to declare TTY prerequisite"
+		return 127
+	fi
+	perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
+}
+
+test_lazy_prereq TTY '
+	test_have_prereq PERL &&
+
 	# Reading from the pty master seems to get stuck _sometimes_
 	# on Mac OS X 10.5.0, using Perl 5.10.0 or 5.8.9.
 	#

@@ -15,21 +29,8 @@ test_expect_success PERL 'set up terminal for tests' '
 	# After 2000 iterations or so it hangs.
 	# https://rt.cpan.org/Ticket/Display.html?id=65692
 	#
-	if test "$(uname -s)" = Darwin
-	then
-		:
-	elif
-		perl "$TEST_DIRECTORY"/test-terminal.perl \
-			sh -c "test -t 1 && test -t 2"
-	then
-		test_set_prereq TTY &&
-		test_terminal () {
-			if ! test_declared_prereq TTY
-			then
-				echo >&4 "test_terminal: need to declare TTY prerequisite"
-				return 127
-			fi
-			perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
-		}
-	fi
+	test "$(uname -s)" != Darwin &&
+
+	perl "$TEST_DIRECTORY"/test-terminal.perl \
+		sh -c "test -t 1 && test -t 2"
 '

Quoted 2 lines · Junio C Hamano

Quoted 2 lines

Something like the patch below (looks like we should be using $PERL_PATH instead of "perl", too).

Actually, we don't need to do this, as of 94221d2 (t: use perl instead of "$PERL_PATH" where applicable, 2013-10-28). If only the author of that commit were here to correct me...

-- >8 --
Subject: t/lib-terminal: make TTY a lazy prerequisite

When lib-terminal.sh is sourced by a test script, we immediately set up the TTY prerequisite. We do so inside a test_expect_success, because that nicely isolates any generated output.

However, this early test can interfere with a script that later wants to skip all tests (e.g., t5541 then goes on to set up the httpd server, and wants to skip_all if that fails). TAP output doesn't let us skip everything after we have already run at least one test.

We could fix this by reordering the inclusion of lib-terminal.sh in t5541 to go after the httpd setup. That solves this case, but we might eventually hit a case with circular dependencies, where either lib-*.sh include might want to skip_all after the other has run a test. So instead, let's just remove the ordering constraint entirely by doing the setup inside a test_lazy_prereq construct, rather than in a regular test. We never cared about the test outcome anyway (it was written to always succeed).

Note that in addition to setting up the prerequisite, the current test also defines test_terminal. Since we can't affect the environment from a lazy_prereq, we have to hoist that out. We previously depended on it _not_ being defined when the TTY prereq isn't set as a way to ensure that tests properly declare their dependency on TTY. However, we still cover the case (see the in-code comment for details).

Reported-by Jens Lehmann <[email protected]>

Signed-off-by Jeff King <[email protected]>

---

 t/lib-terminal.sh | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

t/lib-terminal.sh +19 -18

index 9a2dca5..5184549 100644
@@ -1,6 +1,20 @@
 # Helpers for terminal output tests.
 
-test_expect_success PERL 'set up terminal for tests' '
+# Catch tests which should depend on TTY but forgot to. There's no need
+# to aditionally check that the TTY prereq is set here.  If the test declared
+# it and we are running the test, then it must have been set.
+test_terminal () {
+	if ! test_declared_prereq TTY
+	then
+		echo >&4 "test_terminal: need to declare TTY prerequisite"
+		return 127
+	fi
+	perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
+}
+
+test_lazy_prereq TTY '
+	test_have_prereq PERL &&
+
 	# Reading from the pty master seems to get stuck _sometimes_
 	# on Mac OS X 10.5.0, using Perl 5.10.0 or 5.8.9.
 	#

@@ -15,21 +29,8 @@ test_expect_success PERL 'set up terminal for tests' '
 	# After 2000 iterations or so it hangs.
 	# https://rt.cpan.org/Ticket/Display.html?id=65692
 	#
-	if test "$(uname -s)" = Darwin
-	then
-		:
-	elif
-		perl "$TEST_DIRECTORY"/test-terminal.perl \
-			sh -c "test -t 1 && test -t 2"
-	then
-		test_set_prereq TTY &&
-		test_terminal () {
-			if ! test_declared_prereq TTY
-			then
-				echo >&4 "test_terminal: need to declare TTY prerequisite"
-				return 127
-			fi
-			perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
-		}
-	fi
+	test "$(uname -s)" != Darwin &&
+
+	perl "$TEST_DIRECTORY"/test-terminal.perl \
+		sh -c "test -t 1 && test -t 2"
 '

Quoted 3 lines

Actually, we don't need to do this, as of 94221d2 (t: use perl instead of "$PERL_PATH" where applicable, 2013-10-28). If only the author of that commit were here to correct me...

Yuck. I forgot all about that, too.

I wonder if that commit (actually the one before it) invites subtle bugs by tempting us to say

	sane_unset VAR &&
	VAR=VAL perl -e 0 &&
        test "${VAR+isset}" != "isset"

Quoted 2 lines

-- >8 --
Subject: t/lib-terminal: make TTY a lazy prerequisite

[...]

Thanks.