tap tests remove working directories
One of the things that makes the TAP tests very difficult and annoying
to debug is their insistence on removing their data directories. I'm not
sure why they are doing that. We don't do that with pg_regress. Instead
we have clean targets to remove them if necessary. I suggest that we
either disable that altogether, and provide cleanup make targets, or at
least make it optional, say by setting an environment variable, say
TMP_CLEANUP or some such. There is probably a good case for defaulting
that to off, but I could live with it being on.
Thoughts?
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Dunstan <andrew@dunslane.net> writes:
One of the things that makes the TAP tests very difficult and annoying
to debug is their insistence on removing their data directories. I'm not
sure why they are doing that. We don't do that with pg_regress. Instead
we have clean targets to remove them if necessary. I suggest that we
either disable that altogether, and provide cleanup make targets, or at
least make it optional, say by setting an environment variable, say
TMP_CLEANUP or some such. There is probably a good case for defaulting
that to off, but I could live with it being on.
I thought we'd decided awhile ago that best practice would be to
auto-remove temp directories only on success. Is that a workable
behavior for you, or are you concerned about being able to poke
around even after the test thinks it succeeded?
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 08/07/2015 05:11 PM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
One of the things that makes the TAP tests very difficult and annoying
to debug is their insistence on removing their data directories. I'm not
sure why they are doing that. We don't do that with pg_regress. Instead
we have clean targets to remove them if necessary. I suggest that we
either disable that altogether, and provide cleanup make targets, or at
least make it optional, say by setting an environment variable, say
TMP_CLEANUP or some such. There is probably a good case for defaulting
that to off, but I could live with it being on.I thought we'd decided awhile ago that best practice would be to
auto-remove temp directories only on success. Is that a workable
behavior for you, or are you concerned about being able to poke
around even after the test thinks it succeeded?
That certainly isn't what happens, and given the way this is done in
TestLib.pm, using the CLEANUP parameter of File::Temp's tempdir()
function, it's not clear how we could do that easily. The deletion
behaviour is set when you create the directory, not afterwards. What I
suggested could be done with a couple of lines of code.
I could probably live with your suggestion, especially if I could change
the behaviour easily. But what we have now is quite frustrating. I have
to hack the source just to be able to diagnose an error. That's really
pretty unacceptable.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 7, 2015 at 7:17 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
That certainly isn't what happens, and given the way this is done in
TestLib.pm, using the CLEANUP parameter of File::Temp's tempdir() function,
it's not clear how we could do that easily.
<shot-in-the-dark>
Set cleanup to false and manually remove the directory later in the
code, so that stuff runs only if we haven't died sooner?
</shot-in-the-dark>
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/08/2015 09:31 AM, Robert Haas wrote:
On Fri, Aug 7, 2015 at 7:17 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
That certainly isn't what happens, and given the way this is done in
TestLib.pm, using the CLEANUP parameter of File::Temp's tempdir() function,
it's not clear how we could do that easily.<shot-in-the-dark>
Set cleanup to false and manually remove the directory later in the
code, so that stuff runs only if we haven't died sooner?</shot-in-the-dark>
Yeah, maybe. I'm thinking of trying to do it more globally, like in
src/Makefile.global.in. That way we wouldn't have to add new code to
every test file.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Aug 9, 2015 at 1:40 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
On 08/08/2015 09:31 AM, Robert Haas wrote:
On Fri, Aug 7, 2015 at 7:17 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:That certainly isn't what happens, and given the way this is done in
TestLib.pm, using the CLEANUP parameter of File::Temp's tempdir()
function,
it's not clear how we could do that easily.<shot-in-the-dark>
Set cleanup to false and manually remove the directory later in the
code, so that stuff runs only if we haven't died sooner?</shot-in-the-dark>
Yeah, maybe. I'm thinking of trying to do it more globally, like in
src/Makefile.global.in. That way we wouldn't have to add new code to every
test file.
If we rely on END to clean up the temporary data folder, there is no
need to impact each test file, just the test functions called in
TestLib.pm that could switch a flag to not perform any cleanup at the
end of the run should an unexpected result be found. Now I am as well
curious to see what you have in mind with manipulating
Makefile.global.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/09/2015 08:41 AM, Michael Paquier wrote:
On Sun, Aug 9, 2015 at 1:40 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
On 08/08/2015 09:31 AM, Robert Haas wrote:
On Fri, Aug 7, 2015 at 7:17 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:That certainly isn't what happens, and given the way this is done in
TestLib.pm, using the CLEANUP parameter of File::Temp's tempdir()
function,
it's not clear how we could do that easily.<shot-in-the-dark>
Set cleanup to false and manually remove the directory later in the
code, so that stuff runs only if we haven't died sooner?</shot-in-the-dark>
Yeah, maybe. I'm thinking of trying to do it more globally, like in
src/Makefile.global.in. That way we wouldn't have to add new code to every
test file.If we rely on END to clean up the temporary data folder, there is no
need to impact each test file, just the test functions called in
TestLib.pm that could switch a flag to not perform any cleanup at the
end of the run should an unexpected result be found. Now I am as well
curious to see what you have in mind with manipulating
Makefile.global.
See attached. If you have a better idea please post your patch.
cheers
andrew
Attachments:
fix-tap-test-work-removal.patchtext/x-diff; name=fix-tap-test-work-removal.patchDownload
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 39a7879..b379376 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -349,12 +349,12 @@ endef
ifeq ($(enable_tap_tests),yes)
define prove_installcheck
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl && rm -rf $(CURDIR)/tmp_test*
endef
define prove_check
rm -rf $(CURDIR)/tmp_check/log
-cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
+cd $(srcdir) && TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl && rm -rf $(CURDIR)/tmp_test*
endef
else
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 4927d45..174566c 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -113,7 +113,7 @@ sub tempdir
return File::Temp::tempdir(
'tmp_testXXXX',
DIR => $ENV{TESTDIR} || cwd(),
- CLEANUP => 1);
+ CLEANUP => 0);
}
sub tempdir_short
On Sun, Aug 9, 2015 at 11:19 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
On 08/09/2015 08:41 AM, Michael Paquier wrote:
On Sun, Aug 9, 2015 at 1:40 AM, Andrew Dunstan <andrew@dunslane.net>
wrote:On 08/08/2015 09:31 AM, Robert Haas wrote:
On Fri, Aug 7, 2015 at 7:17 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:That certainly isn't what happens, and given the way this is done in
TestLib.pm, using the CLEANUP parameter of File::Temp's tempdir()
function,
it's not clear how we could do that easily.<shot-in-the-dark>
Set cleanup to false and manually remove the directory later in the
code, so that stuff runs only if we haven't died sooner?</shot-in-the-dark>
Yeah, maybe. I'm thinking of trying to do it more globally, like in
src/Makefile.global.in. That way we wouldn't have to add new code to
every
test file.If we rely on END to clean up the temporary data folder, there is no
need to impact each test file, just the test functions called in
TestLib.pm that could switch a flag to not perform any cleanup at the
end of the run should an unexpected result be found. Now I am as well
curious to see what you have in mind with manipulating
Makefile.global.See attached. If you have a better idea please post your patch.
Sure. Attached is what I have in mind. Contrary to your version we
keep around temp paths should a run succeed after one that has failed
when running make check multiple times in a row. Perhaps it does not
matter much in practice as log files get removed at each new run but I
think that this allows a finer control in case of failure. Feel free
to have a look.
--
Michael
Attachments:
20150810_tap_temppaths.patchtext/x-diff; charset=US-ASCII; name=20150810_tap_temppaths.patchDownload
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 4927d45..11f774e 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -35,6 +35,7 @@ our @EXPORT = qw(
use Cwd;
use File::Basename;
+use File::Path qw(rmtree);
use File::Spec;
use File::Temp ();
use IPC::Run qw(run start);
@@ -107,21 +108,24 @@ $ENV{PGPORT} = int($ENV{PGPORT}) % 65536;
# Helper functions
#
+my $temp_cleanup = 1;
+my @temp_dirs = ();
sub tempdir
{
- return File::Temp::tempdir(
+ my $path = File::Temp::tempdir(
'tmp_testXXXX',
DIR => $ENV{TESTDIR} || cwd(),
- CLEANUP => 1);
+ CLEANUP => 0);
+ push(@temp_dirs, $path);
+ return $path;
}
sub tempdir_short
{
-
# Use a separate temp dir outside the build tree for the
# Unix-domain socket, to avoid file name length issues.
- return File::Temp::tempdir(CLEANUP => 1);
+ return File::Temp::tempdir(CLEANUP => 0);
}
# Initialize a new cluster for testing.
@@ -217,6 +221,13 @@ END
system_log('pg_ctl', '-D', $test_server_datadir, '-m',
'immediate', 'stop');
}
+
+ # Cleanup any temporary directory created
+ return if (!$temp_cleanup);
+ foreach my $temp_dir (@temp_dirs)
+ {
+ rmtree($temp_dir);
+ }
}
sub psql
@@ -278,14 +289,16 @@ sub command_ok
{
my ($cmd, $test_name) = @_;
my $result = run_log($cmd);
- ok($result, $test_name);
+ my $err_num = ok($result, $test_name);
+ $temp_cleanup = 0 if (!$err_num);
}
sub command_fails
{
my ($cmd, $test_name) = @_;
my $result = run_log($cmd);
- ok(!$result, $test_name);
+ my $err_num = ok(!$result, $test_name);
+ $temp_cleanup = 0 if (!$err_num);
}
sub command_exit_is
@@ -304,7 +317,8 @@ sub command_exit_is
# that, use $h->full_result on Windows instead.
my $result = ($Config{osname} eq "MSWin32") ?
($h->full_results)[0] : $h->result(0);
- is($result, $expected, $test_name);
+ my $err_num = is($result, $expected, $test_name);
+ $temp_cleanup = 0 if (!$err_num);
}
sub program_help_ok
@@ -313,9 +327,13 @@ sub program_help_ok
my ($stdout, $stderr);
print("# Running: $cmd --help\n");
my $result = run [ $cmd, '--help' ], '>', \$stdout, '2>', \$stderr;
- ok($result, "$cmd --help exit code 0");
- isnt($stdout, '', "$cmd --help goes to stdout");
- is($stderr, '', "$cmd --help nothing to stderr");
+ my $err_num;
+ $err_num = ok($result, "$cmd --help exit code 0");
+ $temp_cleanup = 0 if (!$err_num);
+ $err_num = isnt($stdout, '', "$cmd --help goes to stdout");
+ $temp_cleanup = 0 if (!$err_num);
+ $err_num = is($stderr, '', "$cmd --help nothing to stderr");
+ $temp_cleanup = 0 if (!$err_num);
}
sub program_version_ok
@@ -324,9 +342,13 @@ sub program_version_ok
my ($stdout, $stderr);
print("# Running: $cmd --version\n");
my $result = run [ $cmd, '--version' ], '>', \$stdout, '2>', \$stderr;
- ok($result, "$cmd --version exit code 0");
- isnt($stdout, '', "$cmd --version goes to stdout");
- is($stderr, '', "$cmd --version nothing to stderr");
+ my $err_num;
+ $err_num = ok($result, "$cmd --version exit code 0");
+ $temp_cleanup = 0 if (!$err_num);
+ $err_num = isnt($stdout, '', "$cmd --version goes to stdout");
+ $temp_cleanup = 0 if (!$err_num);
+ $err_num = is($stderr, '', "$cmd --version nothing to stderr");
+ $temp_cleanup = 0 if (!$err_num);
}
sub program_options_handling_ok
@@ -335,20 +357,27 @@ sub program_options_handling_ok
my ($stdout, $stderr);
print("# Running: $cmd --not-a-valid-option\n");
my $result = run [ $cmd, '--not-a-valid-option' ], '>', \$stdout, '2>',
- \$stderr;
- ok(!$result, "$cmd with invalid option nonzero exit code");
- isnt($stderr, '', "$cmd with invalid option prints error message");
+ \$stderr;
+ my $err_num;
+ $err_num = ok(!$result, "$cmd with invalid option nonzero exit code");
+ $temp_cleanup = 0 if (!$err_num);
+ $err_num = isnt($stderr, '', "$cmd with invalid option prints error message");
+ $temp_cleanup = 0 if (!$err_num);
}
sub command_like
{
my ($cmd, $expected_stdout, $test_name) = @_;
my ($stdout, $stderr);
+ my $err_num;
print("# Running: " . join(" ", @{$cmd}) . "\n");
my $result = run $cmd, '>', \$stdout, '2>', \$stderr;
- ok($result, "@$cmd exit code 0");
+ $err_num = ok($result, "@$cmd exit code 0");
+ $temp_cleanup = 0 if (!$err_num);
is($stderr, '', "@$cmd no stderr");
- like($stdout, $expected_stdout, "$test_name: matches");
+ $temp_cleanup = 0 if (!$err_num);
+ $err_num = like($stdout, $expected_stdout, "$test_name: matches");
+ $temp_cleanup = 0 if (!$err_num);
}
sub issues_sql_like
@@ -356,9 +385,12 @@ sub issues_sql_like
my ($cmd, $expected_sql, $test_name) = @_;
truncate $test_server_logfile, 0;
my $result = run_log($cmd);
- ok($result, "@$cmd exit code 0");
+ my $err_num;
+ $err_num = ok($result, "@$cmd exit code 0");
+ $temp_cleanup = 0 if (!$err_num);
my $log = slurp_file($test_server_logfile);
- like($log, $expected_sql, "$test_name: SQL found in server log");
+ $err_num = like($log, $expected_sql, "$test_name: SQL found in server log");
+ $temp_cleanup = 0 if (!$err_num);
}
1;
On 08/09/2015 08:58 PM, Michael Paquier wrote:
On Sun, Aug 9, 2015 at 11:19 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
On 08/09/2015 08:41 AM, Michael Paquier wrote:
On Sun, Aug 9, 2015 at 1:40 AM, Andrew Dunstan <andrew@dunslane.net>
wrote:On 08/08/2015 09:31 AM, Robert Haas wrote:
On Fri, Aug 7, 2015 at 7:17 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:That certainly isn't what happens, and given the way this is done in
TestLib.pm, using the CLEANUP parameter of File::Temp's tempdir()
function,
it's not clear how we could do that easily.<shot-in-the-dark>
Set cleanup to false and manually remove the directory later in the
code, so that stuff runs only if we haven't died sooner?</shot-in-the-dark>
Yeah, maybe. I'm thinking of trying to do it more globally, like in
src/Makefile.global.in. That way we wouldn't have to add new code to
every
test file.If we rely on END to clean up the temporary data folder, there is no
need to impact each test file, just the test functions called in
TestLib.pm that could switch a flag to not perform any cleanup at the
end of the run should an unexpected result be found. Now I am as well
curious to see what you have in mind with manipulating
Makefile.global.See attached. If you have a better idea please post your patch.
Sure. Attached is what I have in mind. Contrary to your version we
keep around temp paths should a run succeed after one that has failed
when running make check multiple times in a row. Perhaps it does not
matter much in practice as log files get removed at each new run but I
think that this allows a finer control in case of failure. Feel free
to have a look.
Actually, I don't think this is a very good idea. You could end up with
a whole series of opaquely named directories from a series of failing
runs. If you want to keep the directory after a failure, and want to do
another run, then rename the directory. That's what you have to do with
the main regression tests, too. My version also has the benefit of
changing exactly 3 lines in the source :-)
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Dunstan <andrew@dunslane.net> writes:
On 08/09/2015 08:58 PM, Michael Paquier wrote:
Sure. Attached is what I have in mind. Contrary to your version we
keep around temp paths should a run succeed after one that has failed
when running make check multiple times in a row. Perhaps it does not
matter much in practice as log files get removed at each new run but I
think that this allows a finer control in case of failure. Feel free
to have a look.
Actually, I don't think this is a very good idea. You could end up with
a whole series of opaquely named directories from a series of failing
runs. If you want to keep the directory after a failure, and want to do
another run, then rename the directory. That's what you have to do with
the main regression tests, too. My version also has the benefit of
changing exactly 3 lines in the source :-)
FWIW, I think we should keep the behavior of the TAP tests as close as
possible to the established behavior of the main regression tests.
It's certainly possible that there's room for improvement in that
benchmark behavior. But let's first converge the test behaviors,
and then have a discussion about whether we want changes, and if
so change all the tests at the same time.
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 08/10/2015 09:55 AM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
On 08/09/2015 08:58 PM, Michael Paquier wrote:
Sure. Attached is what I have in mind. Contrary to your version we
keep around temp paths should a run succeed after one that has failed
when running make check multiple times in a row. Perhaps it does not
matter much in practice as log files get removed at each new run but I
think that this allows a finer control in case of failure. Feel free
to have a look.Actually, I don't think this is a very good idea. You could end up with
a whole series of opaquely named directories from a series of failing
runs. If you want to keep the directory after a failure, and want to do
another run, then rename the directory. That's what you have to do with
the main regression tests, too. My version also has the benefit of
changing exactly 3 lines in the source :-)FWIW, I think we should keep the behavior of the TAP tests as close as
possible to the established behavior of the main regression tests.It's certainly possible that there's room for improvement in that
benchmark behavior. But let's first converge the test behaviors,
and then have a discussion about whether we want changes, and if
so change all the tests at the same time.
Yeah. To do that we should probably stop using File::Temp to make the
directory, and just use a hardcoded name given to File::Path::mkpath. If
the directory exists, we'd just remove it first.
That won't be a very big change - probably just a handful of lines.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/10/2015 10:32 AM, Andrew Dunstan wrote:
On 08/10/2015 09:55 AM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
On 08/09/2015 08:58 PM, Michael Paquier wrote:
Sure. Attached is what I have in mind. Contrary to your version we
keep around temp paths should a run succeed after one that has failed
when running make check multiple times in a row. Perhaps it does not
matter much in practice as log files get removed at each new run but I
think that this allows a finer control in case of failure. Feel free
to have a look.Actually, I don't think this is a very good idea. You could end up with
a whole series of opaquely named directories from a series of failing
runs. If you want to keep the directory after a failure, and want to do
another run, then rename the directory. That's what you have to do with
the main regression tests, too. My version also has the benefit of
changing exactly 3 lines in the source :-)FWIW, I think we should keep the behavior of the TAP tests as close as
possible to the established behavior of the main regression tests.It's certainly possible that there's room for improvement in that
benchmark behavior. But let's first converge the test behaviors,
and then have a discussion about whether we want changes, and if
so change all the tests at the same time.Yeah. To do that we should probably stop using File::Temp to make the
directory, and just use a hardcoded name given to File::Path::mkpath.
If the directory exists, we'd just remove it first.That won't be a very big change - probably just a handful of lines.
Unfortunately, it's rather more complicated. These tests are extremely
profligate with space and time, creating not less than 29 data
directories in 19 temporary directories, including 14 temp directories
in scripts alone, and not counting those in pg_rewind which puts two
more data directories in a place which is different from all the others.
And of course, in those directories (scripts and pg_ctl) that have
multiple temp directories, we have no idea which one goes with which set
of tests.
It's no wonder that these tests take an inordinate amount of time to run
- on crake, where it takes 23 seconds for "make installcheck" to run, it
takes 176 seconds for "make -C src/bin installcheck" to run. My bet is
all those initdb calls account for a substantial amount of that time.
I think this needs a significant bit of reworking.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers