Re: [PATCH v2 5/8] test_terminal: give priority to test-terminal.perl usage

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 17, 2010, 00:38 UTC
Message-ID: <20101017003807.GF20883@burratino>
In-Reply-To: <1287254223-4496-6-git-send-email-rctay89@gmail.com>

Tay Ray Chuan wrote:

Show 11 quoted lines

>  - use the test_terminal script even when running with "-v"
>    if IO::Pty is available, to allow commands like
> 
> 	test_terminal foo >out 2>err
> 
>  - add a separate TTYREDIR prerequisite which is only set
>    when the test_terminal script is usable
> 
>  - write the "need to declare TTY prerequisite" message to fd 4,
>    where it will be printed when running tests with -v, rather
>    than being swallowed up by an unrelated redireciton.

The patches up to this one look good to me. This one behaves as advertised, but I think the API is lousy --- it is just begging people to use the TTY prereq where TTYREDIR is needed.

Better to change TTY to mean TTYREDIR and drop support for test_terminal on systems without IO::Pty:

-- 8< --
Subject: test_terminal: ensure redirections work reliably

For terminal tests that capture output/stderr, the TTY prerequisite warning does not quite work for commands like

	test_terminal foo >out 2>err

because the warning gets "swallowed" up by the redirection that's supposed only to be done by the subcommand.

Even worse, the outcome depends on whether stdout was already a terminal (in which case test_terminal is a noop) or not (in which case test_terminal introduces a pseudo-tty in the middle of the pipeline).

	$ test_terminal.perl sh -c 'test -t 1 && echo >&2 YES' >out
	YES
	$ sh -c 'test -t 1 && echo >&2 YES' >out
	$

So:

 - use the test_terminal script even when running with "-v".

 - skip tests that require a terminal when the test_terminal
   script is unusable because IO::Pty is not installed.

 - write the "need to declare TTY prerequisite" message to fd 4,
   where it will be printed when running tests with -v, rather
   than being swallowed up by an unrelated redireciton.

Noticed-by: Tay Ray Chuan <rctay89@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The only other sane alternative I can think of is to introduce
TTYNOREDIR, since at least people wouldn't be tempted to use
that.  Distinguishing between

	test_expect_success 'foo' '
		test_terminal bar >out 2>err
	'

and

	test_expect_success 'foo' '
		test_terminal bar
	'

from a script run as

	sh t1234-some-script.sh >log 2>err.log

does not seem to be easy without OS-specific hacks like "readlink /dev/fd/1".

 t/lib-terminal.sh |   38 ++++++++++----------------------------
 1 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 5e7ee9a..c383b57 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -1,37 +1,19 @@
 #!/bin/sh
 
 test_expect_success 'set up terminal for tests' '
-	if test -t 1 && test -t 2
-	then
-		>have_tty
-	elif
+	if
 		test_have_prereq PERL &&
 		"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl \
 			sh -c "test -t 1 && test -t 2"
 	then
-		>test_terminal_works
+		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_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
+		}
 	fi
 '
-
-if test -e have_tty
-then
-	test_terminal_() { "$@"; }
-	test_set_prereq TTY
-elif test -e test_terminal_works
-then
-	test_terminal_() {
-		"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
-	}
-	test_set_prereq TTY
-else
-	say "# no usable terminal, so skipping some tests"
-fi
-
-test_terminal () {
-	if ! test_declared_prereq TTY
-	then
-		echo >&2 'test_terminal: need to declare TTY prerequisite'
-		return 127
-	fi
-	test_terminal_ "$@"
-}

-- 
1.7.2.3