Re: meson: Make test output much more useful on failure (both in CI and locally)

On Mon, 26 Jan 2026 at 17:42, Andres Freund <andres(at)anarazel(dot)de> wrote:
> This doesn't really seem meson specific, right? It just seems about the output
> of our own test tooling?

Yes, it's general improvements. But the problem is worse in meson than
for because that really only shows the TAP output. The Makefile based
builds will show a bit more information currently.

> I think we'd need to limit the size of the output here somewhat. If e.g. the
> server crashes, you end up with all subsequent tests failing and printing out
> a couple hundred kB of pointless diff output.

Fair enough.

> I'm a bit confused. Why are we rerunning diffs and appending to the diff file?

Now I'm confused. We're running the same amount of diffs as before.
Did you miss the removal of the /* Run diff */ below the block that
you quoted?

> I'm afraid that the creation of this many additional tempfiles would slow down
> the tests on some platforms...
< snip>
> > @@ -972,8 +986,22 @@ sub command_fails
> > {
> > local $Test::Builder::Level = $Test::Builder::Level + 1;
> > my ($cmd, $test_name) = @_;
> > - my $result = run_log($cmd);
> > - ok(!$result, $test_name);
> > + # Doesn't rely on detecting end of file on the file descriptors,
> > + # which can fail, causing the process to hang, notably on Msys
> > + # when used with 'pg_ctl start'
>
> Huh? Any more details?

I copy pasted that comment and the pipe to tempfiles from
command_like_safe. I did this after I initially tried to pipe to
variables, but that made the test that we run in SanityCheck get stuck
forever.

I don't get why this is needed either.

> > + my $stdoutfile = File::Temp->new();
> > + my $stderrfile = File::Temp->new();
> > + my $result = IPC::Run::run $cmd, '>' => $stdoutfile, '2>' => $stderrfile;
> > + ok(!$result, $test_name) or do
> > + {
> > + my $stdout = slurp_file($stdoutfile);
> > + my $stderr = slurp_file($stderrfile);
> > + diag("-- command succeeded unexpectedly --");
> > + diag(join(" ", @$cmd));
> > + diag("-------------- stdout --------------"), diag($stdout) if $stdout;
> > + diag("-------------- stderr --------------"), diag($stderr) if $stderr;
> > + diag("------------------------------------");
> > + };
>
> I don't think it makes sense to duplicate the logic for this multiple times.

It seemed a bit much to make it generic over the input, as it would
need to remove dashes accordingly. Especially since it only has two
copies.

> This looked familiar - turns out I had complained about this in the past and
> then forgotten about it :(
>
> https://www.postgresql.org/message-id/20220222181924.eehi7o4pmneeb4hm%40alap3.anarazel.de
>
> At least at the time I found it necessary to separately test for $^S, to avoid
> triggering the logic for syntax errors. Not sure why I concluded that though.
>
> In a reply Andrew suggested to use done_testing(), which avoids the pointless
> "Tests were run but no plan was declared and done_testing() was not seen."
> which does seem like an improvement.

I'll take a look at doing that.

> I think this is too localized a fix. Right now every die in a .pm file has
> this issue. The easiest fix would probably be to just replace all uses of die
> in .pm files with croak (which we already, inconsistently, use).

I didn't even know that function existed (due to my pretty much
non-existent knowlegde of perl).