Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve TAP tests by replacing ok() with better Test::More functions

  2. Fix matching check in recovery test 042_low_level_backup

  3. pg_createsubscriber: Fix matching check in TAP test

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

    Sadhuprasad Patro <b.sadhu@gmail.com> — 2025-10-10T05:52:55Z

    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...

Thanks for considering the change.
Regards,
SadhuPrasad.
EnterpriseDB.

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

    Andrew Dunstan <andrew@dunslane.net> — 2025-10-10T13:33:10Z

    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> .. 1/?
> #   Failed test 'table one dumped'
> #   at t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl> 
> line 103.
> t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl> .. 57/? 
> # Looks like you failed 1 test of 108.
> t/005_pg_dump_filterfile.pl <http://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 <http://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 <http://005_pg_dump_filterfile.pl> .. 1/?
> #   Failed test 'table one dumped'
> #   at t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl> 
> line 103.
> #
>             #                   '--
> # '
> */#     doesn't match '(?^m:^CREATE TABLE public1\.table_one)'/*
> t/005_pg_dump_filterfile.pl <http://005_pg_dump_filterfile.pl> .. 41/? 
> # Looks like you failed 1 test of 108.
> t/005_pg_dump_filterfile.pl <http://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 <http://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

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

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-10-14T07:02:29Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2025-10-14T08:01:14Z

    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

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

    Andrew Dunstan <andrew@dunslane.net> — 2025-10-14T20:55:44Z

    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

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

    Sadhuprasad Patro <b.sadhu@gmail.com> — 2025-10-15T12:36:47Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2025-10-16T07:26:48Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2025-10-17T05:41:07Z

    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

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

    Sadhuprasad Patro <b.sadhu@gmail.com> — 2025-10-30T11:30:27Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2025-10-31T00:48:48Z

    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

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

    Sadhuprasad Patro <b.sadhu@gmail.com> — 2025-11-04T06:50:03Z

    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.

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

    Michael Paquier <michael@paquier.xyz> — 2025-11-04T07:03:53Z

    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