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
