Re: TAP output format in pg_regress

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
enumfloat8 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
enumfloat8 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
enumfloat8 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