On 6/26/23 22:47, Michael Paquier wrote:
> src/test/perl/README and regress.sgml both describe what
> PG_TEST_NOCLEAN does, and it seems to me that these should be updated
> to tell that temporary files are not removed on top of the data
> folders?
I've added a couple of quick lines to the docs in v2; see what you think.
On 6/26/23 23:10, Daniel Gustafsson wrote:
> I think it simply got lost in that thread which had a lot of moving
> parts as it was.
I'll make sure to register it for the CF. :D
On 6/27/23 08:20, Andrew Dunstan wrote:
> This doesn't look quite right. If PG_TEST_CLEAN had a value of 0 we
would still do the cleanup.
That's how it currently works for the data directories, but Dagfinn beat
me to the punch:
On 6/27/23 08:54, Dagfinn Ilmari Mannsåker wrote:
> However, the docs
> (https://www.postgresql.org/docs/16/regress-tap.html#REGRESS-TAP-VARS)
> say "If the environment variable PG_TEST_NOCLEAN is set", not "is set to
> a true value", and the existing test in PostgreSQL::Test::Cluster's END
> block is:
>
> # skip clean if we are requested to retain the basedir
> next if defined $ENV{'PG_TEST_NOCLEAN'};
>
> So the original `not defined` test is consistent with that.
Right. The second patch in v2 now changes that behavior across the
board, so we handle false values. I'm ambivalent on changing the wording
of the docs, but I can do that too if needed. (I'm pretty used to the
phrase "setting an environment variable" implying some sort of
true/false handling, when the envvar is a boolean toggle.)
Thanks all!
--JacobFrom 2f403265509b66879d2d3396a9da4a6ba0dded6d Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@timescale.com>
Date: Fri, 23 Jun 2023 13:30:20 -0700
Subject: [PATCH v2 1/2] Test::Utils: honor PG_TEST_NOCLEAN for tempdirs
---
doc/src/sgml/regress.sgml | 2 ++
src/test/perl/PostgreSQL/Test/Utils.pm | 6 +++---
src/test/perl/README | 5 +++--
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 88a43b8961..675db86e4d 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -818,6 +818,8 @@ make check PROVE_TESTS='t/001_test1.pl t/003_test3.pl'
<programlisting>
PG_TEST_NOCLEAN=1 make -C src/bin/pg_dump check
</programlisting>
+ This environment variable also prevents the test's temporary directories
+ from being removed.
</para>
<para>
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index a27fac83d2..e5a08d7b1a 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -272,7 +272,7 @@ sub all_tests_passing
Securely create a temporary directory inside C<$tmp_check>, like C<mkdtemp>,
and return its name. The directory will be removed automatically at the
-end of the tests.
+end of the tests, unless the PG_TEST_NOCLEAN envvar is provided.
If C<prefix> is given, the new directory is templated as C<${prefix}_XXXX>.
Otherwise the template is C<tmp_test_XXXX>.
@@ -286,7 +286,7 @@ sub tempdir
return File::Temp::tempdir(
$prefix . '_XXXX',
DIR => $tmp_check,
- CLEANUP => 1);
+ CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
}
=pod
@@ -301,7 +301,7 @@ name, to avoid path length issues.
sub tempdir_short
{
- return File::Temp::tempdir(CLEANUP => 1);
+ return File::Temp::tempdir(CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
}
=pod
diff --git a/src/test/perl/README b/src/test/perl/README
index 6ddee42a10..43f6431f6e 100644
--- a/src/test/perl/README
+++ b/src/test/perl/README
@@ -31,8 +31,9 @@ some lesser number of seconds.
Data directories will also be left behind for analysis when a test fails;
they are named according to the test filename. But if the environment
-variable PG_TEST_NOCLEAN is set, data directories will be retained
-regardless of test status.
+variable PG_TEST_NOCLEAN is set, those directories will be retained
+regardless of test status. This environment variable also prevents the
+test's temporary directories from being removed.
Writing tests
--
2.25.1
From 42299731884b939a90495d04fe3e8411d0a2a97d Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@timescale.com>
Date: Tue, 27 Jun 2023 08:43:19 -0700
Subject: [PATCH v2 2/2] test/perl: clean up when PG_TEST_NOCLEAN=0
Prior to this patch, setting PG_TEST_NOCLEAN=0 (or PG_TEST_NOCLEAN=)
still would have skipped the cleanup step.
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 2 +-
src/test/perl/PostgreSQL/Test/Utils.pm | 5 ++---
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 5e161dbee6..f48e935437 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1637,7 +1637,7 @@ END
$node->teardown_node(fail_ok => 1);
# skip clean if we are requested to retain the basedir
- next if defined $ENV{'PG_TEST_NOCLEAN'};
+ next if $ENV{PG_TEST_NOCLEAN};
# clean basedir on clean test invocation
$node->clean_node
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index e5a08d7b1a..a9aecf5aa9 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -286,7 +286,7 @@ sub tempdir
return File::Temp::tempdir(
$prefix . '_XXXX',
DIR => $tmp_check,
- CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
+ CLEANUP => not $ENV{PG_TEST_NOCLEAN});
}
=pod
@@ -300,8 +300,7 @@ name, to avoid path length issues.
sub tempdir_short
{
-
- return File::Temp::tempdir(CLEANUP => not defined $ENV{'PG_TEST_NOCLEAN'});
+ return File::Temp::tempdir(CLEANUP => not $ENV{PG_TEST_NOCLEAN});
}
=pod
--
2.25.1
