Re: [PATCH] chainlint.pl: recognize test bodies defined via heredoc
- From
Jeff King <peff@peff.net>
- Date
- Jul 6, 2024, 06:01 UTC
- Message-ID
- <20240706060143.GD698153@coredump.intra.peff.net>
- In-Reply-To
- <20240702235034.88219-1-ericsunshine@charter.net>
On Tue, Jul 02, 2024 at 07:50:34PM -0400, Eric Sunshine wrote:
Show 7 quoted lines
> This is a clean-room implementation which serves the same purpose as a > change proposed[1] by Peff; it was created before I looked at Peff's > proposal. The two independent implementations turned out quite similar, > but the one implemented by this patch takes a more formal and paranoid > stance. In particular, unlike Peff's patch, it doesn't trust that the > most-recently-seen heredoc body is one associated with the > `test_expect_success` invocation.
Thanks for working on this! I think this is better than the patch I showed earlier. But I am still glad to have worked on that one, because there is no way I'd be able to intelligently review that one without having poked at the code so much myself.
Show 7 quoted lines
> This patch can sit either at the top or bottom of Peff's series[2]. > > There was also related discussion of improving the chainlint self-test > infrastructure[3], however, such proposed changes needn't hold up Peff's > series[2]; such improvements can be applied after the dust settles. On > the other hand, Peff, if you plan to reroll for some reason, feel free > to incorporate this patch into your series.
IMHO we want it all to come together. We should not allow "<<\EOT" without making sure we can chainlint the test bodies, and we should not make such a big change to chainlint.pl without tests to make sure it works.
I'll post some patches in a moment:
[1/3]: chainlint.pl: fix line number reporting [2/3]: t/chainlint: add test_expect_success call to test snippets [3/3]: t/chainlint: add tests for test body in heredoc
with the idea that we'd apply your patch here on top of what Junio has queued in jk/test-body-in-here-doc, and then these three on top. For Junio's sanity, I'll roll it all up into one series. But I wanted to show it to you incrementally first, especially because I think the fixes from patch 1/3 above should probably just get squashed in (or even rewritten). I'll discuss the bugs they fix below.
Show 13 quoted lines
> diff --git a/t/chainlint.pl b/t/chainlint.pl
> index 1bbd985b78..eba509b8e1 100755
> --- a/t/chainlint.pl
> +++ b/t/chainlint.pl
> @@ -174,6 +174,8 @@ sub swallow_heredocs {
> $$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}->{heredocs}->{$$tag[0]} =
> + substr($body, 0, length($body) - length($&));
> $self->{lineno} += () = $body =~ /\n/sg;
> next;
> }OK, this part looks familiar. :)
Show 10 quoted lines
> @@ -232,7 +234,8 @@ sub new {
> my $self = bless {
> buff => [],
> stop => [],
> - output => []
> + output => [],
> + heredocs => {},
> } => $class;
> $self->{lexer} = Lexer->new($self, $s);
> return $self;I think initializing is not strictly necessary here, since we'd only try to read tags if we saw a here-doc. But there might be some invalid cases where we could convince higher-level code to look for tags even though there were none (and generate a perl warning about trying to dereference undef as a hashref).
On the flip side, what about cleaning up? The "heretags" array is emptied as we parse the heredocs in swallow_heredocs(). But I think once a ShellParser's $self->{heredocs}->{FOO} is written, it will hang around forever (even though it's valid only for that one command). Probably not a big deal, but there's probably some correct spot to reset it.
Show 11 quoted lines
> @@ -616,7 +619,9 @@ sub unwrap {
>
> sub check_test {
> my $self = shift @_;
> - my ($title, $body) = map(unwrap, @_);
> + my $title = unwrap(shift @_);
> + my $body = unwrap(shift @_);
> + $body = shift @_ if $body eq '-';
> $self->{ntests}++;
> my $parser = TestParser->new(\$body);
> my @tokens = $parser->parse();This has two problems related to line numbers. You can't see it in the context, but we later do:
my $lineno = $_[1]->[3];
Now that we're shifting @_, that array item is gone.
The second is that the line number for the here-doc is actually one past the initial line number of the test_expect_success. That works automatically for hanging single-quotes, since the newline from that line is inside the quoted area. But for a here-doc, we have to account for it manually. In my original patch I prepended "\n", but you can also just increment $lineno (which is what I did in the fix I'm about to send).
Show 15 quoted lines
> @@ -649,8 +654,13 @@ sub parse_cmd {
> return @tokens unless @tokens && $tokens[0]->[0] =~ /^test_expect_(?:success|failure)$/;
> my $n = $#tokens;
> $n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\|)$/;
> - $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
> + my $herebody;
> + if ($n >= 2 && $tokens[$n-1]->[0] eq '-' && $tokens[$n]->[0] =~ /^<<-?(.+)$/) {
> + $herebody = $self->{heredocs}->{$1};
> + $n--;
> + }
> + $self->check_test($tokens[1], $tokens[2], $herebody) if $n == 2; # title body
> + $self->check_test($tokens[2], $tokens[3], $herebody) if $n > 2; # prereq title body
> return @tokens;
> }OK, mostly as expected. I think the check for "-" here is redundant with what's in check_test(). We could just feed the heredoc body either way, and in the nonsense case of:
test_expect_success 'title' 'test body' <<EOT nobody reads this! EOT
the heredoc data would just be ignored.
Requiring "<<" at the end is somewhat limiting. E.g. this is valid:
test_expect_success <<EOT 'title' - the test body EOT
I don't expect anybody to do that, but it would be nice to be more robust if we can. I think the tokens are still wrapped at this point, so we could read through all of them looking for "<<" anywhere, without getting confused by "$(cat <<INNER_HEREDOC)". I think, anyway (I didn't test).
I didn't address either of those comments in the patches I'm about to send.
-Peff