Improved TAP tests by replacing sub-optimal uses of ok() with better Test::More functions

On 2025-10-10 Fr 1:52 AM, Sadhuprasad Patro wrote:

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 <http://005_pg_dump_filterfile.pl&gt; .. 1/?
#   Failed test 'table one dumped'
#   at t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl&gt;
line 103.
t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl&gt; .. 57/?
# Looks like you failed 1 test of 108.
t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl&gt; ..
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/108 subtests

Test Summary Report
-------------------
t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl&gt; (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 <http://005_pg_dump_filterfile.pl&gt; .. 1/?
#   Failed test 'table one dumped'
#   at t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl&gt;
line 103.
#
            #                   '--
# '
*/#     doesn't match '(?^m:^CREATE TABLE public1\.table_one)'/*
t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl&gt; .. 41/?
# Looks like you failed 1 test of 108.
t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl&gt; ..
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/108 subtests

Test Summary Report
-------------------
t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl&gt; (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 2:25 AM Andrew Dunstan <andrew@dunslane.net> wrote:

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

Yes.. I hv used the same you mentioned Andrew...

--
thank u
SADHU PRASAD

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

Thank you Michael for committing the patch...

On Fri, Oct 17, 2025 at 11:11 AM Michael Paquier <michael@paquier.xyz>
wrote:

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

--
thank u
SADHU PRASAD

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 Fri, Oct 31, 2025 at 6:19 AM Michael Paquier <michael@paquier.xyz> wrote:

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

Hi Michael,

I think we can do this as you suggested...

I can define something like below in perl script and try to use:

*sub log_contains_like { my ($self, $pattern, $msg, $do_test) = @_;
my $log = $self->get_log(); $msg //= "Log output matches pattern"; #
Run as test by default if (!defined $do_test || $do_test) { my
$ok = like($log, $pattern, $msg); # If the test failed, show a
concise log snippet unless ($ok) { my @lines = split
/\n/, $log; my $snippet;*

* $snippet = $log; diag("Log snippet
(last " . scalar(@lines > 20 ? 20 : @lines) . " lines):");*

* diag($snippet); } } else { # Return
boolean for manual usage return ($log =~ /$pattern/); }}*

Is this looks fine to you?
Will share a new patch soon with this content...

Thank you
SadhuPrasad,
EnterpriseDB.

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