On Sat, Feb 24, 2018 at 10:51:22AM -0500, Peter Eisentraut wrote:
> On 2/17/18 08:48, Michael Paquier wrote:
>> Attached is what I have finished with. I have gathered the feedback
>> from everybody on this thread and I think that the result can satisfy
>> all the requirements mentioned:
>> - 0001 is a small patch which makes the SSL and LDAP test suite fail
>> immediately if the build's ./configure is not set up with necessary
>> build options. This uses TestLib::check_pg_config to do the checks.
>
> I'm not sure why we need that. The tests will presumably fail anyway.
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;
}
Would result in this output:
t/001_ssltests.pl .. # SSL tests not supported without support in build
# Looks like your test exited with 255 before it could output anything.
t/001_ssltests.pl .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 40/40 subtests
I would find that more helful to make the difference between an
implementation failure and a test failing because of a missing
dependency.
> I think sticking this into the Perl code is too complicated and
> inflexible. We might want to use the same mechanism for non-TAP tests
> as well.
>
> 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?
--
Michael