On Wed, Feb 28, 2018 at 10:02:37AM -0500, Peter Eisentraut wrote:
> On 2/24/18 18:29, Michael Paquier wrote:
>> Sure. But then I think that it would be nice to show up on screen the
>> reason why the test failed if possible. As of now if SSL is missing the
>> whole run shows in red without providing much useful information.
>> Instead of 0001 as shaped previously, why not using as well diag to show
>> the failure on the screen?
>>
>> For example the following block at the top of each test:
>> if (!check_pg_config("#define USE_OPENSSL 1"))
>> {
>> diag "SSL tests not supported without support in build";
>> die;
>> }
>
> I think BAIL_OUT() is intended for this.That's a better idea. I have reworked that in 0001. What do you think? This has the same effect as diag(), which shows information directly to the screen, and that's the behavior I was looking for. >>> What I had in mind would consist of something like this in >>> src/test/Makefile: >>> >>> ifeq ($(with_ldap),yes) >>> ifneq (,$(filter ldap,$(YOUR_VARIABLE_HERE))) >>> SUBDIRS += ldap >>> endif >>> endif >> >> OK. So let's call it PG_TEST_EXTRA_INSECURE or PG_TEST_EXTRA, which can >> take 'ldap', 'ssl' or 'ldap,ssl' as valid values. Seeing that >> documented is really necessary in my opinion. Any idea for a better >> name? > > I don't have a great idea about the name. The value should be > space-separated to work better with make functions. I have concluded about using the most simple name: PG_TEST_EXTRA. A documentation attempt is added as well. This is unfortunately close to EXTRA_TESTS. It would be tempting to directly reuse EXTRA_TESTS as well, however this is used only for the core regression tests now, so a separated variable seems adapted to me. Thoughts and reviews are welcome. -- Michael
From 98ef83f68975279267f290dfdc772d1e3725ecd8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 2 Mar 2018 17:00:23 +0900
Subject: [PATCH 1/2] Prevent SSL and LDAP tests from running without support
in build
An extra set of checks in each one of the test files is added to make
the tests fail should they be invoked without the proper build options.
This makes use of TestLib::check_pg_config to check for the build
configuration.
---
src/test/ldap/t/001_auth.pl | 4 ++++
src/test/ssl/t/001_ssltests.pl | 4 ++++
src/test/ssl/t/002_scram.pl | 4 ++++
3 files changed, 12 insertions(+)
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index a83d96ae91..f1d2f9a1e6 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -4,6 +4,10 @@ use TestLib;
use PostgresNode;
use Test::More tests => 19;
+# LDAP tests are not supported without proper build options
+BAIL_OUT("LDAP tests not supported without support in build") unless
+ check_pg_config("#define USE_LDAP 1");
+
my ($slapd, $ldap_bin_dir, $ldap_schema_dir);
$ldap_bin_dir = undef; # usually in PATH
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 4b097a69bf..cb8bc81f9a 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -6,6 +6,10 @@ use Test::More tests => 62;
use ServerSetup;
use File::Copy;
+# SSL tests are not supported without proper build options
+BAIL_OUT("SSL tests not supported without support in build") unless
+ check_pg_config("#define USE_OPENSSL 1");
+
#### Some configuration
# This is the hostname used to connect to the server. This cannot be a
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 9460763a65..be1f49fe21 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -8,6 +8,10 @@ use Test::More tests => 6;
use ServerSetup;
use File::Copy;
+# SSL tests are not supported without proper build
+BAIL_OUT("SSL tests not supported without support in build") unless
+ check_pg_config("#define USE_OPENSSL 1");
+
# This is the hostname used to connect to the server.
my $SERVERHOSTADDR = '127.0.0.1';
--
2.16.2
From 29c605031929da4acb2754c923cd1cb99c8820ef Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 2 Mar 2018 17:19:34 +0900
Subject: [PATCH 2/2] Add PG_TEST_EXTRA to control optional test suites
By default, SSL and LDAP test suites are not allowed to run as they are
not secure for multi-user environments, which is why they are not part
of check-world. This commit adds an extra make variable which can be
used to optionally enable them if wanted. The user can make use of the
variable like that for example:
make -C src/test check PG_TEST_EXTRA='ssl ldap'
PG_TEST_EXTRA needs to be a list of items separated by
whitespaces, and supports two values for now: 'ssl' and 'ldap' to be
able to run respectively tests in src/test/ssl and src/test/ldap.
In consequence, the SSL and LDAP test suites are added to check-world
but they are skipped except if the user has asked for them to be
enabled. If the build does not support SSL or LDAP, those tests are
automatically ignored.
---
configure.in | 1 +
doc/src/sgml/regress.sgml | 17 +++++++++++++++++
src/Makefile.global.in | 1 +
src/test/Makefile | 16 +++++++++++++++-
4 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/configure.in b/configure.in
index 4d26034579..aee3ab0867 100644
--- a/configure.in
+++ b/configure.in
@@ -682,6 +682,7 @@ PGAC_ARG_BOOL(with, ldap, no,
[build with LDAP support],
[AC_DEFINE([USE_LDAP], 1, [Define to 1 to build with LDAP support. (--with-ldap)])])
AC_MSG_RESULT([$with_ldap])
+AC_SUBST(with_ldap)
#
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index ca2716a6d7..0a5946203c 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -211,6 +211,23 @@ make installcheck-world
option <option>--enable-tap-tests</option>. This is recommended for
development, but can be omitted if there is no suitable Perl installation.
</para>
+
+ <para>
+ TAP tests under <filename>src/test/ssl</filename> and
+ <filename>src/test/ldap</filename> are not secure to run on a multi-system
+ environment. You can decide which test suites to additionally allow by
+ setting the <command>make</command> variable
+ <varname>PG_TEST_EXTRA</varname> to define a list of tests separated
+ by a whitespace.
+<programlisting>
+make -C src/test check PG_TEST_EXTRA='ssl ldap'
+</programlisting>
+ As of now, two test types are supported: <literal>ssl</literal> to allow
+ tests in <filename>src/test/ssl</filename> to be run, and
+ <literal>ldap</literal> for <filename>src/test/ldap</filename>. Those
+ tests will not be run if the build does not support respectively
+ <acronym>SSL</acronym> and <acronym>LDAP</acronym> even if listed.
+ </para>
</sect2>
<sect2>
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d980f81046..dcb8dc5d90 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -186,6 +186,7 @@ with_tcl = @with_tcl@
with_openssl = @with_openssl@
with_selinux = @with_selinux@
with_systemd = @with_systemd@
+with_ldap = @with_ldap@
with_libxml = @with_libxml@
with_libxslt = @with_libxslt@
with_system_tzdata = @with_system_tzdata@
diff --git a/src/test/Makefile b/src/test/Makefile
index 73abf163f1..2fa3463c87 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -14,10 +14,24 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = perl regress isolation modules authentication recovery subscription
+# Test suites which are not safe by default, but can be run if enforced
+# by the users via the whitespace-separated list in variable PG_TEST_EXTRA.
+ifeq ($(with_ldap),yes)
+ifneq (,$(filter ldap,$(PG_TEST_EXTRA)))
+SUBDIRS += ldap
+endif
+endif
+ifeq ($(with_openssl),yes)
+ifneq (,$(filter ssl,$(PG_TEST_EXTRA)))
+SUBDIRS += ssl
+endif
+endif
+
# We don't build or execute examples/, locale/, or thread/ by default,
# but we do want "make clean" etc to recurse into them. Likewise for
# ldap/ and ssl/, because these test suites are not secure to run on a
-# multi-user system.
+# multi-user system, still those can be enforced if wanted using
+# PG_TEST_EXTRA.
ALWAYS_SUBDIRS = examples ldap locale thread ssl
# We want to recurse to all subdirs for all standard targets, except that
--
2.16.2
signature.asc
Description: PGP signature
