Feedback-ID: id4a34324:Fastmail
Date: Wed, 17 Aug 2022 15:49:49 -0700
From: Andres Freund <andres@anarazel.de>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: TAP output format in pg_regress
Message-ID: <20220817224949.jp7yftjv7ltfi2q6@awork3.anarazel.de>
References: <49A54862-C682-4B60-9155-05F178FA79DC@yesql.se>
 <20220321234924.kcc3q26o7sx6rnfa@alap3.anarazel.de>
 <E35C4A74-7265-4106-908E-5D3E5B9D8C2E@yesql.se>
 <489D2B5A-B26F-4918-BC07-B426329169E3@yesql.se>
 <5e6bc6a6-0a4e-109d-2b7d-c286dc58d609@enterprisedb.com>
 <461F8D8E-77D8-4F15-84C6-F7FD239018C0@yesql.se>
 <20220704225831.peh3a3afdgf6b5rj@awork3.anarazel.de>
 <1803389.1656986184@sss.pgh.pa.us>
 <20220705024011.nsvsfdedvridfhgc@awork3.anarazel.de>
 <2784249C-C92F-45A0-A6B4-5EE5925208FC@yesql.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2784249C-C92F-45A0-A6B4-5EE5925208FC@yesql.se>
Archived-At: 
 <https://www.postgresql.org/message-id/20220817224949.jp7yftjv7ltfi2q6%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2022-08-17 23:04:20 +0200, Daniel Gustafsson wrote:
> Attached is a new version of the pg_regress TAP patch which - per reviewer
> feedback - does away with dual output formats and just converts the existing
> output to be TAP complaint.

Cool.


> while still be TAP compliant enough that running it with prove (with a tiny
> Perl wrapper) works.

Wonder if we could utilize that for making failures of 002_pg_upgrade.pl and
027_stream_regress.pl easier to understand, by reporting pg_regress' steps as
part of the outer test. But that'd probably be at least somewhat hard, due to
the embedded test count etc.


> This version also strips away the verbose output which these days isn't
> terribly useful and mainly consume vertical space.

Yay.


> Another feature of the patch is to switch error logging to using the common
> frontend logging rather than pg_regress rolling its own.  The output from
> pg_log_error is emitted verbatim by prove as it's on stderr.

Sounds good.


> I based the support on the original TAP specification and not the new v13 or
> v14 since it seemed unclear how well those are supported in testrunners.  If
> v14 is adopted by testrunners there are some better functionality for reporting
> errors that we could use, but for how this version seems a safer option.

Yep, that makes sense.

> A normal "make check" with this patch applied now looks like this:
>
>
> +++ regress check in src/test/regress +++
> # running on port 61696 with PID 57910
> ok 1 - test_setup                       326 ms
> ok 2 - tablespace                       401 ms
> # parallel group (20 tests):  varchar char oid pg_lsn txid int4 regproc money int2 uuid float4 text name boolean int8 enum float8 bit numeric rangetypes
> ok 3 - boolean                          129 ms
> ok 4 - char                              73 ms
> ok 5 - name                             117 ms
> ok 6 - varchar                           68 ms
> <...snip...>
> ok 210 - memoize                        137 ms
> ok 211 - stats                          851 ms
> # parallel group (2 tests):  event_trigger oidjoins
> ok 212 - event_trigger                  138 ms
> not ok 213 - oidjoins                   190 ms
> ok 214 - fast_default                   149 ms
> 1..214
> # 1 of 214 tests failed.
> # The differences that caused some tests to fail can be viewed in the
> # file "/<path>/src/test/regress/regression.diffs".  A copy of the test summary that you see
> # above is saved in the file "/<path>/src/test/regress/regression.out".

I'm happy with that compared to our current output.


> The ending comment isn't currently shown when executed via prove as it has to
> go to STDERR for that to work, and I'm not sure that's something we want to do
> in the general case.  I don't expect running the pg_regress tests via prove is
> going to be the common case.  How does the meson TAP ingestion handle
> stdout/stderr?

It'll parse stdout for tap output and log stderr output separately.


> There is a slight reduction in information, for example around tests run
> serially vs in a parallel group.  This could perhaps be handled by marking the
> test name in some way like "tablespace (serial) or prefixing with a symbol or
> somerthing.  I can take a stab at that in case we think that level of detail is
> important to preserve.

I think we could just indent the test "description" for tests in parallel
groups:

I.e. instead of

ok 1 - test_setup                       326 ms
ok 2 - tablespace                       401 ms
# parallel group (20 tests):  varchar char oid pg_lsn txid int4 regproc money int2 uuid float4 text name boolean int8 enum float8 bit numeric rangetypes
ok 3 - boolean                          129 ms
ok 4 - char                              73 ms
ok 5 - name                             117 ms
ok 6 - varchar                           68 ms

do

ok 1 - test_setup                       326 ms
ok 2 - tablespace                       401 ms
# parallel group (20 tests):  varchar char oid pg_lsn txid int4 regproc money int2 uuid float4 text name boolean int8 enum float8 bit numeric rangetypes
ok 3 -         boolean                  129 ms
ok 4 -         char                      73 ms
ok 5 -         name                     117 ms
ok 6 -         varchar                   68 ms

that'd make it sufficiently clear, and is a bit more similar to the current
format?


I wonder if we should indent the test name so that the number doesn't cause
wrapping? And perhaps we can skip the - in that case?

I.e. instead of:

ok 9 - name                             117 ms
ok 10 - varchar                          68 ms
..
ok 99 - something                        60 ms
ok 100 - memoize                        137 ms

something like:

ok 9    name                            117 ms
ok 10   varchar                          68 ms
...
ok 99   something                        60 ms
ok 100  memoize                         137 ms


with parallel tests:

ok 9    name                            117 ms
# parallel group (2 tests):  varchar varchar2
ok 10           varchar                  68 ms
ok 11           varchar2                139 ms
ok 12   varchar3                         44 ms
ok 99   something                        60 ms
ok 100  memoize                         137 ms


Greetings,

Andres Freund