Ensure that STDERR is empty during connect_ok
As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
inspecting STDERR in connect_ok and require it to be empty. This is not really
NSS specific, and could help find issues in other libraries as well so I
propose to apply it regardless of the fate of the NSS patchset.
(The change in the SCRAM tests stems from this now making all testruns have the
same number of tests. While I prefer to not plan at all and instead run
done_testing(), doing that consistently is for another patch, keeping this with
the remainder of the suites.)
--
Daniel Gustafsson https://vmware.com/
Attachments:
0001-Ensure-that-STDERR-is-empty-in-connect_ok-tests.patchapplication/octet-stream; name=0001-Ensure-that-STDERR-is-empty-in-connect_ok-tests.patch; x-unix-mode=0644Download
From f263d24e115e8ab2ba9494f600b3b723ce5c2973 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Wed, 2 Feb 2022 14:44:21 +0100
Subject: [PATCH] Ensure that STDERR is empty in connect_ok tests
Connections performed via connect_ok() in TAP tests should not write
anything to STDERR. Ensure that in connect_ok, and adjust the number
of planned tests accordingly.
Author: Jacob Champion <pchampion@vmware.com>
Discussion: https://postgr.es/m/<tbd>
Discussion: https://postgr.es/m/ec146256e31afa0542f9fa970ec258c5f1a5f98.camel@vmware.com
---
src/test/authentication/t/001_password.pl | 2 +-
src/test/authentication/t/002_saslprep.pl | 2 +-
src/test/kerberos/t/001_auth.pl | 2 +-
src/test/ldap/t/001_auth.pl | 2 +-
src/test/perl/PostgreSQL/Test/Cluster.pm | 5 ++++-
src/test/ssl/t/001_ssltests.pl | 2 +-
src/test/ssl/t/002_scram.pl | 8 +++++---
src/test/ssl/t/003_sslinfo.pl | 2 +-
8 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 5d56455857..29110af614 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -20,7 +20,7 @@ if (!$use_unix_sockets)
}
else
{
- plan tests => 23;
+ plan tests => 32;
}
diff --git a/src/test/authentication/t/002_saslprep.pl b/src/test/authentication/t/002_saslprep.pl
index d8995b1ae5..808ed8991b 100644
--- a/src/test/authentication/t/002_saslprep.pl
+++ b/src/test/authentication/t/002_saslprep.pl
@@ -17,7 +17,7 @@ if (!$use_unix_sockets)
}
else
{
- plan tests => 12;
+ plan tests => 20;
}
# Delete pg_hba.conf from the given node, add a new entry to it
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index 2b539d2402..0c0b7459fb 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -23,7 +23,7 @@ use Time::HiRes qw(usleep);
if ($ENV{with_gssapi} eq 'yes')
{
- plan tests => 44;
+ plan tests => 54;
}
else
{
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index cdc4e1e5c8..39d8010aee 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -9,7 +9,7 @@ use Test::More;
if ($ENV{with_ldap} eq 'yes')
{
- plan tests => 28;
+ plan tests => 40;
}
else
{
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 265f3ae657..ff7178c6f5 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2189,8 +2189,11 @@ sub connect_ok
if (defined($params{expected_stdout}))
{
- like($stdout, $params{expected_stdout}, "$test_name: matches");
+ like($stdout, $params{expected_stdout}, "$test_name: stdout matches");
}
+
+ is($stderr, "", "$test_name: no stderr");
+
if (@log_like or @log_unlike)
{
my $log_contents = PostgreSQL::Test::Utils::slurp_file($self->logfile, $log_location);
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index b1fb15ce80..5e8807bde7 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -21,7 +21,7 @@ if ($ENV{with_ssl} ne 'openssl')
}
else
{
- plan tests => 110;
+ plan tests => 140;
}
#### Some configuration
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 86312be88c..e6c517a5d7 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -20,6 +20,10 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
+else
+{
+ plan tests => 15;
+}
# This is the hostname used to connect to the server.
my $SERVERHOSTADDR = '127.0.0.1';
@@ -30,8 +34,6 @@ my $SERVERHOSTCIDR = '127.0.0.1/32';
my $supports_tls_server_end_point =
check_pg_config("#define HAVE_X509_GET_SIGNATURE_NID 1");
-my $number_of_tests = $supports_tls_server_end_point ? 11 : 12;
-
# Allocation of base connection string shared among multiple tests.
my $common_connstr;
@@ -118,4 +120,4 @@ $node->connect_ok(
qr/connection authenticated: identity="ssltestuser" method=scram-sha-256/
]);
-done_testing($number_of_tests);
+done_testing();
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 8c760b39db..c5fd7f6195 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ssl} ne 'openssl')
}
else
{
- plan tests => 13;
+ plan tests => 14;
}
#### Some configuration
--
2.24.3 (Apple Git-128)
On 2022-Feb-02, Daniel Gustafsson wrote:
As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
inspecting STDERR in connect_ok and require it to be empty. This is not really
NSS specific, and could help find issues in other libraries as well so I
propose to apply it regardless of the fate of the NSS patchset.(The change in the SCRAM tests stems from this now making all testruns have the
same number of tests. While I prefer to not plan at all and instead run
done_testing(), doing that consistently is for another patch, keeping this with
the remainder of the suites.)
Since commit 405f32fc4960 we can rely on subtests for this, so perhaps
we should group all the tests in connect_ok() (including your new one)
into a subtest; then each connect_ok() calls count as a single test, and
we can add more tests in it without having to change the callers.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".
Daniel Gustafsson <daniel@yesql.se> writes:
As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
inspecting STDERR in connect_ok and require it to be empty. This is not really
NSS specific, and could help find issues in other libraries as well so I
propose to apply it regardless of the fate of the NSS patchset.
+1
(The change in the SCRAM tests stems from this now making all testruns have the
same number of tests. While I prefer to not plan at all and instead run
done_testing(), doing that consistently is for another patch, keeping this with
the remainder of the suites.)
+1 to that too, counting the tests is a pretty useless exercise.
regards, tom lane
On 2 Feb 2022, at 16:01, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Feb-02, Daniel Gustafsson wrote:
As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
inspecting STDERR in connect_ok and require it to be empty. This is not really
NSS specific, and could help find issues in other libraries as well so I
propose to apply it regardless of the fate of the NSS patchset.(The change in the SCRAM tests stems from this now making all testruns have the
same number of tests. While I prefer to not plan at all and instead run
done_testing(), doing that consistently is for another patch, keeping this with
the remainder of the suites.)Since commit 405f32fc4960 we can rely on subtests for this, so perhaps
we should group all the tests in connect_ok() (including your new one)
into a subtest; then each connect_ok() calls count as a single test, and
we can add more tests in it without having to change the callers.
Disclaimer: longer term I would prefer to remove test plan counting like above
and Toms comment downthread. This is just to make this patch more palatable on
the way there.
Making this a subtest in order to not having to change the callers, turns the
patch into the attached. For this we must group the new test with one already
existing test, if we group more into it (which would make more sense) then we
need to change callers as that reduce the testcount across the tree.
This makes the patch smaller, but not more readable IMO (given that we can't
make it fully use subtests), so my preference is still the v1 patch.
Or did I misunderstand you?
--
Daniel Gustafsson https://vmware.com/
Attachments:
v2-0001-Ensure-that-STDERR-is-empty-in-connect_ok-tests.patchapplication/octet-stream; name=v2-0001-Ensure-that-STDERR-is-empty-in-connect_ok-tests.patch; x-unix-mode=0644Download
From a05a7ec1fa3d7daaea91ef61a76fb4105289363b Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Wed, 2 Feb 2022 14:44:21 +0100
Subject: [PATCH v2] Ensure that STDERR is empty in connect_ok tests
Connections performed via connect_ok() in TAP tests should not write
anything to STDERR. Ensure that in connect_ok, and adjust the number
of planned tests accordingly.
Author: Jacob Champion <pchampion@vmware.com>
Discussion: https://postgr.es/m/<tbd>
Discussion: https://postgr.es/m/ec146256e31afa0542f9fa970ec258c5f1a5f98.camel@vmware.com
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 265f3ae657..da549e5428 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2185,12 +2185,19 @@ sub connect_ok
connstr => "$connstr",
on_error_stop => 0);
- is($ret, 0, $test_name);
-
- if (defined($params{expected_stdout}))
+ subtest "connect_ok_tests" => sub
{
- like($stdout, $params{expected_stdout}, "$test_name: matches");
- }
+ is($ret, 0, $test_name);
+
+ if (defined($params{expected_stdout}))
+ {
+ like($stdout, $params{expected_stdout}, "$test_name: stdout matches");
+ }
+
+ is($stderr, "", "$test_name: no stderr");
+ done_testing();
+ };
+
if (@log_like or @log_unlike)
{
my $log_contents = PostgreSQL::Test::Utils::slurp_file($self->logfile, $log_location);
--
2.24.3 (Apple Git-128)
On 2022-Feb-02, Daniel Gustafsson wrote:
Making this a subtest in order to not having to change the callers, turns the
patch into the attached. For this we must group the new test with one already
existing test, if we group more into it (which would make more sense) then we
need to change callers as that reduce the testcount across the tree.
Well, it wasn't my intention that this patch would not have to change
the callers. What I was thinking about is making connect_ok() have a
subtest for *all* the tests it contains (and changing the callers to
match), so that *in the future* we can add more tests there without
having to change the callers *again*.
Or did I misunderstand you?
I think you did.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)
Tom Lane <tgl@sss.pgh.pa.us> writes:
Daniel Gustafsson <daniel@yesql.se> writes:
While I prefer to not plan at all and instead run done_testing(),
doing that consistently is for another patch, keeping this with the
remainder of the suites.+1 to that too, counting the tests is a pretty useless exercise.
Rather than waiting for Someone™ to find a suitably-shaped tuit to do a
whole sweep converting everything to done_testing(), I think we should
make a habit of converting individual scripts whenever a change breaks
the count.
- ilmari
On 2/2/22 11:01, Dagfinn Ilmari Mannsåker wrote:
Tom Lane <tgl@sss.pgh.pa.us> writes:
Daniel Gustafsson <daniel@yesql.se> writes:
While I prefer to not plan at all and instead run done_testing(),
doing that consistently is for another patch, keeping this with the
remainder of the suites.+1 to that too, counting the tests is a pretty useless exercise.
Rather than waiting for Someone™ to find a suitably-shaped tuit to do a
whole sweep converting everything to done_testing(), I think we should
make a habit of converting individual scripts whenever a change breaks
the count.
Or when anyone edits a script, even if the number of tests doesn't get
broken.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On 2022-Feb-02, Andrew Dunstan wrote:
On 2/2/22 11:01, Dagfinn Ilmari Mannsåker wrote:
Rather than waiting for Someone™ to find a suitably-shaped tuit to do a
whole sweep converting everything to done_testing(), I think we should
make a habit of converting individual scripts whenever a change breaks
the count.
+1.
Or when anyone edits a script, even if the number of tests doesn't get
broken.
Sure, if the committer is up to doing it, but let's not make that a hard
requirement for any commit that touches a test script.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ni aún el genio muy grande llegaría muy lejos
si tuviera que sacarlo todo de su propio interior" (Goethe)
On Wed, Feb 02, 2022 at 04:42:09PM +0100, Daniel Gustafsson wrote:
Disclaimer: longer term I would prefer to remove test plan counting like above
and Toms comment downthread.
That would be really nice.
--
Michael
On Wed, Feb 02, 2022 at 03:40:39PM +0100, Daniel Gustafsson wrote:
As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
inspecting STDERR in connect_ok and require it to be empty. This is not really
NSS specific, and could help find issues in other libraries as well so I
propose to apply it regardless of the fate of the NSS patchset.
Sounds sensible from here. Thanks!
All the code paths seem to be covered, at quick glance.
--
Michael
On 6 Feb 2022, at 08:00, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Feb 02, 2022 at 03:40:39PM +0100, Daniel Gustafsson wrote:
As part of the NSS patchset, quite a few bugs (and NSS quirks) were found by
inspecting STDERR in connect_ok and require it to be empty. This is not really
NSS specific, and could help find issues in other libraries as well so I
propose to apply it regardless of the fate of the NSS patchset.Sounds sensible from here. Thanks!
All the code paths seem to be covered, at quick glance.
Applied, minus the changes to the test plans which are no longer required after
549ec201d6.
--
Daniel Gustafsson https://vmware.com/