Hi,
I am working on a feature adjacent to the connection service functionality
and noticed some issues with the tests introduced in this thread. Basically
they incorrectly invoke the append perl function by passing multiple
strings to append when the function only takes one string to append. This
caused the generated service files to not actually contain any connection
parameters. The tests were only passing because the connect_ok perl
function set the connection parameters as environment variables which
covered up the misformed connection service file.
The attached patch is much more strict in that it creates a dummy database
that is not started and passes all queries though that and tests that the
connection service file correctly overrides the environment variables set
by the dummy databases' query functions
Thanks,
Andrew Jackson
On Mon, Mar 31, 2025, 4:01 PM Ryo Kanbayashi <kanbayashi....@gmail.com>
wrote:
> On Sat, Mar 22, 2025 at 4:46 PM Michael Paquier <mich...@paquier.xyz>
> wrote:
> >
> > On Thu, Mar 20, 2025 at 06:16:44PM +0900, Ryo Kanbayashi wrote:
> > > Sorry, I found a miss on 006_service.pl.
> > > Fixed patch is attached...
> >
> > Please note that the commit fest app needs all the patches of a a set
> > to be posted in the same message. In this case, v2-0001 is not going
> > to get automatic test coverage.
> >
> > Your patch naming policy is also a bit confusing. I would suggest to
> > use `git format-patch -vN -2`, where N is your version number. 0001
> > would be the new tests for service files, and 0002 the new feature,
> > with its own tests.
>
> All right.
> I attached patches generated with your suggested command :)
>
> > +if ($windows_os) {
> > +
> > + # Windows: use CRLF
> > + print $fh "[my_srv]", "\r\n";
> > + print $fh join( "\r\n", split( ' ', $node->connstr ) ), "\r\n";
> > +}
> > +else {
> > + # Non-Windows: use LF
> > + print $fh "[my_srv]", "\n";
> > + print $fh join( "\n", split( ' ', $node->connstr ) ), "\n";
> > +}
> > +close $fh;
> >
> > That's duplicated. Let's perhaps use a $newline variable and print
> > into the file using the $newline?
>
> OK.
> I reflected above comment.
>
> > Question: you are doing things this way in the test because fgets() is
> > what is used by libpq to retrieve the lines of the service file, is
> > that right?
>
> No. I'm doing above way simply because line ending code of service file
> wrote by users may become CRLF in Windows platform.
>
> > Please note that the CI is failing. It seems to me that you are
> > missing a done_testing() at the end of the script. If you have a
> > github account, I'd suggest to set up a CI in your own fork of
> > Postgres, this is really helpful to double-check the correctness of a
> > patch before posting it to the lists, and saves in round trips between
> > author and reviewer. Please see src/tools/ci/README in the code tree
> > for details.
>
> Sorry.
> I'm using Cirrus CI with GitHub and I checked passing the CI.
> But there were misses when I created patch files...
>
> > +# Copyright (c) 2023-2024, PostgreSQL Global Development Group
> >
> > These dates are incorrect. Should be 2025, as it's a new file.
>
> OK.
>
> > +++ b/src/interfaces/libpq/t/007_servicefile_opt.pl
> > @@ -0,0 +1,100 @@
> > +# Copyright (c) 2023-2024, PostgreSQL Global Development Group
> >
> > Incorrect date again in the second path with the new feature. I'd
> > suggest to merge all the tests in a single script, with only one node
> > initialized and started.
>
> OK.
> Additional test scripts have been merged to a single script ^^ b
>
> ---
> Great regards,
> Ryo Kanbayashi
>
From 0d732ee8edbb16132b95f35775388528fa9003d8 Mon Sep 17 00:00:00 2001
From: AndrewJackson <AndrewJackson947@gmail.com>
Date: Mon, 31 Mar 2025 15:18:48 -0500
Subject: [PATCH] libpq: Fix TAP tests for service files and names
This commit builds on the tests that were added in a prior commit
that tests the connection service file functionality. The tests are
fixed to correctly invoke the append_to_file subroutine which only
takes one argument and not multiple. The test also runs the statements
through a dummy database to ensure that the options from the service
file are actually being picked up and just passing because they are
defaulting to the connection environment variables that are set in
the connect_ok function.
Author: Andrew Jackson <AndrewJackson947@gmail.com>
---
src/interfaces/libpq/t/006_service.pl | 38 ++++++++++++++++++---------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl
index d3ecfa6b6fc..e0d18d72359 100644
--- a/src/interfaces/libpq/t/006_service.pl
+++ b/src/interfaces/libpq/t/006_service.pl
@@ -9,6 +9,9 @@ use Test::More;
# This tests scenarios related to the service name and the service file,
# for the connection options and their environment variables.
+my $dummy_node = PostgreSQL::Test::Cluster->new('dummy_node');
+$dummy_node->init;
+
my $node = PostgreSQL::Test::Cluster->new('node');
$node->init;
$node->start;
@@ -23,8 +26,16 @@ my $newline = $windows_os ? "\r\n" : "\n";
# File that includes a valid service name, that uses a decomposed connection
# string for its contents, split on spaces.
my $srvfile_valid = "$td/pg_service_valid.conf";
-append_to_file($srvfile_valid, "[my_srv]", $newline);
-append_to_file($srvfile_valid, split(/\s+/, $node->connstr) . $newline);
+append_to_file($srvfile_valid, "[my_srv]");
+append_to_file($srvfile_valid, $newline);
+
+append_to_file($srvfile_valid, "host=");
+append_to_file($srvfile_valid, $node->host);
+append_to_file($srvfile_valid, $newline);
+
+append_to_file($srvfile_valid, "port=");
+append_to_file($srvfile_valid, $node->port);
+append_to_file($srvfile_valid, $newline);
# File defined with no contents, used as default value for PGSERVICEFILE,
# so as no lookup is attempted in the user's home directory.
@@ -51,33 +62,33 @@ local $ENV{PGSERVICEFILE} = "$srvfile_empty";
# Checks combinations of service name and a valid service file.
{
local $ENV{PGSERVICEFILE} = $srvfile_valid;
- $node->connect_ok(
+ $dummy_node->connect_ok(
'service=my_srv',
'connection with correct "service" string and PGSERVICEFILE',
sql => "SELECT 'connect1_1'",
expected_stdout => qr/connect1_1/);
- $node->connect_ok(
+ $dummy_node->connect_ok(
'postgres://?service=my_srv',
'connection with correct "service" URI and PGSERVICEFILE',
sql => "SELECT 'connect1_2'",
expected_stdout => qr/connect1_2/);
- $node->connect_fails(
+ $dummy_node->connect_fails(
'service=undefined-service',
'connection with incorrect "service" string and PGSERVICEFILE',
expected_stderr =>
qr/definition of service "undefined-service" not found/);
local $ENV{PGSERVICE} = 'my_srv';
- $node->connect_ok(
+ $dummy_node->connect_ok(
'',
'connection with correct PGSERVICE and PGSERVICEFILE',
sql => "SELECT 'connect1_3'",
expected_stdout => qr/connect1_3/);
local $ENV{PGSERVICE} = 'undefined-service';
- $node->connect_fails(
+ $dummy_node->connect_fails(
'',
'connection with incorrect PGSERVICE and PGSERVICEFILE',
expected_stdout =>
@@ -87,7 +98,7 @@ local $ENV{PGSERVICEFILE} = "$srvfile_empty";
# Checks case of incorrect service file.
{
local $ENV{PGSERVICEFILE} = $srvfile_missing;
- $node->connect_fails(
+ $dummy_node->connect_fails(
'service=my_srv',
'connection with correct "service" string and incorrect PGSERVICEFILE',
expected_stderr =>
@@ -100,33 +111,33 @@ local $ENV{PGSERVICEFILE} = "$srvfile_empty";
my $srvfile_default = "$td/pg_service.conf";
copy($srvfile_valid, $srvfile_default);
- $node->connect_ok(
+ $dummy_node->connect_ok(
'service=my_srv',
'connection with correct "service" string and pg_service.conf',
sql => "SELECT 'connect2_1'",
expected_stdout => qr/connect2_1/);
- $node->connect_ok(
+ $dummy_node->connect_ok(
'postgres://?service=my_srv',
'connection with correct "service" URI and default pg_service.conf',
sql => "SELECT 'connect2_2'",
expected_stdout => qr/connect2_2/);
- $node->connect_fails(
+ $dummy_node->connect_fails(
'service=undefined-service',
'connection with incorrect "service" string and default pg_service.conf',
expected_stderr =>
qr/definition of service "undefined-service" not found/);
local $ENV{PGSERVICE} = 'my_srv';
- $node->connect_ok(
+ $dummy_node->connect_ok(
'',
'connection with correct PGSERVICE and default pg_service.conf',
sql => "SELECT 'connect2_3'",
expected_stdout => qr/connect2_3/);
local $ENV{PGSERVICE} = 'undefined-service';
- $node->connect_fails(
+ $dummy_node->connect_fails(
'',
'connection with incorrect PGSERVICE and default pg_service.conf',
expected_stdout =>
@@ -137,5 +148,6 @@ local $ENV{PGSERVICEFILE} = "$srvfile_empty";
}
$node->teardown_node;
+$dummy_node->teardown_node;
done_testing();
--
2.43.5
