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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 2, 2024, 21:59 UTC
Message-ID: <CAPig+cQna9nDzr9it2=VN-4_hUctG+t4=97s5h9KqmNHS=cuuw@mail.gmail.com>
In-Reply-To: <20240702211913.GB120950@coredump.intra.peff.net>

On Tue, Jul 2, 2024 at 5:19 PM Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

> On Mon, Jul 01, 2024 at 08:51:44PM -0400, Jeff King wrote:
> > 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.
>
> So doing this (with the patch I showed earlier):
>
> diff --git a/t/Makefile b/t/Makefile
> @@ -106,18 +106,28 @@ clean: clean-except-prove-cache
> +       for i in $$(grep -L "'" $(CHAINLINTTESTS_SRC)); do \
> +               echo "test_expect_success '$$i' - <<\\\\EOT" && \
> +               sed -e '/^# LINT: /d' $$i && \
> +               echo "EOT"; \
> +       done >>'$(CHAINLINTTMP_SQ)'/tests && \

Unfortunately, `grep -L` is not POSIX.

Show 9 quoted lines

> does pass. It's just running all of the tests inside an "EOT" block. But
> we have to omit ones that have single quotes in them, because they are
> making the implicit assumption that they're inside a single-quoted block
> (so they do things like '"$foo"', or '\'', etc, which behave differently
> in a here-doc).
>
> It was a nice check that the output is the same in both cases, but it's
> a bit limiting as a test suite, as there's no room to introduce test
> cases that vary the test_expect_success lines.

Agreed. It feels rather hacky and awfully special-case, as it's only (additionally) checking that the `test_expect_* title - <<EOT` form works, but doesn't help at all with testing other parsing-related behaviors of chainlint.pl (which is something I definitely wanted to be able to do when implementing the Perl version).

Show 5 quoted lines

> I'm thinking the path forward may be:
>
>   1. Move the test_expect_success wrapping lines into each
>      chainlint/*.test file. It's a little bit of extra boilerplate, but
>      it makes them a bit easier to reason about on their own.

Yes. This is exactly what I had in mind for moving forward. It's just a one-time noise-patch cost but gives us much more flexibility in terms of testing.

It also makes spot-testing the chainlint self-test files much simpler. We would be able to do this:

    ./chainlint.pl chainlint/block.test

rather than much more painful:

    { echo "test_expect_success foo '" && cat chainlint/block.test &&
echo "'"; } >dummy && ./chainlint.pl dummy; rm dummy

or something similar.

>   2. Add a few new tests that use here-docs with a few variations
>      ("<<EOT", "<<\EOT", probably a here-doc inside the test here-doc).
>
> Does that sound OK to you?

Absolutely. I'm very much in favor of these changes.