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

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