Re: [PATCH 1/2] test-lib: allow test snippets as here-docs

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 2, 2024, 21:25 UTC
Message-ID: <CAPig+cQ6PLZA=s6D1XsdcFeeg-=ffib9QZGFLycsHWLZZ1ibCg@mail.gmail.com>
In-Reply-To: <20240702005144.GA27170@coredump.intra.peff.net>

On Mon, Jul 1, 2024 at 8:51 PM Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

> On Mon, Jul 01, 2024 at 06:45:19PM -0400, Eric Sunshine wrote:
> > We lose `chainlint` functionality for test bodies specified in this manner.
>
> 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.

I created a white-room fix for this issue, as well, before taking a look at your patch. The two implementations bear a strong similarity which suggests that we agree upon the basic approach.

My implementation, however, takes a more formal and paranoid stance. Rather than squirreling away only the most-recently-seen heredoc body, it stores each heredoc body along with the tag which introduced it. This makes it robust against cases when multiple heredocs are initiated on the same line (even within different parse contexts):

    cat <<EOFA && x=$(cat <<EOFB &&
    A body
    EOFA
    B body
    EOFB

Of course, that's not likely to come up in the context of test_expect_* calls, but I prefer the added robustness over the more lax approach.

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

In my implementation, the `<<` token is "interesting" because the heredoc tag is attached to it, and the tag is needed to pluck the heredoc body from the set of saved bodies (since my implementation doesn't assume most-recently-seen body is the correct one).

> 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.

Yes, the "snippet" approach is a throwback to the old chainlint.sed implementation when there wasn't any actual parsing going on. As you note, this unfortunately does not allow for testing parsing-related aspects of the implementation, which is a limitation I most definitely felt when chainlint.pl was implemented. It probably would be a good idea to update the infrastructure to allow for more broad testing but that doesn't need to be part of the changes being discussed here.

Show 7 quoted lines

> diff --git a/t/chainlint.pl b/t/chainlint.pl
> @@ -168,12 +168,15 @@ sub swallow_heredocs {
>                 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;

In my implementation, I use regex to strip off the ending tag before storing the heredoc body. When I later looked at your implementation, I noticed that you used substr() -- which seems preferable -- but discovered that it strips too much in some cases. For instance, in t0600, I saw that:

    cat >expected <<-\EOF &&
    HEAD
    PSEUDO_WT_HEAD
    refs/bisect/wt-random
    refs/heads/main
    refs/heads/wt-main
    EOF

was getting stripped down to:

    HEAD
    PSEUDO_WT_HEAD
    refs/bisect/wt-random
    refs/heads/main
    refs/heads/wt-ma{{missing-nl}}

It wasn't immediately obvious why this was happening, though I didn't spend a lot of time trying to debug it.

Although I think my implementation is complete, I haven't submitted it yet because I discovered that the changes you made to t1404 are triggering false-positives:

    # chainlint: t1404-update-ref-errors.sh
    # chainlint: existing loose ref is a simple prefix of new
    120 prefix=refs/1l &&
    121 test_update_rejected a c e false b c/x d \
    122   '$prefix/c' exists; ?!AMP?! cannot create '$prefix/c/x'

Unfortunately, I ran out of time, thus haven't tracked down this problem yet. I also haven't tested your implementation yet to determine if this is due to a change I made or due to a deeper existing issue with chainlint.pl.