Feedback-ID: i131946ab:Fastmail
Message-ID: <34735bb9-ca5e-8b3d-1ccd-c4ba252bbfa4@enterprisedb.com>
Date: Wed, 15 Mar 2023 11:36:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: TAP output format in pg_regress
Content-Language: en-US
To: Daniel Gustafsson <daniel@yesql.se>, vignesh C <vignesh21@gmail.com>
Cc: Nikolay Shaplov <dhyan@nataraj.su>, Andres Freund <andres@anarazel.de>,
 =?UTF-8?Q?Dagfinn_Ilmari_Manns=c3=a5ker?= <ilmari@ilmari.org>,
 PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
 Andrew Dunstan <andrew@dunslane.net>, Tom Lane <tgl@sss.pgh.pa.us>
References: <5e6bc6a6-0a4e-109d-2b7d-c286dc58d609@enterprisedb.com>
 <E3FDEFE1-6460-4714-BC8F-0A1693B2BED5@yesql.se>
 <20221128182848.rxcmaxtw3mclmyr4@awork3.anarazel.de>
 <5348601.u5redSiXY4@thinkpad-pgpro>
 <655E7296-24A1-4481-A992-B5CD55B7F7DB@yesql.se>
 <CALDaNm0k6iOhzwxCi16B9gHCVpHUsh_AM85ugX4KJdMTTxq8xg@mail.gmail.com>
 <CALDaNm0WtGHpU5iZNfd8nDJ-avEw+btFNJvv7qVOPLy6BZG=dw@mail.gmail.com>
 <CALDaNm21roPooMrok6u6jxPciWyeLGen9qBnavD67=gJdswNrw@mail.gmail.com>
 <4B2CBB72-9B1D-4028-9CAD-E53DF303AB8D@yesql.se>
 <0CBE542D-F333-45DE-9D6B-533CCDB13CDE@yesql.se>
 <42883A10-AA8C-4044-B559-3E741F011B4E@yesql.se>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <42883A10-AA8C-4044-B559-3E741F011B4E@yesql.se>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/34735bb9-ca5e-8b3d-1ccd-c4ba252bbfa4%40enterprisedb.com>
Precedence: bulk

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.).

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

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?

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 as a progress 
indicator.  Or just delete all of them?  I suppose some explicit 
indication about temp-instance versus existing instance would be useful.

5) About the output format.  Obviously, this will require some 
retraining of the eye.  But my first impression is that there 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

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


I think if 1-4 are addressed, this can be committed.