Create shorthand for including all extra tests

Started by Nazir Bilal Yavuzover 2 years ago12 messages

byavuz81@gmail.com

over 2 years ago

1 attachment(s)

Hi,

I realized that I forgot to add the new extra test to my test scripts.
So, I thought maybe we can use shorthand for including all extra
tests. With that, running a full testsuite is easier without having to
keep up with new tests and updates.

I created an 'all' option for PG_TEST_EXTRA to enable all test suites
defined under PG_TEST_EXTRA. I created the check_extra_tests_enabled()
function in the Test/Utils.pm file. This function takes the test's
name as an input and checks if PG_TEST_EXTRA contains 'all' or this
test's name.

I thought another advantage could be that this can be used in CI. But
when 'wal_consistency_checking' is enabled, CI times get longer since
it does resource intensive operations.

Any kind of feedback would be appreciated.

Regards,
Nazir Bilal Yavuz
Microsoft

Attachments:

v1-0001-Create-shorthand-for-including-all-extra-tests.patchtext/x-diff; charset=US-ASCII; name=v1-0001-Create-shorthand-for-including-all-extra-tests.patchDownload

From be91a0aaf926c83bf266f92e0523f41ca333b048 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Mon, 4 Sep 2023 16:58:42 +0300
Subject: [PATCH v1] Create shorthand for including all extra tests

This patch aims to make running full testsuite easier without having to
keep up with new tests and updates.

Create 'all' option for PG_TEST_EXTRA to enable all test suites defined
under PG_TEST_EXTRA. That is achieved by creating
check_extra_tests_enabled() function in Test/Utils.pm file. This
function takes the test's name as an input and checks if PG_TEST_EXTRA
contains 'all' or this test's name.
---
 doc/src/sgml/regress.sgml                        |  9 +++++++++
 src/interfaces/libpq/t/004_load_balance_dns.pl   |  2 +-
 src/test/kerberos/t/001_auth.pl                  |  2 +-
 src/test/ldap/t/001_auth.pl                      |  2 +-
 src/test/ldap/t/002_bindpasswd.pl                |  2 +-
 src/test/modules/Makefile                        |  2 +-
 .../t/001_mutated_bindpasswd.pl                  |  2 +-
 src/test/perl/PostgreSQL/Test/Utils.pm           | 16 ++++++++++++++++
 src/test/recovery/t/027_stream_regress.pl        |  3 +--
 src/test/ssl/t/001_ssltests.pl                   |  2 +-
 src/test/ssl/t/002_scram.pl                      |  2 +-
 src/test/ssl/t/003_sslinfo.pl                    |  2 +-
 12 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 675db86e4d7..40269c258ef 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -262,6 +262,15 @@ make check-world PG_TEST_EXTRA='kerberos ldap ssl load_balance'
 </programlisting>
    The following values are currently supported:
    <variablelist>
+    <varlistentry>
+     <term><literal>all</literal></term>
+     <listitem>
+      <para>
+       Enables all extra tests.
+      </para>
+     </listitem>
+    </varlistentry>
+
     <varlistentry>
      <term><literal>kerberos</literal></term>
      <listitem>
diff --git a/src/interfaces/libpq/t/004_load_balance_dns.pl b/src/interfaces/libpq/t/004_load_balance_dns.pl
index 875070e2120..62eeb21843e 100644
--- a/src/interfaces/libpq/t/004_load_balance_dns.pl
+++ b/src/interfaces/libpq/t/004_load_balance_dns.pl
@@ -6,7 +6,7 @@ use PostgreSQL::Test::Utils;
 use PostgreSQL::Test::Cluster;
 use Test::More;
 
-if ($ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+if (!PostgreSQL::Test::Utils::check_extra_text_enabled('load_balance'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index 0deb9bffc8d..59574178afc 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -28,7 +28,7 @@ if ($ENV{with_gssapi} ne 'yes')
 {
 	plan skip_all => 'GSSAPI/Kerberos not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bkerberos\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('kerberos'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test GSSAPI/Kerberos not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 3e113fd6ebb..9db07e801e9 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ldap} ne 'yes')
 {
 	plan skip_all => 'LDAP not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ldap/t/002_bindpasswd.pl b/src/test/ldap/t/002_bindpasswd.pl
index bcd4aa2b742..a1b1bd8c22f 100644
--- a/src/test/ldap/t/002_bindpasswd.pl
+++ b/src/test/ldap/t/002_bindpasswd.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ldap} ne 'yes')
 {
 	plan skip_all => 'LDAP not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 6331c976dcb..2fdcff24785 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -43,7 +43,7 @@ endif
 
 # Test runs an LDAP server, so only run if ldap is in PG_TEST_EXTRA
 ifeq ($(with_ldap),yes)
-ifneq (,$(filter ldap,$(PG_TEST_EXTRA)))
+ifneq (,$(filter all ldap,$(PG_TEST_EXTRA)))
 SUBDIRS += ldap_password_func
 else
 ALWAYS_SUBDIRS += ldap_password_func
diff --git a/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl b/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
index c96c8d7a4de..0fc9d365287 100644
--- a/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
+++ b/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ldap} ne 'yes')
 {
 	plan skip_all => 'LDAP not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 617caa022f4..835c337797a 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -268,6 +268,22 @@ sub all_tests_passing
 
 =pod
 
+=item check_extra_text_enabled()
+
+Return true if all extra tests or the test given as an input is enabled.
+
+=cut
+
+sub check_extra_text_enabled
+{
+	my ($extra_test) = @_;
+	return
+		 $ENV{PG_TEST_EXTRA} =~ m/\ball\b/
+	  || $ENV{PG_TEST_EXTRA} =~ m/\b$extra_test\b/;
+}
+
+=pod
+
 =item tempdir(prefix)
 
 Securely create a temporary directory inside C<$tmp_check>, like C<mkdtemp>,
diff --git a/src/test/recovery/t/027_stream_regress.pl b/src/test/recovery/t/027_stream_regress.pl
index f2f4e77626f..ba2014fb4cd 100644
--- a/src/test/recovery/t/027_stream_regress.pl
+++ b/src/test/recovery/t/027_stream_regress.pl
@@ -32,8 +32,7 @@ $node_primary->append_conf('postgresql.conf', 'synchronize_seqscans = off');
 
 # WAL consistency checking is resource intensive so require opt-in with the
 # PG_TEST_EXTRA environment variable.
-if (   $ENV{PG_TEST_EXTRA}
-	&& $ENV{PG_TEST_EXTRA} =~ m/\bwal_consistency_checking\b/)
+if (PostgreSQL::Test::Utils::check_extra_text_enabled('wal_consistency_checking'))
 {
 	$node_primary->append_conf('postgresql.conf',
 		'wal_consistency_checking = all');
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 23248d71b06..1ddc636ee5f 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -17,7 +17,7 @@ if ($ENV{with_ssl} ne 'openssl')
 {
 	plan skip_all => 'OpenSSL not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 27abd02abf1..0fa5e0e709c 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ssl} ne 'openssl')
 {
 	plan skip_all => 'OpenSSL not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 5306aad8023..78fa9f2e188 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ssl} ne 'openssl')
 {
 	plan skip_all => 'OpenSSL not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
-- 
2.40.1

Tom Lane

tgl@sss.pgh.pa.us

over 2 years ago

In reply to: Nazir Bilal Yavuz (#1)

Re: Create shorthand for including all extra tests

Nazir Bilal Yavuz <byavuz81@gmail.com> writes:

I created an 'all' option for PG_TEST_EXTRA to enable all test suites
defined under PG_TEST_EXTRA.

I think this is a seriously bad idea. The entire point of not including
certain tests in check-world by default is that the omitted tests are
security hazards, so a developer or buildfarm owner should review each
one before deciding whether to activate it on their machine.

regards, tom lane

Daniel Gustafsson

daniel@yesql.se

over 2 years ago

In reply to: Tom Lane (#2)

Re: Create shorthand for including all extra tests

On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nazir Bilal Yavuz <byavuz81@gmail.com> writes:

I created an 'all' option for PG_TEST_EXTRA to enable all test suites
defined under PG_TEST_EXTRA.

I think this is a seriously bad idea. The entire point of not including
certain tests in check-world by default is that the omitted tests are
security hazards, so a developer or buildfarm owner should review each
one before deciding whether to activate it on their machine.

I dunno, I've certainly managed to not run the tests I hoped to multiple times.
I think it could be useful for sandboxed testrunners which are destroyed after
each run. There is for sure a foot-gun angle to it, no question about that.

--
Daniel Gustafsson

Noah Misch

noah@leadboat.com

over 2 years ago

In reply to: Daniel Gustafsson (#3)

Re: Create shorthand for including all extra tests

On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:

On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Nazir Bilal Yavuz <byavuz81@gmail.com> writes:

I created an 'all' option for PG_TEST_EXTRA to enable all test suites
defined under PG_TEST_EXTRA.

I think this is a seriously bad idea. The entire point of not including
certain tests in check-world by default is that the omitted tests are
security hazards, so a developer or buildfarm owner should review each
one before deciding whether to activate it on their machine.

I dunno, I've certainly managed to not run the tests I hoped to multiple times.
I think it could be useful for sandboxed testrunners which are destroyed after
each run. There is for sure a foot-gun angle to it, no question about that.

Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
they treat the loopback interface as private, so anyone with access to
loopback interface ports can hijack the test. I'd be fine with e.g.
PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting
the tester to review the tests to rediscover this common factor.

Tom Lane

tgl@sss.pgh.pa.us

over 2 years ago

In reply to: Noah Misch (#4)

Re: Create shorthand for including all extra tests

Noah Misch <noah@leadboat.com> writes:

On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:

On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think this is a seriously bad idea. The entire point of not including
certain tests in check-world by default is that the omitted tests are
security hazards, so a developer or buildfarm owner should review each
one before deciding whether to activate it on their machine.

Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
they treat the loopback interface as private, so anyone with access to
loopback interface ports can hijack the test. I'd be fine with e.g.
PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting
the tester to review the tests to rediscover this common factor.

Yeah, I could live with something like that from the security standpoint.
Not sure if it helps Nazir's use-case though. Maybe we could invent
categories that can be used in place of individual test names?
For now,

PG_TEST_EXTRA="needs-private-lo slow"

would cover the territory of "all", and I think it'd be very seldom
that we'd have to invent new categories here (though maybe I lack
imagination today).

regards, tom lane

Noah Misch

noah@leadboat.com

over 2 years ago

In reply to: Tom Lane (#5)

Re: Create shorthand for including all extra tests

On Mon, Sep 04, 2023 at 04:30:31PM -0400, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:

On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think this is a seriously bad idea. The entire point of not including
certain tests in check-world by default is that the omitted tests are
security hazards, so a developer or buildfarm owner should review each
one before deciding whether to activate it on their machine.

Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
they treat the loopback interface as private, so anyone with access to
loopback interface ports can hijack the test. I'd be fine with e.g.
PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting
the tester to review the tests to rediscover this common factor.

Yeah, I could live with something like that from the security standpoint.
Not sure if it helps Nazir's use-case though. Maybe we could invent
categories that can be used in place of individual test names?
For now,

PG_TEST_EXTRA="needs-private-lo slow"

would cover the territory of "all", and I think it'd be very seldom
that we'd have to invent new categories here (though maybe I lack
imagination today).

I could imagine categories for filesystem bytes and RAM bytes. Also, while
needs-private-lo has a bounded definition, "slow" doesn't. If today's one
"slow" test increases check-world duration by 1.1x, we may not let a
100x-increase test use the same keyword.

If one introduced needs-private-lo, the present spelling of "all" would be
"needs-private-lo wal_consistency_checking". Looks okay to me. Doing nothing
here wouldn't be ruinous, of course.

Nazir Bilal Yavuz

byavuz81@gmail.com

over 2 years ago

In reply to: Noah Misch (#6)

1 attachment(s)

Re: Create shorthand for including all extra tests

Hi,

Thanks for the feedback! I updated the patch, 'needs-private-lo'
option enables kerberos, ldap, load_balance and ssl extra tests now.

On Mon, Sep 04, 2023 at 04:30:31PM -0400, Tom Lane wrote:

Yeah, I could live with something like that from the security standpoint.
Not sure if it helps Nazir's use-case though. Maybe we could invent
categories that can be used in place of individual test names?
For now,

Yes, that is not ideal for my use-case but still better.

On Tue, 5 Sept 2023 at 00:09, Noah Misch <noah@leadboat.com> wrote:

I could imagine categories for filesystem bytes and RAM bytes. Also, while
needs-private-lo has a bounded definition, "slow" doesn't. If today's one
"slow" test increases check-world duration by 1.1x, we may not let a
100x-increase test use the same keyword.

I agree. I didn't create a new category as 'slow' but still open to suggestions.

I am not very familiar with perl syntax, I would like to hear your
opinions on how the implementation of the check_extra_text_enabled()
function could be done better.

Regards,
Nazir Bilal Yavuz
Microsoft

Attachments:

v2-0001-Create-shorthand-for-including-extra-tests-that-t.patchtext/x-diff; charset=US-ASCII; name=v2-0001-Create-shorthand-for-including-extra-tests-that-t.patchDownload

From 3a33161eef699e4fbcfeb2d62ec387621a331d78 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Tue, 5 Sep 2023 20:14:15 +0300
Subject: [PATCH v2] Create shorthand for including extra tests that treat the
 loopback interface as private

This patch aims to make running full testsuite easier without having to
keep up with new tests and updates.

Create 'needs-private-lo' option for PG_TEST_EXTRA to enable extra
test suites that treat the loopback interface as private. That is
achieved by creating check_extra_tests_enabled() function in
the Test/Utils.pm file. This function takes the test's name as an
input and checks if PG_TEST_EXTRA contains this extra test or any
option that enables this extra test.
---
 doc/src/sgml/regress.sgml                     | 10 +++++
 .../libpq/t/004_load_balance_dns.pl           |  2 +-
 src/test/kerberos/t/001_auth.pl               |  2 +-
 src/test/ldap/t/001_auth.pl                   |  2 +-
 src/test/ldap/t/002_bindpasswd.pl             |  2 +-
 src/test/modules/Makefile                     |  2 +-
 .../t/001_mutated_bindpasswd.pl               |  2 +-
 src/test/perl/PostgreSQL/Test/Utils.pm        | 45 +++++++++++++++++++
 src/test/recovery/t/027_stream_regress.pl     |  3 +-
 src/test/ssl/t/001_ssltests.pl                |  2 +-
 src/test/ssl/t/002_scram.pl                   |  2 +-
 src/test/ssl/t/003_sslinfo.pl                 |  2 +-
 12 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 675db86e4d7..a9ceb0342d3 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -313,6 +313,16 @@ make check-world PG_TEST_EXTRA='kerberos ldap ssl load_balance'
       </para>
      </listitem>
     </varlistentry>
+
+    <varlistentry>
+     <term><literal>needs_private_lo</literal></term>
+     <listitem>
+      <para>
+       Enables the extra tests that treat the loopback interface as a private.
+       Currently enables kerberos, ldap, load_balance and ssl extra tests.
+      </para>
+     </listitem>
+    </varlistentry>
    </variablelist>
 
    Tests for features that are not supported by the current build
diff --git a/src/interfaces/libpq/t/004_load_balance_dns.pl b/src/interfaces/libpq/t/004_load_balance_dns.pl
index 875070e2120..62eeb21843e 100644
--- a/src/interfaces/libpq/t/004_load_balance_dns.pl
+++ b/src/interfaces/libpq/t/004_load_balance_dns.pl
@@ -6,7 +6,7 @@ use PostgreSQL::Test::Utils;
 use PostgreSQL::Test::Cluster;
 use Test::More;
 
-if ($ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+if (!PostgreSQL::Test::Utils::check_extra_text_enabled('load_balance'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index 0deb9bffc8d..59574178afc 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -28,7 +28,7 @@ if ($ENV{with_gssapi} ne 'yes')
 {
 	plan skip_all => 'GSSAPI/Kerberos not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bkerberos\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('kerberos'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test GSSAPI/Kerberos not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 3e113fd6ebb..9db07e801e9 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ldap} ne 'yes')
 {
 	plan skip_all => 'LDAP not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ldap/t/002_bindpasswd.pl b/src/test/ldap/t/002_bindpasswd.pl
index bcd4aa2b742..a1b1bd8c22f 100644
--- a/src/test/ldap/t/002_bindpasswd.pl
+++ b/src/test/ldap/t/002_bindpasswd.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ldap} ne 'yes')
 {
 	plan skip_all => 'LDAP not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index e81873cb5ae..5c977908eec 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -42,7 +42,7 @@ endif
 
 # Test runs an LDAP server, so only run if ldap is in PG_TEST_EXTRA
 ifeq ($(with_ldap),yes)
-ifneq (,$(filter ldap,$(PG_TEST_EXTRA)))
+ifneq (,$(filter all ldap,$(PG_TEST_EXTRA)))
 SUBDIRS += ldap_password_func
 else
 ALWAYS_SUBDIRS += ldap_password_func
diff --git a/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl b/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
index c96c8d7a4de..0fc9d365287 100644
--- a/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
+++ b/src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ldap} ne 'yes')
 {
 	plan skip_all => 'LDAP not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ldap'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 617caa022f4..b9d2d51e8c6 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -268,6 +268,51 @@ sub all_tests_passing
 
 =pod
 
+=item check_extra_text_enabled()
+
+Return 1 if the extra test given as an input is enabled. Otherwise, return 0.
+
+=cut
+
+sub check_extra_text_enabled
+{
+	my ($extra_test) = @_;
+	# Set the extra tests that treat the loopback interface as a private.
+	my %needs_private_lo = map { $_ => 1 } (
+		'kerberos',
+		'ldap',
+		'load_balance',
+		'ssl',
+	);
+	# Set the tests categories
+	my %test_categories = (
+		'needs_private_lo' => {%needs_private_lo},
+	);
+
+	# Look if the individual extra test is enabled.
+	if ($ENV{PG_TEST_EXTRA} =~ m/\b$extra_test\b/)
+	{
+		return 1;
+	}
+	# Traverse the test categories, look if both the test catagory
+	# is enabled and extra test exists in this category.
+	else
+	{
+		foreach my $test_category (keys %test_categories)
+		{
+			if ($ENV{PG_TEST_EXTRA} =~ m/\b$test_category\b/
+			 && exists $test_categories{$test_category}{$extra_test})
+			{
+				return 1;
+			}
+		}
+	}
+
+	return 0;
+}
+
+=pod
+
 =item tempdir(prefix)
 
 Securely create a temporary directory inside C<$tmp_check>, like C<mkdtemp>,
diff --git a/src/test/recovery/t/027_stream_regress.pl b/src/test/recovery/t/027_stream_regress.pl
index f2f4e77626f..ba2014fb4cd 100644
--- a/src/test/recovery/t/027_stream_regress.pl
+++ b/src/test/recovery/t/027_stream_regress.pl
@@ -32,8 +32,7 @@ $node_primary->append_conf('postgresql.conf', 'synchronize_seqscans = off');
 
 # WAL consistency checking is resource intensive so require opt-in with the
 # PG_TEST_EXTRA environment variable.
-if (   $ENV{PG_TEST_EXTRA}
-	&& $ENV{PG_TEST_EXTRA} =~ m/\bwal_consistency_checking\b/)
+if (PostgreSQL::Test::Utils::check_extra_text_enabled('wal_consistency_checking'))
 {
 	$node_primary->append_conf('postgresql.conf',
 		'wal_consistency_checking = all');
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 23248d71b06..1ddc636ee5f 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -17,7 +17,7 @@ if ($ENV{with_ssl} ne 'openssl')
 {
 	plan skip_all => 'OpenSSL not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 27abd02abf1..0fa5e0e709c 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ssl} ne 'openssl')
 {
 	plan skip_all => 'OpenSSL not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 5306aad8023..78fa9f2e188 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ssl} ne 'openssl')
 {
 	plan skip_all => 'OpenSSL not supported by this build';
 }
-elsif ($ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+elsif (!PostgreSQL::Test::Utils::check_extra_text_enabled('ssl'))
 {
 	plan skip_all =>
 	  'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
-- 
2.40.1

Daniel Gustafsson

daniel@yesql.se

over 2 years ago

In reply to: Noah Misch (#6)

Re: Create shorthand for including all extra tests

On 4 Sep 2023, at 23:09, Noah Misch <noah@leadboat.com> wrote:

I could imagine categories for filesystem bytes and RAM bytes. Also, while
needs-private-lo has a bounded definition, "slow" doesn't. If today's one
"slow" test increases check-world duration by 1.1x, we may not let a
100x-increase test use the same keyword.

Agreed, the names should be descriptive enough to contain a boundary. Any new
test which is orders of magnitude slower than an existing test suite most
likely will have one/more boundary characteristics not shared with existing
suites. The test in 20210423204306.5osfpkt2ggaedyvy@alap3.anarazel.de for
autovacuum wraparound comes to mind as one that would warrant a new category.

If one introduced needs-private-lo, the present spelling of "all" would be
"needs-private-lo wal_consistency_checking".

I think it makes sense to invent a new PG_TEST_EXTRA category which (for now)
only contains wal_consistency_checking to make it consistent, such that "all"
can be achieved by a set of categories.

--
Daniel Gustafsson

Peter Eisentraut

peter@eisentraut.org

over 2 years ago

In reply to: Tom Lane (#5)

Re: Create shorthand for including all extra tests

On 04.09.23 22:30, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:

On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think this is a seriously bad idea. The entire point of not including
certain tests in check-world by default is that the omitted tests are
security hazards, so a developer or buildfarm owner should review each
one before deciding whether to activate it on their machine.

Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
they treat the loopback interface as private, so anyone with access to
loopback interface ports can hijack the test. I'd be fine with e.g.
PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting
the tester to review the tests to rediscover this common factor.

Yeah, I could live with something like that from the security standpoint.
Not sure if it helps Nazir's use-case though. Maybe we could invent
categories that can be used in place of individual test names?
For now,

PG_TEST_EXTRA="needs-private-lo slow"

would cover the territory of "all", and I think it'd be very seldom
that we'd have to invent new categories here (though maybe I lack
imagination today).

At least the kerberos tests also appear to require a lot of randomness
for their setup, and sometimes in VM environments they hang for minutes
until they get that. I suppose that would go under "slow".

Also, at least in my mind, when we added the kerberos and ldap tests, a
partial reason for excluding them from the default run was "requires
additional unusual software to be installed". The additional kerberos
and ldap server software used in those tests is not covered by
configure/meson, so it's a bit more DIY.

#10

Peter Eisentraut

peter@eisentraut.org

about 2 years ago

In reply to: Nazir Bilal Yavuz (#7)

Re: Create shorthand for including all extra tests

On 05.09.23 19:26, Nazir Bilal Yavuz wrote:

Thanks for the feedback! I updated the patch, 'needs-private-lo'
option enables kerberos, ldap, load_balance and ssl extra tests now.

As was discussed, I don't think "needs private lo" is the only condition
for these tests. At least kerberos and ldap also need extra software
installed, and load_balance might need editing the system's hosts file.
So someone would still need to familiarize themselves with these tests
individually before setting a global option like this.

Also, if we were to create test groupings like this, I think the
implementation should be different. The way you have it, there is a
sort of central registry of all affected tests in
src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests.
I would prefer a more decentralized approach where each test decides
on its own whether to run, with pseudo-code conditionals like

if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains
"needs-private-lo"))
skip_all

Anyway, at the moment, I don't see a sensible way to group these things
beyond what we have now (effectively, "ldap" is already a group, because
it affects more than one test suite). Right now, we have six possible
values, which is probably just about doable to keep track of manually.
If we get a lot more, then we need to look into this again, but maybe
then we'll also have more patterns to group things around.

#11

Nazir Bilal Yavuz

byavuz81@gmail.com

almost 2 years ago

In reply to: Peter Eisentraut (#10)

Re: Create shorthand for including all extra tests

Hi,

On Wed, 10 Jan 2024 at 23:48, Peter Eisentraut <peter@eisentraut.org> wrote:

On 05.09.23 19:26, Nazir Bilal Yavuz wrote:

Thanks for the feedback! I updated the patch, 'needs-private-lo'
option enables kerberos, ldap, load_balance and ssl extra tests now.

As was discussed, I don't think "needs private lo" is the only condition
for these tests. At least kerberos and ldap also need extra software
installed, and load_balance might need editing the system's hosts file.
So someone would still need to familiarize themselves with these tests
individually before setting a global option like this.

Also, if we were to create test groupings like this, I think the
implementation should be different. The way you have it, there is a
sort of central registry of all affected tests in
src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests.
I would prefer a more decentralized approach where each test decides
on its own whether to run, with pseudo-code conditionals like

if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains
"needs-private-lo"))
skip_all

Anyway, at the moment, I don't see a sensible way to group these things
beyond what we have now (effectively, "ldap" is already a group, because
it affects more than one test suite). Right now, we have six possible
values, which is probably just about doable to keep track of manually.
If we get a lot more, then we need to look into this again, but maybe
then we'll also have more patterns to group things around.

I see your point. It looks like the best option is to reevaluate this
if there are more PG_TEST_EXTRA options.

--
Regards,
Nazir Bilal Yavuz
Microsoft

#12

Peter Eisentraut

peter@eisentraut.org

almost 2 years ago

In reply to: Nazir Bilal Yavuz (#11)

Re: Create shorthand for including all extra tests

On 15.01.24 09:54, Nazir Bilal Yavuz wrote:

Hi,

On Wed, 10 Jan 2024 at 23:48, Peter Eisentraut <peter@eisentraut.org> wrote:

On 05.09.23 19:26, Nazir Bilal Yavuz wrote:

Thanks for the feedback! I updated the patch, 'needs-private-lo'
option enables kerberos, ldap, load_balance and ssl extra tests now.

As was discussed, I don't think "needs private lo" is the only condition
for these tests. At least kerberos and ldap also need extra software
installed, and load_balance might need editing the system's hosts file.
So someone would still need to familiarize themselves with these tests
individually before setting a global option like this.

Also, if we were to create test groupings like this, I think the
implementation should be different. The way you have it, there is a
sort of central registry of all affected tests in
src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests.
I would prefer a more decentralized approach where each test decides
on its own whether to run, with pseudo-code conditionals like

if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains
"needs-private-lo"))
skip_all

Anyway, at the moment, I don't see a sensible way to group these things
beyond what we have now (effectively, "ldap" is already a group, because
it affects more than one test suite). Right now, we have six possible
values, which is probably just about doable to keep track of manually.
If we get a lot more, then we need to look into this again, but maybe
then we'll also have more patterns to group things around.

I see your point. It looks like the best option is to reevaluate this
if there are more PG_TEST_EXTRA options.

Ok, I'm closing this commitfest entry.