git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jens Lehmann <Jens.Lehmann@web.de>
To: Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] t/lib-terminal: make TTY a lazy prerequisite
Date: Fri, 14 Mar 2014 23:13:01 +0100	[thread overview]
Message-ID: <53237EED.3060508@web.de> (raw)
In-Reply-To: <20140314215723.GB10299@sigill.intra.peff.net>

Am 14.03.2014 22:57, schrieb Jeff King:
> On Fri, Mar 14, 2014 at 02:47:14PM -0700, Junio C Hamano wrote:
> 
>>> 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...
> 
>> ;-)  Also a SP between test_terminal and (), perhaps.
> 
> Fixed below. Here it is with a commit message.

Thanks, this fixes the problem for 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 <Jens.Lehmann@web.de>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/lib-terminal.sh | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
> index 9a2dca5..5184549 100644
> --- a/t/lib-terminal.sh
> +++ b/t/lib-terminal.sh
> @@ -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"
>  '
> 

      parent reply	other threads:[~2014-03-14 22:13 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-14 21:18 [PATCH] t5541: don't call start_httpd after sourcing lib-terminal.sh Jens Lehmann
2014-03-14 21:37 ` Jeff King
2014-03-14 21:47   ` Junio C Hamano
2014-03-14 21:57     ` [PATCH] t/lib-terminal: make TTY a lazy prerequisite Jeff King
2014-03-14 22:05       ` Junio C Hamano
2014-03-15  1:55         ` Jeff King
2014-03-14 22:13       ` Jens Lehmann [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53237EED.3060508@web.de \
    --to=jens.lehmann@web.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).