Re: TAP output format in pg_regress
Nikolay Shaplov <dhyan@nataraj.su>
From: Nikolay Shaplov <dhyan@nataraj.su>
To: Daniel Gustafsson <daniel@yesql.se>, pgsql-hackers@lists.postgresql.org
Cc: Andrew Dunstan <andrew@dunslane.net>, Tom Lane <tgl@sss.pgh.pa.us>,
Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
Andres Freund <andres@anarazel.de>
Date: 2022-11-10T10:44:54Z
Lists: pgsql-hackers
Hi! Do this patch still need reviewer, or reviewer field in commit-fest have
been left empty by mistake?
I am fan of TAP-tests, so I am quite happy that pg_regress output changed to
TAP tests...
I've checked new output, if is conform TAP specification. Checked that prove
consumes new pg_regress output well.
Did not found quick way to include prove TAP harness right into Makefile, so I
check dumped output, but it is not really important for now, I guess.
As for the code, I gave it a quick readthrough... And main issue I've stumbled
was here:
--------------
indent = 36 - (parallel ? 8 : 0) - floor(log10(testnumber) + 1);
status("ok %-5i %s%-*s %8.0f ms",
testnumber, (parallel ? " " : ""), indent,
testname,
runtime);
--------------
This peace of code has non-obvious logic (I can solve it without taking time
for deep thinking) and this code (almost similar) is repeated three times.
This is hard to support, and error prone solution.
I would suggest to add one _print_test_status function, that also accepts
status string and do this complex calculations and formatting only once (
"# SKIP (ignored)" should also added as %s, so we have only one print with
complex format string).
Then you will have to read check and fix it only once.
And may be it would be good to make this calculations more human-readable...
If this patch really needs reviewer and my way of thinking is acceptable,
please let me know, I will set myself as a reviewer and will dig further into
the code.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Commits
-
pg_regress: Emit TAP compliant output
- 558fff0adfa0 16.0 landed
-
Consider a failed process as a failed test in pg_regress
- 337903a16fb0 16.0 landed
-
Avoid reference to nonexistent array element in ExecInitAgg().
- 92957ed98c5c 16.0 cited