tests for client programs

On Wed, January 15, 2014 06:30, Peter Eisentraut wrote:

As we all know, the client programs (src/bin/) don't have any real test
suites. Some pieces are tested as part of the backend regression tests,
some as part of the pg_upgrade test script, but nothing is specifically
targeted, and pg_basebackup for example is not tested at all.

So I wrote something.

I chose to use Perl-based tools, prove and Test::More, because those are
available in a standard Perl installation, and we already require that.

[ 0001-Add-TAP-tests-for-client-programs.patch ]

With perl 5.18.2, in centos 6.5; system provided perl 5.10 has the same problem.

The seems to be a dependency on IPC::Run

I can install that, of course... but I suppose you want to make this work without that.

Thanks,

Erik Rijkers

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1/15/14, 1:46 AM, Erik Rijkers wrote:

The seems to be a dependency on IPC::Run

I can install that, of course... but I suppose you want to make this work without that.

No, IPC::Run will be required. It looked like it was part of the
default installation where I tested, but apparently not.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, January 15, 2014 06:30, Peter Eisentraut wrote:

As we all know, the client programs (src/bin/) don't have any real test

So I wrote something.

I chose to use Perl-based tools, prove and Test::More, because those are

[ 0001-Add-TAP-tests-for-client-programs.patch ] 32 k

I gave this a quick try.

Centos 6.5 final / perl 5.18.2

As mentioned earlier I had to install IPC::Run.

2 tests stumbled:

1. One test ( pg_ctl/t/001_start_stop.pl ) failed because I had PGDATA set. I unset all PG+ vars after that. No a big
problem but nonetheless it might be better if the test suite removes /controls the variables before running.

2. The pg_isready test failed command_fails() ('fails with no server running') because it defaults to the compiled-in
server-port (and that server was running). I added the test-designated port (65432, as defined in TestLib.pm). This
simple change is in the attached patch.

With these two changes the whole test suite passed.

Thanks,

Erik Rijkers

On Wed, 2014-01-15 at 20:56 +0100, Erik Rijkers wrote:

2 tests stumbled:

1. One test ( pg_ctl/t/001_start_stop.pl ) failed because I had PGDATA set. I unset all PG+ vars after that. No a big
problem but nonetheless it might be better if the test suite removes /controls the variables before running.

2. The pg_isready test failed command_fails() ('fails with no server running') because it defaults to the compiled-in
server-port (and that server was running). I added the test-designated port (65432, as defined in TestLib.pm). This
simple change is in the attached patch.

With these two changes the whole test suite passed.

Fixed those two things by unsetting environment variables and picking a
different port.

New patch attached.

Updated patch. Changes:

- added documentation
- avoid port conflicts with running instances
- added tests for pg_basebackup -T
- removed TODO tests for rejected pg_basebackup feature

A test on Windows would be nice. Otherwise we'll let the buildfarm do it.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 4/4/14, 10:44 AM, Andres Freund wrote:

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?

I opine it's committed. ;-)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-04-04 16:44:46 +0200, Andres Freund wrote:

On 2014-02-27 21:44:48 -0500, Peter Eisentraut wrote:

+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.

The issues here don't seem to have been addressed in the commit. At
least the latin1 thing should be fixed.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-04-30 18:09:15 +0200, Andres Freund wrote:

The issues here don't seem to have been addressed in the commit. At
least the latin1 thing should be fixed.

As an additional issue it currently doesn't seem to work in VPATH
builds. That's imo a must fix.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, 2014-04-30 at 18:09 +0200, Andres Freund wrote:

On 2014-04-04 16:44:46 +0200, Andres Freund wrote:

On 2014-02-27 21:44:48 -0500, Peter Eisentraut wrote:

+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...

The socket file for the test server instance is in a private directory,
so that should be safe enough.

+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?

Yes.

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.

That could be improved in the future.

+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?

-k is to set the socket directory. You might be thinking of initdb -k?

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

More refactoring is always possible as needs arise.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-05-06 20:44:56 -0400, Peter Eisentraut wrote:

On Wed, 2014-04-30 at 18:09 +0200, Andres Freund wrote:

On 2014-04-04 16:44:46 +0200, Andres Freund wrote:

On 2014-02-27 21:44:48 -0500, Peter Eisentraut wrote:

+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...

The socket file for the test server instance is in a private directory,
so that should be safe enough.

Well, you're explicitly configuring host connections... That's why I was
wondering.

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.

That could be improved in the future.

Oddly enough you're overwriting it in Magefile.global.in's prove_check anyway.

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

-k is to set the socket directory. You might be thinking of initdb -k?

Yes, sorry. Confused the line with initdb with the pg_ctl one.

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

More refactoring is always possible as needs arise.

I was thinking of

+command_ok(['pg_ctl', 'initdb', '-D', "$tempdir/data"], 'pg_ctl initdb');
+open CONF, ">>$tempdir/data/postgresql.conf";
+print CONF "listen_addresses = ''\n";
+print CONF "unix_socket_directories = '$tempdir'\n";
+close CONF;

Sorry, accidentally wrote hba.conf instead of postgresql.conf because I
was thinking about listen_addresses/authentication.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-04-30 18:17:54 +0200, Andres Freund wrote:

On 2014-04-30 18:09:15 +0200, Andres Freund wrote:

The issues here don't seem to have been addressed in the commit. At
least the latin1 thing should be fixed.

As an additional issue it currently doesn't seem to work in VPATH
builds. That's imo a must fix.

A "cd $(srcdir) && .." in prove_installcheck and prove_check seems to do
the trick.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, 2014-05-07 at 03:08 +0200, Andres Freund wrote:

As an additional issue it currently doesn't seem to work in VPATH
builds. That's imo a must fix.

A "cd $(srcdir) && .." in prove_installcheck and prove_check seems to do
the trick.

Here is my proposed patch for this.

Peter Eisentraut <peter_e@gmx.net> writes:

On Wed, 2014-05-07 at 03:08 +0200, Andres Freund wrote:

As an additional issue it currently doesn't seem to work in VPATH
builds. That's imo a must fix.
A "cd $(srcdir) && .." in prove_installcheck and prove_check seems to do
the trick.

Here is my proposed patch for this.

BTW, my Salesforce colleagues were complaining to me that this stuff
doesn't work at all on older Perl versions; apparently IPC::Run has
changed significantly since Perl 5.8 or so. Can we do anything about
that?

They were also not too happy that the checks get skipped if IPC::Run isn't
installed (as it is not on stock RHEL, for instance). It'd be better if
we could avoid depending on stuff that isn't in a pretty-vanilla Perl
installation.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-06-04 20:40:40 -0400, Peter Eisentraut wrote:

On Wed, 2014-05-07 at 03:08 +0200, Andres Freund wrote:

As an additional issue it currently doesn't seem to work in VPATH
builds. That's imo a must fix.

A "cd $(srcdir) && .." in prove_installcheck and prove_check seems to do
the trick.

Here is my proposed patch for this.

Except that I'd rather named CURDIR REGRESSDIR or such this looks sane.

sub tempdir
{
-	return File::Temp::tempdir('testXXXX', DIR => cwd(), CLEANUP => 1);
+	return File::Temp::tempdir('testXXXX', DIR => $ENV{CURDIR} || cwd(), CLEANUP => 1);
}

Unrelated to this, but for me cleanup doesn't always seem to succeed?
Also could we name the directories tmp_testXXXX akin to tmp_check?

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jun 05, 2014 at 10:57:03AM +0200, Andres Freund wrote:

On 2014-06-04 20:40:40 -0400, Peter Eisentraut wrote:

On Wed, 2014-05-07 at 03:08 +0200, Andres Freund wrote:

As an additional issue it currently doesn't seem to work in VPATH
builds. That's imo a must fix.

A "cd $(srcdir) && .." in prove_installcheck and prove_check seems to do
the trick.

Here is my proposed patch for this.

Except that I'd rather named CURDIR REGRESSDIR or such this looks sane.

sub tempdir
{
-	return File::Temp::tempdir('testXXXX', DIR => cwd(), CLEANUP => 1);
+	return File::Temp::tempdir('testXXXX', DIR => $ENV{CURDIR} || cwd(), CLEANUP => 1);
}

I recommend "TMPDIR => 1" instead of setting DIR. This temporary directory is
used for Unix sockets, so path length limitations will be a problem:

/messages/by-id/20140329172224.GA170273@tornado.leadboat.com

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, 2014-06-05 at 21:57 -0400, Noah Misch wrote:

I recommend "TMPDIR => 1" instead of setting DIR.

I originally decided against doing that, because

1) I don't know if all systems would have enough space in their regular
temporary directory for the kinds of things we put there. Using the
build directory seems safer.

2) One "debugging" method is to set CLEANUP to false and then manually
inspect the data directory left behind. (In the future, this might be
exposed via a command-line option.) This would become more cumbersome
and error-prone if we used TMPDIR.

This temporary directory is
used for Unix sockets, so path length limitations will be a problem:

/messages/by-id/20140329172224.GA170273@tornado.leadboat.com

That, however, is a good argument for doing it the other way. Maybe we
need two temporary directories.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jun 09, 2014 at 09:12:27PM -0400, Peter Eisentraut wrote:

On Thu, 2014-06-05 at 21:57 -0400, Noah Misch wrote:

I recommend "TMPDIR => 1" instead of setting DIR.

I originally decided against doing that, because

1) I don't know if all systems would have enough space in their regular
temporary directory for the kinds of things we put there. Using the
build directory seems safer.

2) One "debugging" method is to set CLEANUP to false and then manually
inspect the data directory left behind. (In the future, this might be
exposed via a command-line option.) This would become more cumbersome
and error-prone if we used TMPDIR.

This temporary directory is
used for Unix sockets, so path length limitations will be a problem:

/messages/by-id/20140329172224.GA170273@tornado.leadboat.com

That, however, is a good argument for doing it the other way. Maybe we
need two temporary directories.

Two temporary directories sounds fair, given those constraints.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2014-06-04 20:40:40 -0400, Peter Eisentraut wrote:

On Wed, 2014-05-07 at 03:08 +0200, Andres Freund wrote:

As an additional issue it currently doesn't seem to work in VPATH
builds. That's imo a must fix.

A "cd $(srcdir) && .." in prove_installcheck and prove_check seems to do
the trick.

Here is my proposed patch for this.

Works here. The tmpdir thing is probably a separate patch...

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers