New test function replace_conf() to get rid of unlink/append pattern

Hi,

On Mon, 12 Oct 2026 at 14:49, Daniel Gustafsson <daniel@yesql.se> wrote:

We have a fairly common pattern in the TAP tests where we first unlink() a
config file and then do append_conf() on it in order to rewrite it from
scratch. There's nothing wrong with it but it's IMO not as readable as it can
be (especially when unlink and append_conf are vertically separated), and
slightly errorprone as the unlink requires one to assemble the path whereas
append_conf does that automatically (yes, I have done that exact mistake during
hacking).

To make the TAP tests more readable I propose a utility function which combines
these two operations, replace_conf(). It works just like how unlink() together
with append_conf() does but in fewer and more readable lines of code. I chose
replace rather than rewrite since it IMO more clearly signals that nothing in
the file is kept.

Thanks for the patch! It makes sense to me to have this function, makes
the tests easier to follow.

Small nit on the replace vs rewrite thing you mentioned: should we change
the below comment too to use "replacing"?

-A shortcut method for rewriting a configuration file.
+A shortcut method for replacing a configuration file.

Also, I noticed the forked-backend SKIP block in
src/test/ssl/t/004_sni.pl still skips 11 tests. Since we're removing the
ok(unlink(...)) assertion, ig that should be 10 now.

Regards,
Ayush

Hi,

On Mon, 12 Oct 2026 at 12:19, Daniel Gustafsson <daniel@yesql.se> wrote:

To make the TAP tests more readable I propose a utility function which combines
these two operations, replace_conf(). It works just like how unlink() together
with append_conf() does but in fewer and more readable lines of code. I chose
replace rather than rewrite since it IMO more clearly signals that nothing in
the file is kept.

LGTM. We might add these as well:

diff --git a/src/test/ssl/t/SSL/Server.pm b/src/test/ssl/t/SSL/Server.pm
index db70e8d5255..d0e7d29c12c 100644
--- a/src/test/ssl/t/SSL/Server.pm
+++ b/src/test/ssl/t/SSL/Server.pm
@@ -323,8 +323,7 @@ sub switch_server_cert
        my %params = @_;
        my $pgdata = $node->data_dir;

-       ok(unlink($node->data_dir . '/sslconfig.conf'));
-       $node->append_conf('sslconfig.conf', 'ssl=on');
+       $node->replace_conf('sslconfig.conf', 'ssl=on');
        $node->append_conf('sslconfig.conf',
$backend->set_server_cert(\%params));
        # use lists of ECDH curves and cipher suites for syntax testing
        $node->append_conf('sslconfig.conf', 'ssl_groups=prime256v1:secp521r1');
@@ -357,8 +356,7 @@ sub _configure_hba_for_ssl
        # but seems best to keep it as narrow as possible for security reasons.
        #
        # When connecting to certdb, also check the client certificate.
-       ok(unlink($node->data_dir . '/pg_hba.conf'));
-       $node->append_conf(
+       $node->replace_conf(
                'pg_hba.conf', <<EOF
 # TYPE  DATABASE      USER            ADDRESS       METHOD         OPTIONS
 hostssl trustdb       md5testuser     $servercidr   md5
@@ -374,8 +372,7 @@ EOF
        );

        # Also set the ident maps. Note: fields with commas must be quoted
-       ok(unlink($node->data_dir . '/pg_ident.conf'));
-       $node->append_conf(
+       $node->replace_conf(
                'pg_ident.conf', <<EOF
 # MAPNAME SYSTEM-USERNAME                                         PG-USERNAME
 dn        "CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG"    ssltestuser

I also thought if it would be better that replace_conf() accepts full
path instead of a relative path. I think what you did (relative path)
is better.

--
Regards,
Nazir Bilal Yavuz
Microsoft

On 12 Oct 2026, at 21:15, Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:

Thanks for the patch! It makes sense to me to have this function, makes
the tests easier to follow.

Thanks for review!

Small nit on the replace vs rewrite thing you mentioned: should we change
the below comment too to use "replacing"?

Fixed.

Also, I noticed the forked-backend SKIP block in
src/test/ssl/t/004_sni.pl still skips 11 tests. Since we're removing the
ok(unlink(...)) assertion, ig that should be 10 now.

Right. The skip number isn't actually used for no_plan plans which we use so
the number isn't particularly useful, but since we do specify it the number
should match so: Fixed.

On 13 Oct 2026, at 10:30, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

LGTM. We might add these as well:

diff --git a/src/test/ssl/t/SSL/Server.pm b/src/test/ssl/t/SSL/Server.pm

Ah, good catch, fixed.

--
Daniel Gustafsson

Hi,

On Thu, 22 Oct 2026 at 15:37, Daniel Gustafsson <daniel@yesql.se> wrote:

On 12 Oct 2026, at 21:15, Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:

Thanks for the patch! It makes sense to me to have this function, makes
the tests easier to follow.

Thanks for review!

Small nit on the replace vs rewrite thing you mentioned: should we change
the below comment too to use "replacing"?

Fixed.

Also, I noticed the forked-backend SKIP block in
src/test/ssl/t/004_sni.pl still skips 11 tests. Since we're removing the
ok(unlink(...)) assertion, ig that should be 10 now.

Right. The skip number isn't actually used for no_plan plans which we use so
the number isn't particularly useful, but since we do specify it the number
should match so: Fixed.

A v tiny nit: with v2 it looks like that number should be 8 now.

On 13 Oct 2026, at 10:30, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

LGTM. We might add these as well:

diff --git a/src/test/ssl/t/SSL/Server.pm b/src/test/ssl/t/SSL/Server.pm

Ah, good catch, fixed.

Thanks for updating the patch, it looks good to me.

Regards,
Ayush