Обсуждение: 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

On Wed, Oct 15, 2025 at 06:06:47PM +0530, Sadhuprasad Patro wrote:
> Yes.. I hv used the same you mentioned Andrew...

I have been reading the patch.

-ok( $stderr =~ /index uniqueness is violated for index "bttest_unique_idx1"/,
+like($stderr,
+    qr/index uniqueness is violated for index "bttest_unique_idx1"/,
[...]
-ok($npages >= 10, 'table has at least 10 pages');
+cmp_ok($npages, '>=', 10, 'table has at least 10 pages');
[...]
-ok($conf !~ qr/^WORK_MEM = /m, "WORK_MEM should not be configured");
+unlike($conf, qr/^WORK_MEM = /m, "WORK_MEM should not be configured");

Agreed that such replacements are clear improvements.  We have no need
to depend directly on the perl operators.  Most of the patch is made
of that.  I'll try to have a second look at these tomorrow, applying
these if there are no objections, of course.

-ok($node_s->safe_psql($db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
-    'two pre-existing publications on subscriber');
+is($node_s->safe_psql($db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
+   't',
+   'two pre-existing publications on subscriber');

Isn't this one a bug fix, actually?  Using ok() here is weird.  That
seems worth a backpatch.

-ok($node_replica->safe_psql('postgres', $canary_query) == 0,
-    'canary is missing');
+is($node_replica->safe_psql('postgres', $canary_query), 0,
+   'canary is missing');
[...]
-ok($node_replica->safe_psql('postgres', $canary_query) == 1,
+is($node_replica->safe_psql('postgres', $canary_query), 1,
     'canary is present');

These two canary queries in 042_low_level_backup.pl are also buggy
written this way, with and without the patch: safe_psql() returns a
string, these two ok() check compare it with an integer.  This part
also needs a backpatch.

-ok($node->log_contains(qr/no SSL error reported/) == 0,
+is($node->log_contains(qr/no SSL error reported/), 0,

I am skeptical that this stuff for log_contains() or safe_psql() are
better, TBH.  log_contains() is an internal routine, and we control
its internal result.  The important part is to be able to report the
contents of the logs we are trying to report on failure, so how about
introducing a new log_contains_like(), which acts as a wrapper of
like() but retrieves the contents of the node's log first, with an
optional offset?

For some of these safe_psql() patterns, perhaps  we should just let
these be.
--
Michael

On Thu, Oct 16, 2025 at 04:26:48PM +0900, Michael Paquier wrote:
> -ok($node_s->safe_psql($db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
> -    'two pre-existing publications on subscriber');
> +is($node_s->safe_psql($db1, "SELECT COUNT(*) = 2 FROM pg_publication"),
> +   't',
> +   'two pre-existing publications on subscriber');

Note here: we can just compare the result with '2'.

> -ok($node_replica->safe_psql('postgres', $canary_query) == 0,
> -    'canary is missing');
> +is($node_replica->safe_psql('postgres', $canary_query), 0,
> +   'canary is missing');
> [...]
> -ok($node_replica->safe_psql('postgres', $canary_query) == 1,
> +is($node_replica->safe_psql('postgres', $canary_query), 1,
>      'canary is present');

Fixed and backpatched these two at the end as they could mask bugs, on
the branches where they matter.

Not sure that the bits in 010_pg_basebackup were an improvement.  In
004_test_parser_perf, the result is indeed empty, but it's a matter of
the result returned by IPC::run::run.

-ok($node->log_contains(qr/no SSL error reported/) == 0,
+is($node->log_contains(qr/no SSL error reported/), 0

The CI was failing with the change in the SSL tests, as of:
[05:03:12.647] #   at
/tmp/cirrus-ci-build/src/test/ssl/t/001_ssltests.pl line 127.
[05:03:12.647] #          got: ''
[05:03:12.647] #     expected: '0'

There was already a lot of content already.  So I have left this one
out for the moment and applied a good chunk of the rest with some
indentation.
--
Michael

On Thu, Oct 30, 2025 at 05:00:27PM +0530, Sadhuprasad Patro wrote:
> On Fri, Oct 17, 2025 at 11:11 AM Michael Paquier <michael@paquier.xyz>
> wrote:
>> -ok($node->log_contains(qr/no SSL error reported/) == 0,
>> +is($node->log_contains(qr/no SSL error reported/), 0
>>
>> The CI was failing with the change in the SSL tests, as of:
>> [05:03:12.647] #   at
>> /tmp/cirrus-ci-build/src/test/ssl/t/001_ssltests.pl line 127.
>> [05:03:12.647] #          got: ''
>> [05:03:12.647] #     expected: '0'

Sadhuprasad, there was still a bit more we could do.  Related to this
issue with the SSL test, what do you think about the introduction of a
log_contains_like() in Cluster.pm where we would directly call like()
in the subroutine?  This way, we would be able to report in the output
the contents of the server logs we are trying to match (or not match)
with a pattern, making debugging easier.  What do you think?
--
Michael

On Tue, Nov 04, 2025 at 12:20:03PM +0530, Sadhuprasad Patro wrote:
> Is this looks fine to you?

I'm pretty sure that this should also be made aware of an optional
offset, so as the server logs could be compared based on a sub-set of
the logs generated, and not a whole file.

> Will share a new patch soon with this content...

Thanks!
--
Michael