[PATCH 1/3] test-terminal: give the child an empty stdin TTY

[PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Thomas Rast]

So far, test-terminal.perl did not care at all about the stdin (that
is, leave it as-is).  This mostly works well, but git-shortlog is a
problem:

* It takes decisions based on isatty(0).  (No test checks this, but
  compare 'git shortlog </dev/null' with 'git shortlog' in a
  terminal.)

* It reads all of stdin if !isatty(0) and no arguments were passed.

Because of the latter, t7006.58ff cause unexpected results if you do:

  git rev-list <range> |
  while read sha; do
    git checkout sha
    make test
  done

If t7006 runs during any 'make test' run, the next 'read sha' will
fail (git-shortlog ate all of stdin already) and the while loop stops
immediately.  In particular, this loop will only ever successfully
test *one* revision.

To fix this, change test-terminal.perl to open a third PTY for stdin,
send an EOF (Ctrl-D) immediately and close it later.  Since this may
not be portable, we use POSIX::Termios to set it to Ctrl-D.

Noticed-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/test-terminal.perl |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index ee01eb9..87b5a8c 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -4,14 +4,16 @@
 use warnings;
 use IO::Pty;
 use File::Copy;
+use POSIX ();
 
-# Run @$argv in the background with stdio redirected to $out and $err.
+# Run @$argv in the background with stdio redirected from $in and to $out and $err.
 sub start_child {
-	my ($argv, $out, $err) = @_;
+	my ($argv, $in, $out, $err) = @_;
 	my $pid = fork;
 	if (not defined $pid) {
 		die "fork failed: $!"
 	} elsif ($pid == 0) {
+		open STDIN, "<&", $in;
 		open STDOUT, ">&", $out;
 		open STDERR, ">&", $err;
 		close $out;
@@ -64,13 +66,27 @@ sub copy_stdio {
 		or exit 1;
 }
 
+sub set_default_eof_char {
+	my $fd = fileno shift;
+	my $termios = POSIX::Termios->new;
+	$termios->getattr($fd);
+	$termios->setcc(&POSIX::VEOF, 4);
+	$termios->setattr($fd, &POSIX::TCSANOW)
+		or die "Termios::setattr failed: $!";
+}
+
 if ($#ARGV < 1) {
 	die "usage: test-terminal program args";
 }
+my $master_in = new IO::Pty;
 my $master_out = new IO::Pty;
 my $master_err = new IO::Pty;
-my $pid = start_child(\@ARGV, $master_out->slave, $master_err->slave);
+set_default_eof_char($master_in->slave);
+my $pid = start_child(\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave);
+close $master_in->slave;
 close $master_out->slave;
 close $master_err->slave;
+print $master_in "\cD";
 copy_stdio($master_out, $master_err);
+close $master_in;
 exit(finish_child($pid));
-- 
1.7.8.431.g2abf2

[PATCH 2/3] test-terminal: set output terminals to raw mode

[Thomas Rast]

Not setting them to raw mode causes funny things to happen, such as
\n -> \r\n translation:

  ./test-terminal.perl echo foo | xxd
  0000000: 666f 6f0d 0a                             foo..

(Notice the added 0d.)

To avoid this, set the (pseudo)terminal to raw mode.  Note that the
IO::Pty docs recommend doing it on both master and slave.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/test-terminal.perl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 87b5a8c..a2bfbe5 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -81,6 +81,10 @@ sub set_default_eof_char {
 my $master_in = new IO::Pty;
 my $master_out = new IO::Pty;
 my $master_err = new IO::Pty;
+$master_out->set_raw();
+$master_err->set_raw();
+$master_out->slave->set_raw();
+$master_err->slave->set_raw();
 set_default_eof_char($master_in->slave);
 my $pid = start_child(\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave);
 close $master_in->slave;
-- 
1.7.8.431.g2abf2

[PATCH 3/3] t/lib-terminal: test test-terminal's sanity

[Thomas Rast]

The previous two commits show that getting test-terminal.perl right is
not trivial.  Let lib-terminal.sh run a simple test that ensures it
actually opens TTYs for std{in,out,err} and that it does not let stdin
pass through.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/lib-terminal.sh |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index 58d911d..9e4ff9c 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -33,3 +33,22 @@ test_expect_success PERL 'set up terminal for tests' '
 		}
 	fi
 '
+
+cat >expect1 <<EOF
+stdin: 1
+stdout: 1
+stderr: 1
+EOF
+: >expect2
+
+test_expect_success TTY 'test-terminal.perl is sane' '
+	test_terminal perl -e "
+		use POSIX qw(isatty);
+		print \"stdin: \", isatty(STDIN), \"\\n\";
+		print \"stdout: \", isatty(STDOUT), \"\\n\";
+		print \"stderr: \", isatty(STDERR), \"\\n\";
+	" >actual1 &&
+	test_cmp expect1 actual1 &&
+	echo foo | test_terminal cat - >actual2 &&
+	test_cmp expect2 actual2
+'
-- 
1.7.8.431.g2abf2

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Jonathan Nieder]

Hi,

Thomas Rast wrote:

> --- a/t/test-terminal.perl
> +++ b/t/test-terminal.perl
> @@ -4,14 +4,16 @@
>  use warnings;
>  use IO::Pty;
>  use File::Copy;
> +use POSIX ();
>
> -# Run @$argv in the background with stdio redirected to $out and $err.
> +# Run @$argv in the background with stdio redirected from $in and to $out and $err.

I'm not thrilled about this change.  The original purpose of
test_terminal was to test commands like "git log" that need to check
whether stdout is a tty in order to decide whether to use color and to
paginate their output.  Perhaps whether stdin is a tty _should_ affect
those decisions, but it currently doesn't (for example, "echo HEAD |
git log --stdin" works) and that would deserve a separate test, I'd
think.

The testsuite bug you mentioned sounds like a real one and worth
fixing, though.  Maybe there would be some way for test_terminal to
give the caller some control over which file descriptors to replace
with a terminal.

Just musing,
Jonathan

Re: [PATCH 2/3] test-terminal: set output terminals to raw mode

[Jonathan Nieder]

Thomas Rast wrote:

> Not setting them to raw mode causes funny things to happen, such as
> \n -> \r\n translation:
[...]
> To avoid this, set the (pseudo)terminal to raw mode.  Note that the
> IO::Pty docs recommend doing it on both master and slave.

Good idea, so for what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Does this change the behavior in
<https://rt.cpan.org/Ticket/Display.html?id=65692> (oh please oh
please)?

Re: [PATCH 3/3] t/lib-terminal: test test-terminal's sanity

[Jonathan Nieder]

Thomas Rast wrote:

> The previous two commits show that getting test-terminal.perl right is
> not trivial.  Let lib-terminal.sh run a simple test that ensures it
> actually opens TTYs for std{in,out,err} and that it does not let stdin
> pass through.

This test would get run again for each lib-terminal.sh user.  If we
want to "test the test machinery" like this, I suppose it would make
the most sense to run the check somewhere in the t00* series.

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Thomas Rast]

Jonathan Nieder wrote:
> Hi,
> 
> Thomas Rast wrote:
> > -# Run @$argv in the background with stdio redirected to $out and $err.
> > +# Run @$argv in the background with stdio redirected from $in and to $out and $err.
> 
> I'm not thrilled about this change.  The original purpose of
> test_terminal was to test commands like "git log" that need to check
> whether stdout is a tty in order to decide whether to use color and to
> paginate their output.  Perhaps whether stdin is a tty _should_ affect
> those decisions, but it currently doesn't (for example, "echo HEAD |
> git log --stdin" works) and that would deserve a separate test, I'd
> think.
> 
> The testsuite bug you mentioned sounds like a real one and worth
> fixing, though.  Maybe there would be some way for test_terminal to
> give the caller some control over which file descriptors to replace
> with a terminal.

I'm not sure I understand what you are arguing for or why.  That I
avoid wasting a Pty, and only replace stdin with /dev/null?

(Because with the current state of the tests, this shouldn't make much
of a difference.  I just figured I should go all the way and give
commands an environment that really looks like they'd been called from
the terminal.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 2/3] test-terminal: set output terminals to raw mode

[Thomas Rast]

Jonathan Nieder wrote:
> Thomas Rast wrote:
> 
> > Not setting them to raw mode causes funny things to happen, such as
> > \n -> \r\n translation:
> [...]
> > To avoid this, set the (pseudo)terminal to raw mode.  Note that the
> > IO::Pty docs recommend doing it on both master and slave.
> 
> Good idea, so for what it's worth,
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> Does this change the behavior in
> <https://rt.cpan.org/Ticket/Display.html?id=65692> (oh please oh
> please)?

I don't think so.  I tested this tweak of the script:

  perl -MIO::Pty -MFile::Copy -e '
    for (my $i = 0;; $i++) {
      my $master = new IO::Pty;
      my $slave = $master->slave;
      $master->set_raw();
      $slave->set_raw();
      if (fork == 0) {
        close $master or die "close: $!";
        open STDOUT, ">&", $slave or die "dup2: $!";
        close $slave or die "close: $!";
        exec("echo", "hi", $i) or die "exec: $!";
      }
      close $slave or die "close: $!";
      copy($master, \*STDOUT) or die "copy: $!";
      close $master or die "close: $!"; wait;
    }
  '

That's over ssh on

  $ uname -a
  Darwin mackeller.inf.ethz.ch 11.1.0 Darwin Kernel Version 11.1.0: Tue Jul 26 16:07:11 PDT 2011; root:xnu-1699.22.81~1/RELEASE_X86_64 x86_64

What's odd is that when I was logged in at university (over Gbit
ethernet, but still over ssh), the unmodified version wouldn't even
get off the ground.  I may just have been dreaming however.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Jonathan Nieder]

Thomas Rast wrote:

> I'm not sure I understand what you are arguing for or why.  That I
> avoid wasting a Pty, and only replace stdin with /dev/null?

Sorry, I was thinking narrowly about the "git log" tests in
t7006-pager.sh.  I was saying that there, the fact that
lib-terminal.sh creates an environment in which stdin is not
guaranteed to be a terminal is a feature, not a bug, since it improves
the test coverage (and I tend to find the "stdin not a tty" case more
interesting).

That said, a test for the implicit HEAD behavior of "git shortlog"
would be a welcome addition to t4201-shortlog.sh, and we might need a
helper "test_with_stdin_as_a_terminal" for that.

The tests you mentioned in t7006 are just buggy --- it was just meant
to make sure we don't regress in the change introduced by 773b69bf
("shortlog: run setup_git_directory_gently() sooner"), and I doubt I
was thinking about the "implicit HEAD when stdin is a tty" behavior at
all.  So independently of everything else, I believe we should do the
following.

-- >8 --
Subject: test: do not let "git shortlog" DWIM based on tty

In the spirit of v1.7.3.3~14^2 (t4203: do not let "git shortlog" DWIM
based on tty, 2010-10-19), do not leave out the revision argument to
"git shortlog" and rely on the default of HEAD in the usual case that
standard input is not connected to a terminal.  Otherwise, tests break
when you do

  git rev-list <range> |
  while read sha; do
    git checkout sha
    make test
  done

Reported-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7006-pager.sh |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 320e1d1d..6f05b11a 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -409,14 +409,14 @@ test_GIT_PAGER_overrides  expect_success test_must_fail 'git -p'
 
 test_doesnt_paginate      expect_failure test_must_fail 'git -p nonsense'
 
-test_pager_choices                       'git shortlog'
+test_pager_choices                       'git shortlog HEAD'
 test_expect_success 'setup: configure shortlog not to paginate' '
 	git config pager.shortlog false
 '
-test_doesnt_paginate      expect_success 'git shortlog'
-test_no_local_config_subdir expect_success 'git shortlog'
-test_default_pager        expect_success 'git -p shortlog'
-test_core_pager_subdir    expect_success 'git -p shortlog'
+test_doesnt_paginate      expect_success 'git shortlog HEAD'
+test_no_local_config_subdir expect_success 'git shortlog HEAD'
+test_default_pager        expect_success 'git -p shortlog HEAD'
+test_core_pager_subdir    expect_success 'git -p shortlog HEAD'
 
 test_core_pager_subdir    expect_success test_must_fail \
 					 'git -p apply </dev/null'
-- 
1.7.8

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> So far, test-terminal.perl did not care at all about the stdin (that
> is, leave it as-is).  This mostly works well, but git-shortlog is a
> problem:
>
> * It takes decisions based on isatty(0).  (No test checks this, but
>   compare 'git shortlog </dev/null' with 'git shortlog' in a
>   terminal.)
>
> * It reads all of stdin if !isatty(0) and no arguments were passed.
>
> Because of the latter, t7006.58ff cause unexpected results if you do:
>
>   git rev-list <range> |
>   while read sha; do
>     git checkout sha
>     make test
>   done

In the above, lack of dollar-sign in "git checkout $sha" is obvious ;-)
but I think it is a bug that you are not running make with its stdin
redirected from /dev/null in the first place.

Perhaps "make test" should do that for all tests, not just this terminal
related one? Doing it this way we do not have to worry about other tests
reading from the standard input by mistake.

-- >8 --
Subject: Do not let the tests read from standard input stream

Consider running a loop like this:

   git rev-list <range> |
   while read commit
   do
	git checkout $commit && make test || break
   done

If any of the test reads from the standard input, we may end up running a
test for just one commit (and while running the test for that commit, the
remainder of the rev-list output is consumed by the test). A workaround
would be to redirect like this:

    git checkout $commit && make test </dev/null || break

but the Makefile should be doing that for us instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index ed82320..7a85237 100644
--- a/Makefile
+++ b/Makefile
@@ -2239,7 +2239,7 @@ export NO_SVN_TESTS
 ### Testing rules
 
 test: all
-	$(MAKE) -C t/ all
+	$(MAKE) -C t/ all </dev/null
 
 test-ctype$X: ctype.o
 

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Thomas Rast]

Junio C Hamano wrote:
> 
> Perhaps "make test" should do that for all tests, not just this terminal
> related one? Doing it this way we do not have to worry about other tests
> reading from the standard input by mistake.
[...]
> --- a/Makefile
> +++ b/Makefile
> @@ -2239,7 +2239,7 @@ export NO_SVN_TESTS
>  ### Testing rules
>  
>  test: all
> -	$(MAKE) -C t/ all
> +	$(MAKE) -C t/ all </dev/null

But now you haven't fixed (in t/) 'make prove', 'make valgrind', or
./tXXXX-foo.sh.  If anything, this angle of attack should go into
test-lib.sh...

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Jeff King]

On Mon, Dec 12, 2011 at 11:07:21AM -0800, Junio C Hamano wrote:

> > Because of the latter, t7006.58ff cause unexpected results if you do:
> >
> >   git rev-list <range> |
> >   while read sha; do
> >     git checkout sha
> >     make test
> >   done
> 
> In the above, lack of dollar-sign in "git checkout $sha" is obvious ;-)
> but I think it is a bug that you are not running make with its stdin
> redirected from /dev/null in the first place.
> 
> Perhaps "make test" should do that for all tests, not just this terminal
> related one? Doing it this way we do not have to worry about other tests
> reading from the standard input by mistake.

That was my thought, as well. We want the test environment to be as
sterile and predictable as possible, so connecting stdin to /dev/null
seems like a sensible thing.

> diff --git a/Makefile b/Makefile
> index ed82320..7a85237 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2239,7 +2239,7 @@ export NO_SVN_TESTS
>  ### Testing rules
>  
>  test: all
> -	$(MAKE) -C t/ all
> +	$(MAKE) -C t/ all </dev/null

Is this right place to do it?

It doesn't catch "cd t && make".  I would expect at the least for it to
happen in t/Makefile. But I actually wonder if it should be in
test-lib.sh, as it is as much about cleaning up the test script's
environment as it is about protecting people running "make test" in a
loop. I.e., something like:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..5a38505 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -198,6 +198,9 @@ else
 	exec 4>/dev/null 3>/dev/null
 fi
 
+exec 6<&0
+exec 0</dev/null
+
 test_failure=0
 test_count=0
 test_fixed=0

One downside of that approach is that it makes it harder to insert
questionable debugging statements into test scripts. E.g., sometimes I
will temporarily throw a "gdb" or even a "bash" invocation into a test
script to investigate a failure. But that would still be possible by
redirecting from "<6".

-Peff

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Sorry, I was thinking narrowly about the "git log" tests in
> t7006-pager.sh.  I was saying that there, the fact that
> lib-terminal.sh creates an environment in which stdin is not
> guaranteed to be a terminal is a feature, not a bug, since it improves
> the test coverage (and I tend to find the "stdin not a tty" case more
> interesting).

I agree with Thomas's objective of giving ttys to all the streams of
process being tested by default to emulate the usage better, but I also
think being able to test some of the streams redirected to non-tty a good
feature to have in test-terminal driver. And I do not think these two have
to be either-or proposition.

I do not think "Run 'git log' as if the user is on an interactive
terminal, but has plugged /dev/null to its standard input" can be spelled
like this:

    test-terminal git log </dev/null

but test-terminal could learn an equivalent feature perhaps from the
command line, no?  Perhaps like

    test-terminal --stdin=/dev/null git log
    test-terminal --stdout=actual --stderr=error git shortlog

or somesuch?

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Is this right place to do it?
>
> It doesn't catch "cd t && make"....

That's just an illustration. I know my audiences who will give us real
tested patches are intelligent ;-).

And I tend to agree that at the test-lib.sh level might be a bit too low
for convenience of debugging, but it is probably a good start. An obvious
alternative would be a patch similar to the illustration patch applied to
t/Makefile.

As I said in a separate post, I think this is orthogonal to the
test-terminal patch under discussion. Being able to give the tested
programs an environment that mimics an interactive tty session better is a
good thing to do regardless of the "test should not read from make's stdin"
issue.

> happen in t/Makefile. But I actually wonder if it should be in
> test-lib.sh, as it is as much about cleaning up the test script's
> environment as it is about protecting people running "make test" in a
> loop. I.e., something like:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index bdd9513..5a38505 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -198,6 +198,9 @@ else
>  	exec 4>/dev/null 3>/dev/null
>  fi
>  
> +exec 6<&0
> +exec 0</dev/null
> +
>  test_failure=0
>  test_count=0
>  test_fixed=0
>
> One downside of that approach is that it makes it harder to insert
> questionable debugging statements into test scripts. E.g., sometimes I
> will temporarily throw a "gdb" or even a "bash" invocation into a test
> script to investigate a failure. But that would still be possible by
> redirecting from "<6".
>
> -Peff

Re: [PATCH 1/3] test-terminal: give the child an empty stdin TTY

[Jonathan Nieder]

Jeff King wrote:

> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -198,6 +198,9 @@ else
>  	exec 4>/dev/null 3>/dev/null
>  fi
>
> +exec 6<&0
> +exec 0</dev/null
> +
>  test_failure=0
>  test_count=0
>  test_fixed=0

Independently of the changes to explicitly pass HEAD to "git shortlog"
in t7006 (we should) and make test_terminal make stdin into a tty, too
(if tests still make sure "git log --stdin" launches a pager with
stdin not a tty, then it should be a fine change), this looks good to
me.

Re: [PATCH 2/3] test-terminal: set output terminals to raw mode

[Jonathan Nieder]

Thomas Rast wrote:

> I tested this tweak of the script:
[...]
> That's over ssh on
>
>   $ uname -a
>   Darwin mackeller.inf.ethz.ch 11.1.0 Darwin Kernel Version 11.1.0: Tue Jul 26 16:07:11 PDT 2011; root:xnu-1699.22.81~1/RELEASE_X86_64 x86_64

Thanks.  I've passed this information on to Apple (rdar://9046540),
though I have no reason to believe anyone reads the reports there. :)