Re: tests for client programs

Hi,

I personally would very much like to get this patch commited. It doesn't
have much risk in destabilizing stuff, rather the contrary.

Peter, what's you opinion about the current state?

On 2014-02-27 21:44:48 -0500, Peter Eisentraut wrote:
> diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
> index 16b3621..aee049a 100644
> --- a/doc/src/sgml/regress.sgml
> +++ b/doc/src/sgml/regress.sgml
> @@ -204,6 +204,12 @@ <title>Additional Test Suites</title>
> located in <filename>src/test/isolation</>.
> </para>
> </listitem>
> + <listitem>
> + <para>
> + Tests of client programs under <filename>src/bin</filename>. See
> + also <xref linkend="regress-tap">.
> + </para>
> + </listitem>
> </itemizedlist>
>
> <para>
> @@ -660,6 +666,28 @@ <title>Variant Comparison Files</title>
>
> </sect1>
>
> + <sect1 id="regress-tap">
> + <title>TAP Tests</title>
> +
> + <para>
> + The client program tests under <filename>src/bin</filename> use the Perl
> + TAP tools and are run by <command>prove</command>. You can pass
> + command-line options to <command>prove</command> by setting
> + the <command>make</command> variable <varname>PROVE_FLAGS</>, for example:
> +<programlisting>
> +make -C src/bin check PROVE_FLAGS='--reverse'
> +</programlisting>
> + The default is <literal>--verbose</literal>. See the manual page
> + of <command>prove</command> for more information.
> + </para>
> +
> + <para>
> + The tests written in Perl require the Perl
> + module <literal>IPC::Run</literal>, otherwise most tests will be skipped.
> + This module is available from CPAN or an operating system package.
> + </para>
> + </sect1>
> +

There's actually also some binaries in /contrib, so maybe phrase this a
bit more generally?

> <sect1 id="regress-coverage">

> lcov.info: $(gcda_files)
> rm -f *.gcov
> - $(if $^,$(LCOV) -d . -c -o $@ $(LCOVFLAGS))
> + $(if $^,$(LCOV) -d . -c -o $@ $(LCOVFLAGS) --gcov-tool $(GCOV))

Looks unrelated, but whatever.

> +open HBA, ">>$tempdir/pgdata/pg_hba.conf";
> +print HBA "local replication all trust\n";
> +print HBA "host replication all 127.0.0.1/32 trust\n";
> +print HBA "host replication all ::1/128 trust\n";
> +close HBA;

Given the recent make check security discussions, this doesn't seem like
a good idea...

> +issues_sql_like(['createdb', 'foobar1'], qr/statement: CREATE DATABASE foobar1/, 'SQL CREATE DATABASE run');
> +issues_sql_like(['createdb', 'foobar2', '-l', 'C', '-E', 'LATIN1', '-T', 'template0'], qr/statement: CREATE DATABASE foobar2 ENCODING 'LATIN1'/, 'create database with encoding');

Hm. Are all platforms guaranteed to provide latin1?

> diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
> +if (!$ENV{PGPORT}) {
> + $ENV{PGPORT} = 65432;
> +}
> +
> +$ENV{PGPORT} = int($ENV{PGPORT}) % 65536;

Hm. I think this should use logical similar to what pg_regress is using,
namely test a few ports.

> +sub start_test_server {
> + my ($tempdir) = @_;
> + my $ret;
> +
> + system "initdb -D $tempdir/pgdata -A trust -N >/dev/null";
> + $ret = system 'pg_ctl', '-D', "$tempdir/pgdata", '-s', '-w', '-l', "$tempdir/logfile", '-o', "--fsync=off -k $tempdir --listen-addresses='' --log-statement=all", 'start';
> +
> + if ($ret != 0) {
> + system('cat', "$tempdir/logfile");
> + BAIL_OUT("pg_ctl failed");
> + }
> +
> + $ENV{PGHOST} = $tempdir;
> + $test_server_datadir = "$tempdir/pgdata";
> + $test_server_logfile = "$tempdir/logfile";
> +}

Should stuff like --fsync-off, -k, really be on by default?

I think the code to massage pg_hba.conf should also be here, there'll be
a fair number of tests that need it.

Some questions:
* I haven't looked very careful, but does this set PATH correctly to
pick up programs?
* What does installcheck mean for this?
* I think there should be support for contrib modules to use this
automatically, without overwriting makefile targets.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services