Re: TAP output format in pg_regress

> On 15 Mar 2023, at 11:36, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> On 24.02.23 10:49, Daniel Gustafsson wrote:
>> Another rebase on top of 337903a16f.  Unless there are conflicting reviews, I
>> consider this patch to be done and ready for going in during the next CF.
>
> I think this is just about as good as it's going to get, so I think we can consider committing this soon.
>
> A few comments along the way:
>
> 1) We can remove the gettext markers _() inside calls like note(), bail(), etc.  If we really wanted to do
translation,we would do that inside those functions (similar to errmsg() etc.). 

Fixed.

The attached also removes all explicit \n from output and leaves the decision
on when to add a linebreak to the TAP emitting function.  I think this better
match how we typically handle printing of output like this.  It also ensures
that all bail messages follow the same syntax.

> 2) There are a few fprintf(stderr, ...) calls left.  Should those be changed to something TAP-enabled?

Initially the patch kept errors happening before testing started a non-TAP
output, there were leftovers which are now converted.

> 3) Maybe these lines
>
> +++ isolation check in src/test/isolation +++
>
> should be changed to TAP format?  Arguably, if I run "make -s check", then everything printed should be valid TAP,
right?

Fixed.

> 4) From the introduction lines
>
> ============== creating temporary instance            ==============
> ============== initializing database system           ==============
> ============== starting postmaster                    ==============
> running on port 61696 with PID 85346
> ============== creating database "regression"         ==============
> ============== running regression test queries        ==============
>
> you have kept
>
> # running on port 61696 with PID 85346
>
> which, well, is that the most interesting of those?
>
> The first three lines (creating, initializing, starting) take some noticeable amount of time, so they could be kept
asa progress indicator.  Or just delete all of them?  I suppose some explicit indication about temp-instance versus
existinginstance would be useful. 

This was discussed in 20220221164736.rq3ornzjdkmwk2wo@alap3.anarazel.de which
concluded that this was about the only thing of interest, and even at that it
was more of a maybe than a definite yes.

As this patch stands, it prints the above for a temp install and the host/port
for an existing install, but I don't have ny problems removing that as well.

> 5) About the output format.  Obviously, this will require some retraining of the eye.  But my first impression is
thatthere is a lot of whitespace without any guiding lines, so to speak, horizontally or vertically.  It makes me a bit
dizzy.;-) 
>
> I think instead
>
> # parallel group (2 tests):  event_trigger oidjoins
> ok 210              event_trigger                      131 ms
> ok 211              oidjoins                           190 ms
> ok 212       fast_default                              158 ms
> ok 213       tablespace                                319 ms
>
> Something like this would be less dizzy-making:
>
> # parallel group (2 tests):  event_trigger oidjoins
> ok 210 - + event_trigger                               131 ms
> ok 211 - + oidjoins                                    190 ms
> ok 212 - fast_default                                  158 ms
> ok 213 - tablespace                                    319 ms

The current format is chosen to be close to the old format, while also adding
sufficient padding that it won't yield ragged columns.  The wide padding is
needed to cope with long names in the isolation and ecgp test suites and not so
much regress suite.

In the attached I've dialled back the padding a little bit to make it a bit
more compact, but I doubt it's enough.

> Just an idea, we don't have to get this exactly perfect right now, but it's something to think about.

I'm quite convinced that this will be revisited once this lands and is in front
of developers.

I think the attached is a good candidate for going in, but I would like to see it
for another spin in the CF bot first.

--
Daniel Gustafsson