Re: [PATCH 6/7] tests: Add tests for automatic use of pager

7 messages, 4 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 6/7] tests: Add tests for automatic use of pager

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:19

Jonathan Nieder [off-list ref] writes:

quoted hunk

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
new file mode 100644
index 0000000..2e9cb9d
--- /dev/null
+++ b/t/t7006-pager.sh

@@ -0,0 +1,163 @@

...
+rm -f paginated.out
+git config color.ui auto
+test_expect_success TTY 'color when writing to a pager' '
+	TERM=vt100 test_terminal git log &&
+	colorful paginated.out
+'

I didn't see test_terminal defined up to this point.  Am I missing
something?

[PATCH v2 6/7] tests: Add tests for automatic use of pager

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:48:19

Git’s automatic pagination support has some subtleties.  Add some
tests to make sure we don’t break:

 - when git will use a pager by default;
 - the effect of the --paginate and --no-pager options;
 - the effect of pagination on use of color;
 - how the choice of pager is configured.

This does not yet test:

 - use of the pager by scripted commands (git svn and git am);
 - effect of the pager.* configuration variables;
 - setting of the LESS variable.

Some features involve checking whether stdout is a terminal, so many
of these tests are skipped unless output is passed through to the
terminal (i.e., unless $GIT_TEST_OPTS includes --verbose).

The immediate purpose for these tests was to avoid making things worse
after the breakage from my jn/editor-pager series (see commit 376f39,
2009-11-20).  Thanks to Sebastian Celis [off-list ref]
for the report.

Helped-by: Jeff King [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
On Sat, Feb 20, 2010 at 09:33:00AM -0800, Junio C Hamano wrote:

Jonathan Nieder [off-list ref] writes:

quoted

+	TERM=vt100 test_terminal git log &&

I didn't see test_terminal defined up to this point.  Am I missing
something?

Good catch, thanks.

 t/t7006-pager.sh |  163 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 163 insertions(+), 0 deletions(-)
 create mode 100644 t/t7006-pager.sh

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
new file mode 100644
index 0000000..4f52ea5
--- /dev/null
+++ b/t/t7006-pager.sh

@@ -0,0 +1,163 @@
+#!/bin/sh
+
+test_description='Test automatic use of a pager.'
+
+. ./test-lib.sh
+
+rm -f stdout_is_tty
+test_expect_success 'is stdout a terminal?' '
+	if test -t 1
+	then
+		: > stdout_is_tty
+	fi
+'
+
+if test -e stdout_is_tty
+then
+	test_set_prereq TTY
+else
+	say stdout is not a terminal, so skipping some tests.
+fi
+
+unset GIT_PAGER GIT_PAGER_IN_USE
+git config --unset core.pager
+PAGER='cat > paginated.out'
+export PAGER
+
+test_expect_success 'setup' '
+	test_commit initial
+'
+
+rm -f paginated.out
+test_expect_success TTY 'some commands use a pager' '
+	git log &&
+	test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'some commands do not use a pager' '
+	git rev-list HEAD &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success 'no pager when stdout is a pipe' '
+	git log | cat &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success 'no pager when stdout is a regular file' '
+	git log > file &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'git --paginate rev-list uses a pager' '
+	git --paginate rev-list HEAD  &&
+	test -e paginated.out
+'
+
+rm -f file paginated.out
+test_expect_success 'no pager even with --paginate when stdout is a pipe' '
+	git --paginate log | cat &&
+	! test -e paginated.out
+'
+
+rm -f paginated.out
+test_expect_success TTY 'no pager with --no-pager' '
+	git --no-pager log &&
+	! test -e paginated.out
+'
+
+# A colored commit log will begin with an appropriate ANSI escape
+# for the first color; the text "commit" comes later.
+colorful() {
+	read firstline < $1
+	! expr "$firstline" : "^[a-zA-Z]" >/dev/null
+}
+
+rm -f colorful.log colorless.log
+test_expect_success 'tests can detect color' '
+	git log --no-color > colorless.log &&
+	git log --color > colorful.log &&
+	! colorful colorless.log &&
+	colorful colorful.log
+'
+
+rm -f colorless.log
+git config color.ui auto
+test_expect_success 'no color when stdout is a regular file' '
+	git log > colorless.log &&
+	! colorful colorless.log
+'
+
+rm -f paginated.out
+git config color.ui auto
+test_expect_success TTY 'color when writing to a pager' '
+	TERM=vt100 git log &&
+	colorful paginated.out
+'
+
+rm -f colorful.log
+git config color.ui auto
+test_expect_success 'color when writing to a file intended for a pager' '
+	TERM=vt100 GIT_PAGER_IN_USE=true git log > colorful.log &&
+	colorful colorful.log
+'
+
+unset PAGER GIT_PAGER
+git config --unset core.pager
+test_expect_success 'determine default pager' '
+	less=$(git var GIT_PAGER) &&
+	test -n "$less"
+'
+
+if expr "$less" : '^[a-z]*$' > /dev/null && test_have_prereq TTY
+then
+	test_set_prereq SIMPLEPAGER
+fi
+
+unset PAGER GIT_PAGER
+git config --unset core.pager
+rm -f default_pager_used
+test_expect_success SIMPLEPAGER 'default pager is used by default' '
+	cat > $less <<-EOF &&
+	#!$SHELL_PATH
+	: > default_pager_used
+	EOF
+	chmod +x $less &&
+	PATH=.:$PATH git log &&
+	test -e default_pager_used
+'
+
+unset GIT_PAGER
+git config --unset core.pager
+rm -f PAGER_used
+test_expect_success TTY 'PAGER overrides default pager' '
+	PAGER=": > PAGER_used" &&
+	export PAGER &&
+	git log &&
+	test -e PAGER_used
+'
+
+unset GIT_PAGER
+rm -f core.pager_used
+test_expect_success TTY 'core.pager overrides PAGER' '
+	PAGER=: &&
+	export PAGER &&
+	git config core.pager ": > core.pager_used" &&
+	git log &&
+	test -e core.pager_used
+'
+
+rm -f GIT_PAGER_used
+test_expect_success TTY 'GIT_PAGER overrides core.pager' '
+	git config core.pager : &&
+	GIT_PAGER=": > GIT_PAGER_used" &&
+	export GIT_PAGER &&
+	git log &&
+	test -e GIT_PAGER_used
+'
+
+test_done

-- 
1.7.0

[PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:48:19

Testing pagination requires (fake or real) access to a terminal so we
can see whether the pagination automatically kicks in, which makes it
hard to get good coverage when running tests without --verbose.  There
are a number of ways to work around that:

 - Replace all isatty calls with calls to a custom xisatty wrapper
   that usually checks for a terminal but can be overridden for tests.
   This would be workable, but it would require implementing xisatty
   separately in three languages (C, shell, and perl) and making sure
   that any code that is to be tested always uses the wrapper.

 - Redirect stdout to /dev/tty.  This would be problematic because
   there might be no terminal available, and even if a terminal is
   available, it might not be appropriate to spew output to it.

 - Create a new pseudo-terminal on the fly and capture its output.

This patch implements the third approach.

The new test-terminal.perl helper uses IPC::Open3 and IO::Pty from
Expect.pm to create a terminal and execute the program specified by
its arguments with that terminal as stdout.  If either module is
missing or not working on a system, the test script will maintain its
old behavior (skipping most of its tests unless GIT_TEST_OPTS includes
--verbose).

It also uses File::Copy, which has been part of core Perl since
version 5.002 (1996).

Signed-off-by: Jonathan Nieder <redacted>
---
Here’s test-terminal on top of that.  As long as this using IO::Pty
from CPAN, there’s not much reason not to use the often-used
IPC::Open3 to simplify this.

The nice thing about using a script for this is that it fails
gracefully and doesn’t affect the makefile.

Thanks for bearing with me.

 t/t7006-pager.sh           |   35 ++++++++++++++++++++++++-----------
 t/t7006/test-terminal.perl |   28 ++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 11 deletions(-)
 create mode 100755 t/t7006/test-terminal.perl

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4f52ea5..da0f962 100644
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh

@@ -5,18 +5,31 @@ test_description='Test automatic use of a pager.'
 . ./test-lib.sh
 
 rm -f stdout_is_tty
-test_expect_success 'is stdout a terminal?' '
+test_expect_success 'set up terminal for tests' '
 	if test -t 1
 	then
 		: > stdout_is_tty
+	elif
+		test_have_prereq PERL &&
+		"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl \
+			sh -c "test -t 1"
+	then
+		: > test_terminal_works
 	fi
 '
 
 if test -e stdout_is_tty
 then
+	test_terminal() { "$@"; }
+	test_set_prereq TTY
+elif test -e test_terminal_works
+then
+	test_terminal() {
+		"$PERL_PATH" "$TEST_DIRECTORY"/t7006/test-terminal.perl "$@"
+	}
 	test_set_prereq TTY
 else
-	say stdout is not a terminal, so skipping some tests.
+	say no usable terminal, so skipping some tests
 fi
 
 unset GIT_PAGER GIT_PAGER_IN_USE

@@ -30,13 +43,13 @@ test_expect_success 'setup' '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands use a pager' '
-	git log &&
+	test_terminal git log &&
 	test -e paginated.out
 '
 
 rm -f paginated.out
 test_expect_success TTY 'some commands do not use a pager' '
-	git rev-list HEAD &&
+	test_terminal git rev-list HEAD &&
 	! test -e paginated.out
 '

@@ -54,7 +67,7 @@ test_expect_success 'no pager when stdout is a regular file' '
 
 rm -f paginated.out
 test_expect_success TTY 'git --paginate rev-list uses a pager' '
-	git --paginate rev-list HEAD  &&
+	test_terminal git --paginate rev-list HEAD &&
 	test -e paginated.out
 '

@@ -66,7 +79,7 @@ test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 
 rm -f paginated.out
 test_expect_success TTY 'no pager with --no-pager' '
-	git --no-pager log &&
+	test_terminal git --no-pager log &&
 	! test -e paginated.out
 '

@@ -95,7 +108,7 @@ test_expect_success 'no color when stdout is a regular file' '
 rm -f paginated.out
 git config color.ui auto
 test_expect_success TTY 'color when writing to a pager' '
-	TERM=vt100 git log &&
+	TERM=vt100 test_terminal git log &&
 	colorful paginated.out
 '

@@ -127,7 +140,7 @@ test_expect_success SIMPLEPAGER 'default pager is used by default' '
 	: > default_pager_used
 	EOF
 	chmod +x $less &&
-	PATH=.:$PATH git log &&
+	PATH=.:$PATH test_terminal git log &&
 	test -e default_pager_used
 '

@@ -137,7 +150,7 @@ rm -f PAGER_used
 test_expect_success TTY 'PAGER overrides default pager' '
 	PAGER=": > PAGER_used" &&
 	export PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e PAGER_used
 '

@@ -147,7 +160,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 	PAGER=: &&
 	export PAGER &&
 	git config core.pager ": > core.pager_used" &&
-	git log &&
+	test_terminal git log &&
 	test -e core.pager_used
 '

@@ -156,7 +169,7 @@ test_expect_success TTY 'GIT_PAGER overrides core.pager' '
 	git config core.pager : &&
 	GIT_PAGER=": > GIT_PAGER_used" &&
 	export GIT_PAGER &&
-	git log &&
+	test_terminal git log &&
 	test -e GIT_PAGER_used
 '

diff --git a/t/t7006/test-terminal.perl b/t/t7006/test-terminal.perl
new file mode 100755
index 0000000..fc3fbb4
--- /dev/null
+++ b/t/t7006/test-terminal.perl

@@ -0,0 +1,28 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+use IO::Pty;
+use File::Copy;
+use IPC::Open3;
+
+# Wait for $_[0] to finish.
+sub finish_child {
+	# Simplified from wait_or_whine() in run-command.c.
+	waitpid($_[0], 0) == $_[0] or die "waitpid failed: $!";
+	if ($? & 127) {
+		my $code = $? & 127;
+		warn "died of signal $code";
+		return $code - 128;
+	}
+	return $? >> 8;
+}
+
+@ARGV >= 1 or die "usage: test-terminal program args";
+my $master = new IO::Pty;
+my $slave = $master->slave;
+my $pid = open3(\*STDIN, '>&' . fileno($slave), \*STDERR, @ARGV);
+close $slave;
+# Reads from a disconnected terminal may return EIO or end-of-file.
+# Either is fine.
+copy($master, \*STDOUT, 4096) or $!{EIO} or die "cannot copy from child: $!";
+exit(finish_child($pid));

-- 
1.7.0

Re: [PATCH v4 7/7] t7006-pager: if stdout is not a terminal, make a new one

From: Jeff King <hidden>
Date: 2016-06-15 22:48:19

On Sat, Feb 20, 2010 at 08:09:22PM -0600, Jonathan Nieder wrote:

Testing pagination requires (fake or real) access to a terminal so we
can see whether the pagination automatically kicks in, which makes it
hard to get good coverage when running tests without --verbose.  There
are a number of ways to work around that:

 - Replace all isatty calls with calls to a custom xisatty wrapper
   that usually checks for a terminal but can be overridden for tests.
   This would be workable, but it would require implementing xisatty
   separately in three languages (C, shell, and perl) and making sure
   that any code that is to be tested always uses the wrapper.

 - Redirect stdout to /dev/tty.  This would be problematic because
   there might be no terminal available, and even if a terminal is
   available, it might not be appropriate to spew output to it.

 - Create a new pseudo-terminal on the fly and capture its output.

This patch implements the third approach.

Just to wrap up my end of this patch discussion, I think the approach
you take here is the sanest one. While it would be nice to get test
coverage on every system, I don't think it is worth the effort of
trying to write portable terminal creation code. And this way at least
the code in git is fairly minimal.

So looks good to me.

-Peff

Re: [PATCH v2 6/7] tests: Add tests for automatic use of pager

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:48:19

Don't the following pager settings suffer from a race condition?

+	cat > $less <<-EOF &&
+	#!$SHELL_PATH
+	: > default_pager_used
+	EOF

+	PAGER=": > PAGER_used" &&

+	PAGER=: &&

+	git config core.pager ": > core.pager_used" &&

+	git config core.pager : &&
+	GIT_PAGER=": > GIT_PAGER_used" &&

They depend on that the subsequent 'git log' writes to the pipe (and does
not fill it) before the pager can run. If the pager runs first and
completes, and then 'git log' writes to the pipe, it will die from
SIGPIPE, won't it?

I suggest to rewrite all ':' to something that processes stdin, eg. 'wc'
(not 'cat', because this is a magic token).

-- Hannes

Re: [PATCH 8/7] tests: Fix race condition in t7006-pager

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:48:19

Jonathan Nieder wrote:

Johannes Sixt wrote:

quoted

Don't the following pager settings suffer from a race condition?

Yes.  Thanks for noticing.

And to save the reader time wondering: yes, the following examples
suffer from the same race, but if they are used then we _want_ the
test to fail.  It might make sense to squash them in anyway to make
the patch more self-explanatory.

There is also an instance of PAGER='cat > paginated.out'; since the
pager string only starts with but does not equal "cat", this works
fine.  It needed for test ‘12: color when writing to a pager’ to be
able to read the paginated output.

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index ec6fd06..d9202d5 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh

@@ -157,7 +157,7 @@ test_expect_success TTY 'PAGER overrides default pager' '
 unset GIT_PAGER
 rm -f core.pager_used
 test_expect_success TTY 'core.pager overrides PAGER' '
-	PAGER=: &&
+	PAGER=wc &&
 	export PAGER &&
 	git config core.pager "wc > core.pager_used" &&
 	test_terminal git log &&

@@ -166,7 +166,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 
 rm -f GIT_PAGER_used
 test_expect_success TTY 'GIT_PAGER overrides core.pager' '
-	git config core.pager : &&
+	git config core.pager wc &&
 	GIT_PAGER="wc > GIT_PAGER_used" &&
 	export GIT_PAGER &&
 	test_terminal git log &&

[PATCH 8/7] tests: Fix race condition in t7006-pager

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:48:19

Pagers that do not consume their input are dangerous: for example,

 $ GIT_PAGER=: git log
 $ echo $?
 141
 $

The only reason these tests were able to work before was that
'git log' would write to the pipe (and not fill it) before the
pager had time to terminate and close the pipe.

Fix it by using a program that consumes its input, namely wc (as
suggested by Johannes).

Reported-by: Johannes Sixt <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Johannes Sixt wrote:

Don't the following pager settings suffer from a race condition?

Yes.  Thanks for noticing.

 t/t7006-pager.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index da0f962..ec6fd06 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh

@@ -137,7 +137,7 @@ rm -f default_pager_used
 test_expect_success SIMPLEPAGER 'default pager is used by default' '
 	cat > $less <<-EOF &&
 	#!$SHELL_PATH
-	: > default_pager_used
+	wc > default_pager_used
 	EOF
 	chmod +x $less &&
 	PATH=.:$PATH test_terminal git log &&

@@ -148,7 +148,7 @@ unset GIT_PAGER
 git config --unset core.pager
 rm -f PAGER_used
 test_expect_success TTY 'PAGER overrides default pager' '
-	PAGER=": > PAGER_used" &&
+	PAGER="wc > PAGER_used" &&
 	export PAGER &&
 	test_terminal git log &&
 	test -e PAGER_used

@@ -159,7 +159,7 @@ rm -f core.pager_used
 test_expect_success TTY 'core.pager overrides PAGER' '
 	PAGER=: &&
 	export PAGER &&
-	git config core.pager ": > core.pager_used" &&
+	git config core.pager "wc > core.pager_used" &&
 	test_terminal git log &&
 	test -e core.pager_used
 '

@@ -167,7 +167,7 @@ test_expect_success TTY 'core.pager overrides PAGER' '
 rm -f GIT_PAGER_used
 test_expect_success TTY 'GIT_PAGER overrides core.pager' '
 	git config core.pager : &&
-	GIT_PAGER=": > GIT_PAGER_used" &&
+	GIT_PAGER="wc > GIT_PAGER_used" &&
 	export GIT_PAGER &&
 	test_terminal git log &&
 	test -e GIT_PAGER_used

-- 
1.7.0

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help