Re: [PATCH v3] tests: adjust whitespace in chainlint expectations

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 15, 2023, 06:24 UTC
Message-ID: <CAPig+cQozi+aiTc5Bve4OHrfuSRGUCSkKmhoYtkGTmn64Ps-rw@mail.gmail.com>
In-Reply-To: <8c3e1cb5eae13210070cc14f5f2a3f8c0dfc39c3.1702620230.git.ps@pks.im>

On Fri, Dec 15, 2023 at 1:04 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 9 quoted lines

> [...]
> Instead of improving the detection logic, fix our ".expect" files so
> that we do not need any post-processing at all anymore. This allows us
> to drop the `-w` flag when diffing so that we can always use diff(1)
> now.
>
> Note that we leave the post-processing of `chainlint.pl` output intact.
> All we do here is to strip leading line numbers that it would otherwise
> generate.

Hmm, okay, but... (see below)

> Having these would cause a rippling effect whenever we add a
> new test that sorts into the middle of existing tests and would require
> us to renumerate all subsequent lines, which seems rather pointless.

Just an aside, not strictly relevant at this time: Ævar has proposed that check-chainlint should not be creating conglomerate "test", "expect", and "actual" files, but should instead let `make` run chainlint.pl separately on each chainlint self-test file, thus benefiting from `make`'s innate parallelism rather than baking parallelism into chainlint.pl itself. More importantly, `make`'s dependency tracking would ensure that a chainlint self-test file only gets rechecked if its timestamp changes. That differs from the current situation in which _all_ of the chainlint self-test files are checked on _every_ `make test` which is wasteful if none of them have changed. Anyhow, with his proposed approach, there wouldn't be cascading line number changes just because a new self-test file was added.

Show 6 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/Makefile b/t/Makefile
> @@ -103,20 +103,12 @@ check-chainlint:
>         $(CHAINLINT) --emit-all '$(CHAINLINTTMP_SQ)'/tests | \
>                 sed -e 's/^[1-9][0-9]* //;/^[   ]*$$/d' >'$(CHAINLINTTMP_SQ)'/actual && \

The commit message claims that this is only stripping the line numbers which prefix each emitted line, but the `/^[ ]*$$/d` bit is also deleting blank lines from the output of chainlint.pl. Thus, this ought to be:

    sed -e 's/^[1-9][0-9]* //' >'$(CHAINLINTTMP_SQ)'/actual && \