Re: tests for client programs

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.

I put together three handfuls of tests to show what it would look like.
For extra fun, I added a "todo" test in pg_basebackup for a feature
that's currently being proposed in the commit fest.

A significant near-future project would be adding tests for pg_dump and
pg_upgrade.

Lots of things to argue about here: tools, file layout, naming, Perl
code, test quality, etc.

To try it out, apply the attached patch and run make -C src/bin check
(or installcheck, after installation).

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

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.

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

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. ;-)

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

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

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.

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

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

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(at)gmx(dot)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

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

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:

http://www.postgresql.org/message-id/20140329172224.GA170273@tornado.leadboat.com

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

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:
>
> http://www.postgresql.org/message-id/20140329172224.GA170273@tornado.leadboat.com

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

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:
> >
> > http://www.postgresql.org/message-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

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