From: Jeff King <peff@peff.net>
Date: Tue, 02 Jul 2024 00:51:44 GMT
Subject: Re: [PATCH 1/2] test-lib: allow test snippets as here-docs
Message-ID: <20240702005144.GA27170@coredump.intra.peff.net>
In-Reply-To: <CAPig+cQdioTBinkacLFb7yktm168oB+qyP9MpZtTRE=1aBz5yg@mail.gmail.com>

On Mon, Jul 01, 2024 at 06:45:19PM -0400, Eric Sunshine wrote:

> > @@ -906,6 +906,14 @@ see test-lib-functions.sh for the full list and their options.
> > +   If <script> is `-` (a single dash), then the script to run is read
> > +   from stdin. This lets you more easily use single quotes within the
> > +   script by using a here-doc. For example:
> > +
> > +       test_expect_success 'output contains expected string' - <<\EOT
> > +               grep "this string has 'quotes' in it" output
> > +       EOT
> 
> We lose `chainlint` functionality for test bodies specified in this manner.
> 
> Restoring such functionality will require some (possibly)
> not-so-subtle changes. There are at least a couple issues which need
> to be addressed:
> 
> (1) chainlint.pl:ScriptParser::parse_cmd() only currently recognizes
> `test_expect_* [prereq] 'title' 'body'` but will now also need to
> recognize `test_expect_success [prereq] 'title' - <body-as-here-doc>`.
> 
> (2) Until now, chainlint.pl has never had to concern itself with the
> body of a here-doc; it just throws them away. With this new calling
> convention, here-doc bodies become relevant and must be returned by
> the lexer. This may involve some not-so-minor surgery.

Hmm. The patch below seems to work on a simple test.

The lexer stuffs the heredoc into a special variable. Which at first
glance feels like a hack versus returning it from the token stream, but
the contents really _aren't_ part of that stream. They're a separate
magic thing that is found on the stdin of whatever command the tokens
represent.

And then ScriptParser::parse_cmd() just has to recognize that any "<<"
token isn't interesting, and that "-" means "read the here-doc".

Obviously we'd want to add to the chainlint tests here. It looks like
the current test infrastructure is focused on evaluating snippets, with
the test_expect_success part already handled.

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 1bbd985b78..7eb904afaa 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -168,12 +168,15 @@ sub swallow_heredocs {
 	my $self = shift @_;
 	my $b = $self->{buff};
 	my $tags = $self->{heretags};
+	$self->{parser}->{heredoc} = '';
 	while (my $tag = shift @$tags) {
 		my $start = pos($$b);
 		my $indent = $$tag[0] =~ s/^\t// ? '\\s*' : '';
 		$$b =~ /(?:\G|\n)$indent\Q$$tag[0]\E(?:\n|\z)/gc;
 		if (pos($$b) > $start) {
 			my $body = substr($$b, $start, pos($$b) - $start);
+			$self->{parser}->{heredoc} .=
+				substr($body, 0, length($body) - length($&));
 			$self->{lineno} += () = $body =~ /\n/sg;
 			next;
 		}
@@ -618,6 +621,9 @@ sub check_test {
 	my $self = shift @_;
 	my ($title, $body) = map(unwrap, @_);
 	$self->{ntests}++;
+	if ($body eq '-') {
+		$body = $self->{heredoc};
+	}
 	my $parser = TestParser->new(\$body);
 	my @tokens = $parser->parse();
 	my $problems = $parser->{problems};
@@ -648,7 +654,7 @@ sub parse_cmd {
 	my @tokens = $self->SUPER::parse_cmd();
 	return @tokens unless @tokens && $tokens[0]->[0] =~ /^test_expect_(?:success|failure)$/;
 	my $n = $#tokens;
-	$n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\|)$/;
+	$n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\||<<[A-Za-z]+)$/;
 	$self->check_test($tokens[1], $tokens[2]) if $n == 2; # title body
 	$self->check_test($tokens[2], $tokens[3]) if $n > 2;  # prereq title body
 	return @tokens;

-Peff