Обсуждение: Improved TAP tests by replacing sub-optimal uses of ok() with better Test::More functions

Hi all,

I've noticed that many TAP tests in the codebase make sub-optimal use of the "ok()" function. Specifically, ok() is often used for expressions involving comparison operators or regex matches, which is not ideal because other Test::More functions provide much clearer diagnostic messages when tests fail.

For example, instead of writing:

                   ok($var =~ /foo/, "found foo")

it’s better to write:

                   like($var, /foo/, "found foo")

I experimented by modifying a TAP test in src/bin/pg_dump to deliberately fail using ok(). The failure output was quite minimal and didn’t give much detail:

# +++ tap check in src/bin/pg_dump +++
t/005_pg_dump_filterfile.pl .. 1/?
#   Failed test 'table one dumped'
#   at t/005_pg_dump_filterfile.pl line 103.
t/005_pg_dump_filterfile.pl .. 57/? # Looks like you failed 1 test of 108.
t/005_pg_dump_filterfile.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/108 subtests

Test Summary Report
-------------------
t/005_pg_dump_filterfile.pl (Wstat: 256 (exited 1) Tests: 108 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1

Then I changed the same test to use like() instead of ok(), which produced much more informative diagnostics:

# +++ tap check in src/bin/pg_dump +++
t/005_pg_dump_filterfile.pl .. 1/?
#   Failed test 'table one dumped'
#   at t/005_pg_dump_filterfile.pl line 103.
#

            #                   '--
# '
#     doesn't match '(?^m:^CREATE TABLE public1\.table_one)'
t/005_pg_dump_filterfile.pl .. 41/? # Looks like you failed 1 test of 108.
t/005_pg_dump_filterfile.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/108 subtests

Test Summary Report
-------------------
t/005_pg_dump_filterfile.pl (Wstat: 256 (exited 1) Tests: 108 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1

Based on this, I’ve replaced all such uses of ok() with the more appropriate is(), isnt(), like(), unlike(), and cmp_ok() functions, depending on the test case.

Please find the attached patch implementing these improvements...    

  '

Great, I think this is a definite improvement. I saw someone recently complaining about this overuse of ok(), so thanks for doing the work to improve it.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

Thank you for the patch! I think it can make life easier in case of
failure of affected tests. It has conflicts with the current master,
so rebase is needed.


Best regards,
Arseniy Mukhin

On Fri, Oct 10, 2025 at 09:33:10AM -0400, Andrew Dunstan wrote:
> Great, I think this is a definite improvement. I saw someone recently
> complaining about this overuse of ok(), so thanks for doing the work to
> improve it.

Yeah, it's really cool to see someone step up and do all this leg
work for the existing code.  I have not checked the patch in details
or if there are missing spots.  Andrew, is that something you are
planning to do?
--
Michael

On 2025-10-14 Tu 4:01 AM, Michael Paquier wrote:
> On Fri, Oct 10, 2025 at 09:33:10AM -0400, Andrew Dunstan wrote:
>> Great, I think this is a definite improvement. I saw someone recently
>> complaining about this overuse of ok(), so thanks for doing the work to
>> improve it.
> Yeah, it's really cool to see someone step up and do all this leg
> work for the existing code.  I have not checked the patch in details
> or if there are missing spots.  Andrew, is that something you are
> planning to do?



I believe Sadhuprasad used this recipe to find these:


   find src contrib -type f -name '*.p[lm]' -print | \
       xargs grep -P '\bok[(].*[~=]'


Maybe that would miss a few, but I bet not too many.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com