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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]

On Fri, Dec 15, 2023 at 01:24:20AM -0500, Eric Sunshine wrote:
> On Fri, Dec 15, 2023 at 1:04 AM Patrick Steinhardt <ps@pks.im> wrote:
> > [...]
> > 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.

I was indeed also thinking along this way and would tend to agree. I
punted on it as I honestly only really care for fixing the immediate
issue that the post-processing causes for me.

Are you fine with deferring this bigger refactoring?

> > 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 && \

Gah, you're right, I missed the second part. Will fix in another round.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]