pg_upgrade test writes to source directory
The 002_pg_upgrade.pl test leaves a file delete_old_cluster.sh in the
source directory. In vpath builds, there shouldn't be any files written
to the source directory.
Note that the TAP tests run with the source directory as the current
directory, so this is the result of pg_upgrade leaving its output files
in the current directory.
It looks like an addition of
chdir $ENV{TESTOUTDIR};
could fix it. Please check the patch.
Attachments:
0001-Run-pg_upgrade-test-in-build-directory.patchtext/plain; charset=UTF-8; name=0001-Run-pg_upgrade-test-in-build-directory.patchDownload
From c064fc9a78f301d840cf608d2f5b997ad1d37a28 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 25 May 2022 08:18:44 +0200
Subject: [PATCH] Run pg_upgrade test in build directory
In passing, add names to tests that didn't have any yet.
---
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 75ac768a96..114e5fbcb6 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -2,7 +2,7 @@
use strict;
use warnings;
-use Cwd qw(abs_path getcwd);
+use Cwd qw(abs_path);
use File::Basename qw(dirname);
use File::Compare;
@@ -10,6 +10,8 @@
use PostgreSQL::Test::Utils;
use Test::More;
+chdir $ENV{TESTOUTDIR};
+
# Generate a database with a name made of a range of ASCII characters.
sub generate_db
{
@@ -23,7 +25,7 @@ sub generate_db
}
$dbname .= $suffix;
- $node->command_ok([ 'createdb', $dbname ]);
+ $node->command_ok([ 'createdb', $dbname ], 'created database');
}
# The test of pg_upgrade requires two clusters, an old one and a new one
@@ -71,7 +73,7 @@ sub generate_db
# Load the dump using the "postgres" database as "regression" does
# not exist yet, and we are done here.
- $oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ]);
+ $oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ], 'loaded old dump file');
}
else
{
@@ -136,7 +138,8 @@ sub generate_db
'psql', '-X',
'-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
'regression'
- ]);
+ ],
+ 'ran adapt script');
}
# Initialize a new node for the upgrade.
@@ -233,7 +236,8 @@ sub generate_db
'pg_dumpall', '--no-sync',
'-d', $newnode->connstr('postgres'),
'-f', "$tempdir/dump2.sql"
- ]);
+ ],
+ 'dump before running pg_upgrade');
# Compare the two dumps, there should be no differences.
my $compare_res = compare("$tempdir/dump1.sql", "$tempdir/dump2.sql");
--
2.36.1
On Wed, May 25, 2022 at 08:21:26AM +0200, Peter Eisentraut wrote:
The 002_pg_upgrade.pl test leaves a file delete_old_cluster.sh in the source
directory. In vpath builds, there shouldn't be any files written to the
source directory.Note that the TAP tests run with the source directory as the current
directory, so this is the result of pg_upgrade leaving its output files in
the current directory.
Good catch, thanks.
It looks like an addition of
chdir $ENV{TESTOUTDIR};
could fix it. Please check the patch.
I think that you mean TESTDIR, and not TESTOUTDIR? Doing a chdir at
the beginning of the tests would cause pg_regress to fail as we would
not find anymore the regression schedule in a VPATH build, but it is
possible to chdir before the execution of pg_upgrade, like the
attached.
--
Michael
Attachments:
v2-0001-Run-pg_upgrade-test-in-build-directory.patchtext/x-diff; charset=us-asciiDownload
From 719b25ee7182340ae37c126ca7c6e392db2465cc Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 25 May 2022 16:24:23 +0900
Subject: [PATCH v2] Run pg_upgrade test in build directory
In passing, add names to tests that didn't have any yet.
---
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 75ac768a96..117514e38f 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -2,7 +2,7 @@
use strict;
use warnings;
-use Cwd qw(abs_path getcwd);
+use Cwd qw(abs_path);
use File::Basename qw(dirname);
use File::Compare;
@@ -23,7 +23,7 @@ sub generate_db
}
$dbname .= $suffix;
- $node->command_ok([ 'createdb', $dbname ]);
+ $node->command_ok([ 'createdb', $dbname ], 'created database');
}
# The test of pg_upgrade requires two clusters, an old one and a new one
@@ -71,7 +71,8 @@ if (defined($ENV{olddump}))
# Load the dump using the "postgres" database as "regression" does
# not exist yet, and we are done here.
- $oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ]);
+ $oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ],
+ 'loaded old dump file');
}
else
{
@@ -136,7 +137,8 @@ if (defined($ENV{oldinstall}))
'psql', '-X',
'-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
'regression'
- ]);
+ ],
+ 'ran adapt script');
}
# Initialize a new node for the upgrade.
@@ -202,6 +204,14 @@ if (defined($ENV{oldinstall}))
}
}
+# In a VPATH build, we'll be started in the source directory, but we want
+# to run pg_upgrade in the build directory so that any files generated finish
+# in it, like delete_old_cluster.sh.
+if ($ENV{TESTDIR})
+{
+ chdir $ENV{TESTDIR} or die "could not chdir to \"$ENV{TESTDIR}\": $!";
+}
+
# Upgrade the instance.
$oldnode->stop;
command_ok(
@@ -233,7 +243,8 @@ $newnode->command_ok(
'pg_dumpall', '--no-sync',
'-d', $newnode->connstr('postgres'),
'-f', "$tempdir/dump2.sql"
- ]);
+ ],
+ 'dump before running pg_upgrade');
# Compare the two dumps, there should be no differences.
my $compare_res = compare("$tempdir/dump1.sql", "$tempdir/dump2.sql");
--
2.36.1
On 25.05.22 09:25, Michael Paquier wrote:
It looks like an addition of
chdir $ENV{TESTOUTDIR};
could fix it. Please check the patch.
I think that you mean TESTDIR, and not TESTOUTDIR?
I chose TESTOUTDIR because it corresponds to the tmp_check directory, so
that the output files of the pg_upgrade run are removed when the test
artifacts are cleaned up. When using TESTDIR, the pg_upgrade output
files end up in the build directory, which is less bad than the source
directory, but still not ideal.
On Thu, May 26, 2022 at 04:36:47PM +0200, Peter Eisentraut wrote:
I chose TESTOUTDIR because it corresponds to the tmp_check directory, so
that the output files of the pg_upgrade run are removed when the test
artifacts are cleaned up. When using TESTDIR, the pg_upgrade output files
end up in the build directory, which is less bad than the source directory,
but still not ideal.
Where does the choice of TESTOUTDIR come from? I am a bit surprised
by this choice, to be honest, because there is no trace of it in the
buildfarm client or the core code. TESTDIR, on the other hand, points
to tmp_check/ if not set. It gets set it in vcregress.pl and
Makefile.global.in.
--
Michael
On Fri, May 27, 2022 at 05:43:04AM +0900, Michael Paquier wrote:
On Thu, May 26, 2022 at 04:36:47PM +0200, Peter Eisentraut wrote:
I chose TESTOUTDIR because it corresponds to the tmp_check directory, so
that the output files of the pg_upgrade run are removed when the test
artifacts are cleaned up. When using TESTDIR, the pg_upgrade output files
end up in the build directory, which is less bad than the source directory,
but still not ideal.Where does the choice of TESTOUTDIR come from? I am a bit surprised
by this choice, to be honest, because there is no trace of it in the
buildfarm client or the core code. TESTDIR, on the other hand, points
to tmp_check/ if not set. It gets set it in vcregress.pl and
Makefile.global.in.
It looks like Peter working on top of the meson branch.
TESTOUTDIR is not yet in master.
https://commitfest.postgresql.org/38/3395/
https://github.com/anarazel/postgres/tree/meson
https://github.com/anarazel/postgres/commit/e754bde6d0d3cb6329a5bf568e19eb271c3bdc7c
commit e754bde6d0d3cb6329a5bf568e19eb271c3bdc7c
Author: Andres Freund <andres@anarazel.de>
Date: Mon Feb 14 21:47:07 2022 -0800
wip: split TESTDIR into two.
On Thu, May 26, 2022 at 03:52:18PM -0500, Justin Pryzby wrote:
It looks like Peter working on top of the meson branch.
TESTOUTDIR is not yet in master.
Thanks for the reference. I didn't know this part of the puzzle.
https://commitfest.postgresql.org/38/3395/
https://github.com/anarazel/postgres/tree/meson
https://github.com/anarazel/postgres/commit/e754bde6d0d3cb6329a5bf568e19eb271c3bdc7ccommit e754bde6d0d3cb6329a5bf568e19eb271c3bdc7c
Author: Andres Freund <andres@anarazel.de>
Date: Mon Feb 14 21:47:07 2022 -0800wip: split TESTDIR into two.
Well, we need to do something about that on HEAD, and it also means
that TESTDIR is the best fit for the job now, except if the variable
split happens before REL_15_STABLE is forked.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Thu, May 26, 2022 at 03:52:18PM -0500, Justin Pryzby wrote:
It looks like Peter working on top of the meson branch.
TESTOUTDIR is not yet in master.
Well, we need to do something about that on HEAD, and it also means
that TESTDIR is the best fit for the job now, except if the variable
split happens before REL_15_STABLE is forked.
It looks like that patch is meant to resolve misbehaviors equivalent to
this one that already exist in several other places. So fixing this
one along with the other ones seems like an appropriate thing to do
when that lands.
regards, tom lane
On Thu, May 26, 2022 at 06:19:56PM -0400, Tom Lane wrote:
It looks like that patch is meant to resolve misbehaviors equivalent to
this one that already exist in several other places. So fixing this
one along with the other ones seems like an appropriate thing to do
when that lands.
Well, would this specific change land in REL_15_STABLE? From what I
can see, generating delete_old_cluster.sh in the source rather than
the build directory is a defect from 322becb, as test.sh issues
pg_upgrade from the build path in ~14, but we do it from the source
path to get an access to parallel_schedule.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Thu, May 26, 2022 at 06:19:56PM -0400, Tom Lane wrote:
It looks like that patch is meant to resolve misbehaviors equivalent to
this one that already exist in several other places. So fixing this
one along with the other ones seems like an appropriate thing to do
when that lands.
Well, would this specific change land in REL_15_STABLE?
I wouldn't object to doing that, and even back-patching. It looked
like a pretty sane change, and we've learned before that skimping on
back-branch test infrastructure is a poor tradeoff.
regards, tom lane
On Thu, May 26, 2022 at 07:51:08PM -0400, Tom Lane wrote:
I wouldn't object to doing that, and even back-patching. It looked
like a pretty sane change, and we've learned before that skimping on
back-branch test infrastructure is a poor tradeoff.
Okay, fine by me. Andres, what do you think about backpatching [1]https://github.com/anarazel/postgres/commit/e754bde6d0d3cb6329a5bf568e19eb271c3bdc7c -- Michael?
[1]: https://github.com/anarazel/postgres/commit/e754bde6d0d3cb6329a5bf568e19eb271c3bdc7c -- Michael
--
Michael
On 26.05.22 22:52, Justin Pryzby wrote:
On Fri, May 27, 2022 at 05:43:04AM +0900, Michael Paquier wrote:
On Thu, May 26, 2022 at 04:36:47PM +0200, Peter Eisentraut wrote:
I chose TESTOUTDIR because it corresponds to the tmp_check directory, so
that the output files of the pg_upgrade run are removed when the test
artifacts are cleaned up. When using TESTDIR, the pg_upgrade output files
end up in the build directory, which is less bad than the source directory,
but still not ideal.Where does the choice of TESTOUTDIR come from? I am a bit surprised
by this choice, to be honest, because there is no trace of it in the
buildfarm client or the core code. TESTDIR, on the other hand, points
to tmp_check/ if not set. It gets set it in vcregress.pl and
Makefile.global.in.It looks like Peter working on top of the meson branch.
TESTOUTDIR is not yet in master.
Ooops, yeah. :)
I think you can just chdir to ${PostgreSQL::Test::Utils::tmp_check}.
On Fri, May 27, 2022 at 02:45:57PM +0200, Peter Eisentraut wrote:
I think you can just chdir to ${PostgreSQL::Test::Utils::tmp_check}.
Hmm. I think that I prefer your initial suggestion with TESTOUTDIR.
This sticks better in the long term, while making things consistent
with 010_tab_completion.pl, the only test that moves to TESTDIR while
running. So my vote would be to backpatch first the addition of
TESTOUTDIR, then fix the TAP test of pg_upgrade on HEAD to do the
same.
And I have just noticed that I completely forgot to add Andres about
this specific point, as meson is his work. So done now.
--
Michael
On 28.05.22 10:56, Michael Paquier wrote:
On Fri, May 27, 2022 at 02:45:57PM +0200, Peter Eisentraut wrote:
I think you can just chdir to ${PostgreSQL::Test::Utils::tmp_check}.
Hmm. I think that I prefer your initial suggestion with TESTOUTDIR.
This sticks better in the long term, while making things consistent
with 010_tab_completion.pl, the only test that moves to TESTDIR while
running. So my vote would be to backpatch first the addition of
TESTOUTDIR, then fix the TAP test of pg_upgrade on HEAD to do the
same.
I think it's a bit premature to talk about backpatching, since the patch
in question hasn't been committed anywhere yet, and AFAICT hasn't even
really been reviewed yet.
If you want to go this direction, I suggest you extract the patch and
present it here on its own merit. -- But then I might ask why such a
broad change post beta when apparently a one-line change would also work.
Hi,
On 2022-05-27 09:05:43 +0900, Michael Paquier wrote:
On Thu, May 26, 2022 at 07:51:08PM -0400, Tom Lane wrote:
I wouldn't object to doing that, and even back-patching. It looked
like a pretty sane change, and we've learned before that skimping on
back-branch test infrastructure is a poor tradeoff.Okay, fine by me. Andres, what do you think about backpatching [1]?
[1]: https://github.com/anarazel/postgres/commit/e754bde6d0d3cb6329a5bf568e19eb271c3bdc7c
Well, committing and backpatching ;)
I suspect there might be a bit more polish might be needed - that's why I
hadn't proposed the commit on its own yet. I was also wondering about
proposing a different split (test data, test logs).
I don't even know if we still need TESTDIR - since f4ce6c4d3a3 we add the
build dir to PATH, which IIUC was the reason for TESTDIR previously. Afaics
after f4ce6c4d3a3 and the TESTOUTDIR split the only TESTDIR use is in
src/tools/msvc/ecpg_regression.proj - so we could at least restrict it to
that.
Stuff I noticed on a quick skim:
# In a VPATH build, we'll be started in the source directory, but we want
# to run in the build directory so that we can use relative paths to
# access the tmp_check subdirectory; otherwise the output from filename
# completion tests is too variable.
Just needs a bit of rephrasing.
# Determine output directories, and create them. The base path is the
# TESTDIR environment variable, which is normally set by the invoking
# Makefile.
$tmp_check = $ENV{TESTOUTDIR} ? "$ENV{TESTOUTDIR}" : "tmp_check";
$log_path = "$tmp_check/log";
Probably just needs a s/TESTDIR/TESTOUTDIR/
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
I suspect there might be a bit more polish might be needed - that's why I
hadn't proposed the commit on its own yet.
Yeah, I'd noticed the obsoleted comments too, but not bothered to complain
since that was just WIP and not an officially proposed patch. I'll be
happy to review if you want to put up a full patch.
I was also wondering about
proposing a different split (test data, test logs).
Might be too invasive for back-patch.
regards, tom lane
On Sat, May 28, 2022 at 04:14:01PM -0400, Tom Lane wrote:
Yeah, I'd noticed the obsoleted comments too, but not bothered to complain
since that was just WIP and not an officially proposed patch. I'll be
happy to review if you want to put up a full patch.
Well, here is a formal patch set, then. Please feel free to comment.
FWIW, I am on the fence with dropping TESTDIR, as it could be used by
out-of-core test code as well. If there are doubts about
back-patching the first part, doing that only on HEAD would be fine to
fix the problem of this thread.
--
Michael
Attachments:
v3-0001-Add-TESTOUTDIR-as-directory-for-the-output-of-the.patchtext/x-diff; charset=us-asciiDownload
From 3e6b833deafdfafed949abae49f171c8def689dc Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 31 May 2022 15:44:49 +0900
Subject: [PATCH v3 1/2] Add TESTOUTDIR as directory for the output of the TAP
tests
TESTDIR is the directory of the build, while TESTOUTDIR would be
generally $TESTDIR/tmp_check/.
---
src/bin/psql/t/010_tab_completion.pl | 40 +++++++++++++-------------
src/test/perl/PostgreSQL/Test/Utils.pm | 4 +--
src/Makefile.global.in | 11 ++++---
src/tools/msvc/vcregress.pl | 2 ++
4 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index 2eea515e87..1f41ce47e6 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -67,26 +67,26 @@ delete $ENV{TERM};
delete $ENV{LS_COLORS};
# In a VPATH build, we'll be started in the source directory, but we want
-# to run in the build directory so that we can use relative paths to
-# access the tmp_check subdirectory; otherwise the output from filename
-# completion tests is too variable.
-if ($ENV{TESTDIR})
+# to run in the test output directory so that we can use relative paths from
+# the tmp_check subdirectory; otherwise the output from filename completion
+# tests is too variable.
+if ($ENV{TESTOUTDIR})
{
- chdir $ENV{TESTDIR} or die "could not chdir to \"$ENV{TESTDIR}\": $!";
+ chdir "$ENV{TESTOUTDIR}" or die "could not chdir to \"$ENV{TESTOUTDIR}\": $!";
}
# Create some junk files for filename completion testing.
my $FH;
-open $FH, ">", "tmp_check/somefile"
- or die("could not create file \"tmp_check/somefile\": $!");
+open $FH, ">", "somefile"
+ or die("could not create file \"somefile\": $!");
print $FH "some stuff\n";
close $FH;
-open $FH, ">", "tmp_check/afile123"
- or die("could not create file \"tmp_check/afile123\": $!");
+open $FH, ">", "afile123"
+ or die("could not create file \"afile123\": $!");
print $FH "more stuff\n";
close $FH;
-open $FH, ">", "tmp_check/afile456"
- or die("could not create file \"tmp_check/afile456\": $!");
+open $FH, ">", "afile456"
+ or die("could not create file \"afile456\": $!");
print $FH "other stuff\n";
close $FH;
@@ -272,16 +272,16 @@ clear_query();
# check filename completion
check_completion(
- "\\lo_import tmp_check/some\t",
- qr|tmp_check/somefile |,
+ "\\lo_import some\t",
+ qr|somefile |,
"filename completion with one possibility");
clear_query();
# note: readline might print a bell before the completion
check_completion(
- "\\lo_import tmp_check/af\t",
- qr|tmp_check/af\a?ile|,
+ "\\lo_import af\t",
+ qr|af\a?ile|,
"filename completion with multiple possibilities");
# broken versions of libedit require clear_line not clear_query here
@@ -291,15 +291,15 @@ clear_line();
# note: broken versions of libedit want to backslash the closing quote;
# not much we can do about that
check_completion(
- "COPY foo FROM tmp_check/some\t",
- qr|'tmp_check/somefile\\?' |,
+ "COPY foo FROM some\t",
+ qr|'somefile\\?' |,
"quoted filename completion with one possibility");
clear_line();
check_completion(
- "COPY foo FROM tmp_check/af\t",
- qr|'tmp_check/afile|,
+ "COPY foo FROM af\t",
+ qr|'afile|,
"quoted filename completion with multiple possibilities");
# some versions of readline/libedit require two tabs here, some only need one
@@ -307,7 +307,7 @@ check_completion(
# the quotes might appear, too
check_completion(
"\t\t",
- qr|afile123'? +'?(tmp_check/)?afile456|,
+ qr|afile123'? +'?afile456|,
"offer multiple file choices");
clear_line();
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 1ca2cc5917..6cd3ff2caf 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -190,9 +190,9 @@ INIT
$SIG{PIPE} = 'IGNORE';
# Determine output directories, and create them. The base path is the
- # TESTDIR environment variable, which is normally set by the invoking
+ # TESTOUTDUR environment variable, which is normally set by the invoking
# Makefile.
- $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}/tmp_check" : "tmp_check";
+ $tmp_check = $ENV{TESTOUTDIR} ? $ENV{TESTOUTDIR} : "tmp_check";
$log_path = "$tmp_check/log";
mkdir $tmp_check;
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 051718e4fe..4a4e1bd4c9 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -452,7 +452,8 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTOUTDIR='$(CURDIR)/tmp_check' TESTDIR='$(CURDIR)' \
+ PATH="$(bindir):$(CURDIR):$$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) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -463,8 +464,9 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' PATH="$(bindir):$(CURDIR):$$PATH" \
- PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
+ TESTOUTDIR='$(CURDIR)/tmp_check' TESTDIR='$(CURDIR)' \
+ PATH="$(bindir):$(CURDIR):$$PATH" PGPORT='6$(DEF_PGPORT)' \
+ top_builddir='$(top_builddir)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
@@ -475,7 +477,8 @@ echo "+++ tap check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
+ TESTOUTDIR='$(CURDIR)/tmp_check' 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) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index c3729f6be5..6f07a31464 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -296,6 +296,8 @@ sub tap_check
# add the module build dir as the second element in the PATH
$ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
+ $ENV{TESTOUTDIR} = "$dir/tmp_check";
+
rmtree('tmp_check');
system(@args);
my $status = $? >> 8;
--
2.36.1
v3-0002-Run-pg_upgrade-test-in-test-output-directory-as-o.patchtext/x-diff; charset=us-asciiDownload
From 0de7e32dbb79fda17384b480452f9b26b22e0ccb Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 31 May 2022 16:01:46 +0900
Subject: [PATCH v3 2/2] Run pg_upgrade test in test output directory, as of
TESTOUTDIR
In passing, add names to tests that didn't have any yet.
---
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 75ac768a96..cb685d5d73 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -2,7 +2,7 @@
use strict;
use warnings;
-use Cwd qw(abs_path getcwd);
+use Cwd qw(abs_path);
use File::Basename qw(dirname);
use File::Compare;
@@ -23,7 +23,9 @@ sub generate_db
}
$dbname .= $suffix;
- $node->command_ok([ 'createdb', $dbname ]);
+ $node->command_ok(
+ [ 'createdb', $dbname ],
+ "created database with ASCII characters from $from_char to $to_char");
}
# The test of pg_upgrade requires two clusters, an old one and a new one
@@ -71,7 +73,8 @@ if (defined($ENV{olddump}))
# Load the dump using the "postgres" database as "regression" does
# not exist yet, and we are done here.
- $oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ]);
+ $oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ],
+ 'loaded old dump file');
}
else
{
@@ -136,7 +139,8 @@ if (defined($ENV{oldinstall}))
'psql', '-X',
'-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
'regression'
- ]);
+ ],
+ 'ran adapt script');
}
# Initialize a new node for the upgrade.
@@ -202,6 +206,15 @@ if (defined($ENV{oldinstall}))
}
}
+# In a VPATH build, we'll be started in the source directory, but we want
+# to run pg_upgrade in the test output directory so that any files generated
+# finish in it, like delete_old_cluster.sh.
+if ($ENV{TESTOUTDIR})
+{
+ chdir $ENV{TESTOUTDIR}
+ or die "could not chdir to \"$ENV{TESTOUTDIR}\": $!";
+}
+
# Upgrade the instance.
$oldnode->stop;
command_ok(
@@ -233,7 +246,8 @@ $newnode->command_ok(
'pg_dumpall', '--no-sync',
'-d', $newnode->connstr('postgres'),
'-f', "$tempdir/dump2.sql"
- ]);
+ ],
+ 'dump before running pg_upgrade');
# Compare the two dumps, there should be no differences.
my $compare_res = compare("$tempdir/dump1.sql", "$tempdir/dump2.sql");
--
2.36.1
On 31.05.22 09:17, Michael Paquier wrote:
On Sat, May 28, 2022 at 04:14:01PM -0400, Tom Lane wrote:
Yeah, I'd noticed the obsoleted comments too, but not bothered to complain
since that was just WIP and not an officially proposed patch. I'll be
happy to review if you want to put up a full patch.Well, here is a formal patch set, then. Please feel free to comment.
FWIW, I am on the fence with dropping TESTDIR, as it could be used by
out-of-core test code as well. If there are doubts about
back-patching the first part, doing that only on HEAD would be fine to
fix the problem of this thread.
I don't understand the point of this first patch at all. Why define
TESTOUTDIR as a separate variable if it's always TESTDIR + tmp_check?
Why define TESTOUTDIR in pg_regress invocations, if nothing uses it? If
you want it as a separate variable, it could be defined in some Per
utility module, but I don't see why it needs to be in Makefile.global.
What is the problem that this is trying to solve?
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
On 31.05.22 09:17, Michael Paquier wrote:
Well, here is a formal patch set, then. Please feel free to comment.
FWIW, I am on the fence with dropping TESTDIR, as it could be used by
out-of-core test code as well. If there are doubts about
back-patching the first part, doing that only on HEAD would be fine to
fix the problem of this thread.
I don't understand the point of this first patch at all. Why define
TESTOUTDIR as a separate variable if it's always TESTDIR + tmp_check?
Why define TESTOUTDIR in pg_regress invocations, if nothing uses it? If
you want it as a separate variable, it could be defined in some Per
utility module, but I don't see why it needs to be in Makefile.global.
What is the problem that this is trying to solve?
Yeah, after looking this over it seems like we could drop 0001 and
just change 0002 to chdir into TESTDIR then into tmp_check. I'm not
sure I see the point of inventing a new global variable either,
and I'm definitely not happy with the proposed changes to
010_tab_completion.pl. My recollection is that those tests
were intentionally written to test tab completion involving a
directory name, but this change just loses that aspect entirely.
regards, tom lane
Hi,
On 2022-06-01 16:11:16 +0200, Peter Eisentraut wrote:
On 31.05.22 09:17, Michael Paquier wrote:
On Sat, May 28, 2022 at 04:14:01PM -0400, Tom Lane wrote:
Yeah, I'd noticed the obsoleted comments too, but not bothered to complain
since that was just WIP and not an officially proposed patch. I'll be
happy to review if you want to put up a full patch.Well, here is a formal patch set, then. Please feel free to comment.
FWIW, I am on the fence with dropping TESTDIR, as it could be used by
out-of-core test code as well. If there are doubts about
back-patching the first part, doing that only on HEAD would be fine to
fix the problem of this thread.I don't understand the point of this first patch at all. Why define
TESTOUTDIR as a separate variable if it's always TESTDIR + tmp_check? Why
define TESTOUTDIR in pg_regress invocations, if nothing uses it? If you
want it as a separate variable, it could be defined in some Per utility
module, but I don't see why it needs to be in Makefile.global. What is the
problem that this is trying to solve?
Until recently TESTDIR needed to point to the build directory containing the
binaries. But I'd like to be able to separate test log output from the build
tree, so that it's easier to capture files generated by tests for CI /
buildfarm. The goal is to have a separate directory for each test, so we can
present logs for failed tests separately. That was impossible with TESTDIR,
because it needed to point to the build directory.
Greetings,
Andres Freund
On Wed, Jun 01, 2022 at 02:11:12PM -0700, Andres Freund wrote:
Until recently TESTDIR needed to point to the build directory containing the
binaries. But I'd like to be able to separate test log output from the build
tree, so that it's easier to capture files generated by tests for CI /
buildfarm. The goal is to have a separate directory for each test, so we can
present logs for failed tests separately. That was impossible with TESTDIR,
because it needed to point to the build directory.
FWIW, this argument sounds sensible to me since I looked at 0001, not
only for the log files, but also to help in the capture of files
generated by the tests like 010_tab_completion.pl.
I don't know yet what to do about this part, so for now I have fixed
the other issue reported by Peter where the test names were missing.
--
Michael
On 2022-06-01 We 20:37, Michael Paquier wrote:
On Wed, Jun 01, 2022 at 02:11:12PM -0700, Andres Freund wrote:
Until recently TESTDIR needed to point to the build directory containing the
binaries. But I'd like to be able to separate test log output from the build
tree, so that it's easier to capture files generated by tests for CI /
buildfarm. The goal is to have a separate directory for each test, so we can
present logs for failed tests separately. That was impossible with TESTDIR,
because it needed to point to the build directory.FWIW, this argument sounds sensible to me since I looked at 0001, not
only for the log files, but also to help in the capture of files
generated by the tests like 010_tab_completion.pl.I don't know yet what to do about this part, so for now I have fixed
the other issue reported by Peter where the test names were missing.
I hope we fix the original issue soon - it's apparently been the cause
of numerous buildfarm failures that it was on my list to investigate
e.g.
<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2022-05-15%2019%3A24%3A27>
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, Jun 02, 2022 at 05:17:34PM -0400, Andrew Dunstan wrote:
I hope we fix the original issue soon - it's apparently been the cause
of numerous buildfarm failures that it was on my list to investigate
e.g.
<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2022-05-15%2019%3A24%3A27>
Oops. Thanks, Andrew, I was not aware of that. I don't really want
to wait more if this impacts some of the buildfarm animals. Even if
we don't conclude with the use of TESTOUTDIR for the time being, I see
no strong objections in using TESTDIR/tmp_check, aka
${PostgreSQL::Test::Utils::tmp_check}. So I propose to apply a fix
doing that in the next 24 hours or so. We can always switch to a
different path once we decide something else, if necessary.
--
Michael
On 2022-06-03 12:29:04 +0900, Michael Paquier wrote:
On Thu, Jun 02, 2022 at 05:17:34PM -0400, Andrew Dunstan wrote:
I hope we fix the original issue soon - it's apparently been the cause
of numerous buildfarm failures that it was on my list to investigate
e.g.
<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2022-05-15%2019%3A24%3A27>Oops. Thanks, Andrew, I was not aware of that. I don't really want
to wait more if this impacts some of the buildfarm animals. Even if
we don't conclude with the use of TESTOUTDIR for the time being, I see
no strong objections in using TESTDIR/tmp_check, aka
${PostgreSQL::Test::Utils::tmp_check}. So I propose to apply a fix
doing that in the next 24 hours or so. We can always switch to a
different path once we decide something else, if necessary.
+1
Greetings,
Andres Freund
On Thu, Jun 02, 2022 at 08:50:12PM -0700, Andres Freund wrote:
+1
OK, applied the extra chdir to PostgreSQL::Test::Utils::tmp_check, as
initially suggested by Peter. The CI looked happy on that.
--
Michael
Hi,
On 2022-06-01 10:55:28 -0400, Tom Lane wrote:
[...] I'm definitely not happy with the proposed changes to
010_tab_completion.pl. My recollection is that those tests
were intentionally written to test tab completion involving a
directory name, but this change just loses that aspect entirely.
How about creating a dedicated directory for the created files, to maintain
that? My goal of being able to redirect the test output elsewhere can be
achieved with just a hunk like this:
@@ -70,11 +70,13 @@ delete $ENV{LS_COLORS};
# to run in the build directory so that we can use relative paths to
# access the tmp_check subdirectory; otherwise the output from filename
# completion tests is too variable.
-if ($ENV{TESTDIR})
+if ($ENV{TESTOUTDIR})
{
- chdir $ENV{TESTDIR} or die "could not chdir to \"$ENV{TESTDIR}\": $!";
+ chdir "$ENV{TESTOUTDIR}" or die "could not chdir to \"$ENV{TESTOUTDIR}\": $!";
}+mkdir "tmp_check" unless -d "tmp_check";
+
# Create some junk files for filename completion testing.
my $FH;
open $FH, ">", "tmp_check/somefile"Of course it'd need a comment adjustment etc. It's a bit ugly to use a
otherwise empty tmp_check/ directory just to reduce the diff size, but it's
also not too bad.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2022-06-01 10:55:28 -0400, Tom Lane wrote:
[...] I'm definitely not happy with the proposed changes to
010_tab_completion.pl. My recollection is that those tests
were intentionally written to test tab completion involving a
directory name, but this change just loses that aspect entirely.
How about creating a dedicated directory for the created files, to maintain
that? My goal of being able to redirect the test output elsewhere can be
achieved with just a hunk like this:
Sure, there's no need for these files to be in the exact same place that
the output is collected. I just want to keep their same relationship
to the test's CWD.
Of course it'd need a comment adjustment etc. It's a bit ugly to use a
otherwise empty tmp_check/ directory just to reduce the diff size, but it's
also not too bad.
Given that it's no longer going to be the same tmp_check dir used
elsewhere, maybe we could s/tmp_check/tab_comp_dir/g or something
like that? That'd add some clarity I think.
regards, tom lane
Hi,
On 2022-08-11 11:26:39 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2022-06-01 10:55:28 -0400, Tom Lane wrote:
[...] I'm definitely not happy with the proposed changes to
010_tab_completion.pl. My recollection is that those tests
were intentionally written to test tab completion involving a
directory name, but this change just loses that aspect entirely.How about creating a dedicated directory for the created files, to maintain
that? My goal of being able to redirect the test output elsewhere can be
achieved with just a hunk like this:Sure, there's no need for these files to be in the exact same place that
the output is collected. I just want to keep their same relationship
to the test's CWD.Of course it'd need a comment adjustment etc. It's a bit ugly to use a
otherwise empty tmp_check/ directory just to reduce the diff size, but it's
also not too bad.Given that it's no longer going to be the same tmp_check dir used
elsewhere, maybe we could s/tmp_check/tab_comp_dir/g or something
like that? That'd add some clarity I think.
Done in the attached patch (0001).
A bunch of changes (e.g. f4ce6c4d3a3) made since I'd first written that
TESTOUTDIR patch means that we don't need two different variables anymore. So
patch 0002 just moves the addition of /tmp_check from Utils.pm to the places
in which TESTDIR is defined.
That still "forces" tmp_check/ to exist when going through pg_regress, but
that's less annoying because pg_regress at least keeps
regression.{diffs,out}/log files/directory outside of tmp_check/.
I've also attached a 0003 that splits the log location from the data
location. That could be used to make the log file location symmetrical between
pg_regress (log/) and tap tests (tmp_check/log). But it'd break the
buildfarm's tap test log file collection, so I don't think that's something we
really can do soon-ish?
Greetings,
Andres Freund
Attachments:
0001-Create-test-files-for-010_tab_completion.pl-in-dedic.patchtext/x-diff; charset=us-asciiDownload
From eebc08f6628b15e0e8cc1a84dec9bbbd9931696b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 11 Aug 2022 08:57:58 -0700
Subject: [PATCH 1/3] Create test files for 010_tab_completion.pl in dedicated
directory
This is mainly motivated by the meson patchset, which wants to put the log /
data for tests in a different place than the autoconf
build. 010_tab_completion.pl is the only test hardcoding the tmp_check/
directory, thereby mandating that directory name.
It's also a bit cleaner independently, because it doesn't intermingle the
files with more important things like the log/ directory.
---
src/bin/psql/t/010_tab_completion.pl | 33 ++++++++++++++--------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index 2eea515e871..cb36e8e4811 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -68,7 +68,7 @@ delete $ENV{LS_COLORS};
# In a VPATH build, we'll be started in the source directory, but we want
# to run in the build directory so that we can use relative paths to
-# access the tmp_check subdirectory; otherwise the output from filename
+# access the tab_comp_dir subdirectory; otherwise the output from filename
# completion tests is too variable.
if ($ENV{TESTDIR})
{
@@ -76,17 +76,18 @@ if ($ENV{TESTDIR})
}
# Create some junk files for filename completion testing.
+mkdir "tab_comp_dir";
my $FH;
-open $FH, ">", "tmp_check/somefile"
- or die("could not create file \"tmp_check/somefile\": $!");
+open $FH, ">", "tab_comp_dir/somefile"
+ or die("could not create file \"tab_comp_dir/somefile\": $!");
print $FH "some stuff\n";
close $FH;
-open $FH, ">", "tmp_check/afile123"
- or die("could not create file \"tmp_check/afile123\": $!");
+open $FH, ">", "tab_comp_dir/afile123"
+ or die("could not create file \"tab_comp_dir/afile123\": $!");
print $FH "more stuff\n";
close $FH;
-open $FH, ">", "tmp_check/afile456"
- or die("could not create file \"tmp_check/afile456\": $!");
+open $FH, ">", "tab_comp_dir/afile456"
+ or die("could not create file \"tab_comp_dir/afile456\": $!");
print $FH "other stuff\n";
close $FH;
@@ -272,16 +273,16 @@ clear_query();
# check filename completion
check_completion(
- "\\lo_import tmp_check/some\t",
- qr|tmp_check/somefile |,
+ "\\lo_import tab_comp_dir/some\t",
+ qr|tab_comp_dir/somefile |,
"filename completion with one possibility");
clear_query();
# note: readline might print a bell before the completion
check_completion(
- "\\lo_import tmp_check/af\t",
- qr|tmp_check/af\a?ile|,
+ "\\lo_import tab_comp_dir/af\t",
+ qr|tab_comp_dir/af\a?ile|,
"filename completion with multiple possibilities");
# broken versions of libedit require clear_line not clear_query here
@@ -291,15 +292,15 @@ clear_line();
# note: broken versions of libedit want to backslash the closing quote;
# not much we can do about that
check_completion(
- "COPY foo FROM tmp_check/some\t",
- qr|'tmp_check/somefile\\?' |,
+ "COPY foo FROM tab_comp_dir/some\t",
+ qr|'tab_comp_dir/somefile\\?' |,
"quoted filename completion with one possibility");
clear_line();
check_completion(
- "COPY foo FROM tmp_check/af\t",
- qr|'tmp_check/afile|,
+ "COPY foo FROM tab_comp_dir/af\t",
+ qr|'tab_comp_dir/afile|,
"quoted filename completion with multiple possibilities");
# some versions of readline/libedit require two tabs here, some only need one
@@ -307,7 +308,7 @@ check_completion(
# the quotes might appear, too
check_completion(
"\t\t",
- qr|afile123'? +'?(tmp_check/)?afile456|,
+ qr|afile123'? +'?(tab_comp_dir/)?afile456|,
"offer multiple file choices");
clear_line();
--
2.37.0.3.g30cc8d0f14
0002-Don-t-hardcode-tmp_check-as-test-directory-for-tap-t.patchtext/x-diff; charset=us-asciiDownload
From 675b62575456e1696913545695e99770835fc224 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 11 Aug 2022 09:41:54 -0700
Subject: [PATCH 2/3] Don't hardcode tmp_check/ as test directory for tap tests
This is motivated by the meson patchset, which wants to put the log / data for
tests in a different place than the autoconf build. Right now log files for
tap tests have to be inside $TESTDIR/tmp_check, whereas log files for
pg_regress/isolationtester are outside of tmp_check. This change doesn't fix
the latter, but is a prerequisite.
---
src/test/perl/PostgreSQL/Test/Utils.pm | 2 +-
src/Makefile.global.in | 6 +++---
src/tools/msvc/vcregress.pl | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 1ca2cc59170..88a472f2442 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -192,7 +192,7 @@ INIT
# Determine output directories, and create them. The base path is the
# TESTDIR environment variable, which is normally set by the invoking
# Makefile.
- $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}/tmp_check" : "tmp_check";
+ $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}" : "tmp_check";
$log_path = "$tmp_check/log";
mkdir $tmp_check;
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 0625b60c434..3cc40d117d0 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -454,7 +454,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTDIR='$(CURDIR)/tmp_check' PATH="$(bindir):$(CURDIR):$$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) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -465,7 +465,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTDIR='$(CURDIR)/tmp_check' PATH="$(bindir):$(CURDIR):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -477,7 +477,7 @@ echo "+++ tap check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
+ TESTDIR='$(CURDIR)/tmp_check' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index c3729f6be5e..da152da8e5f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -291,7 +291,7 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- $ENV{TESTDIR} = "$dir";
+ $ENV{TESTDIR} = "$dir/tmp_check";
my $module = basename $dir;
# add the module build dir as the second element in the PATH
$ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
--
2.37.0.3.g30cc8d0f14
0003-Split-TESTDIR-into-TESTLOGDIR-and-TESTDATADIR.patchtext/x-diff; charset=us-asciiDownload
From 43f87a08ccce69386444e43e202c40c0ba905a9a Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 15 Aug 2022 19:49:44 -0700
Subject: [PATCH 3/3] Split TESTDIR into TESTLOGDIR and TESTDATADIR
The motivation for this is twofold. For one the meson patchset would like to
have more control over the logfiles. For another, the log file location for
tap tests (tmp_check/log) is not symmetric to the log location for
pg_regress/isolation tests (log/).
This commit does not change the default location for log files for tap tests,
as that'd break the buildfarm log collection, it just provides the
infrastructure for doing so.
---
src/bin/psql/t/010_tab_completion.pl | 4 ++--
src/test/perl/PostgreSQL/Test/Utils.pm | 4 ++--
src/Makefile.global.in | 12 +++++++++---
src/tools/msvc/vcregress.pl | 4 +++-
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index cb36e8e4811..35f0aea3391 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -70,9 +70,9 @@ delete $ENV{LS_COLORS};
# to run in the build directory so that we can use relative paths to
# access the tab_comp_dir subdirectory; otherwise the output from filename
# completion tests is too variable.
-if ($ENV{TESTDIR})
+if ($ENV{TESTDATADIR})
{
- chdir $ENV{TESTDIR} or die "could not chdir to \"$ENV{TESTDIR}\": $!";
+ chdir $ENV{TESTDATADIR} or die "could not chdir to \"$ENV{TESDATATDIR}\": $!";
}
# Create some junk files for filename completion testing.
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 88a472f2442..0e3b75dfb51 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -192,8 +192,8 @@ INIT
# Determine output directories, and create them. The base path is the
# TESTDIR environment variable, which is normally set by the invoking
# Makefile.
- $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}" : "tmp_check";
- $log_path = "$tmp_check/log";
+ $tmp_check = $ENV{TESTDATADIR} ? "$ENV{TESTDATADIR}" : "tmp_check";
+ $log_path = $ENV{TESTLOGDIR} ? "$ENV{TESTLOGDIR}" : "log";
mkdir $tmp_check;
mkdir $log_path;
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 3cc40d117d0..2bad609d701 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -454,7 +454,9 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)/tmp_check' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTLOGDIR='$(CURDIR)/tmp_check/log' \
+ TESTDATADIR='$(CURDIR)/tmp_check' \
+ PATH="$(bindir):$(CURDIR):$$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) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -465,7 +467,9 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)/tmp_check' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTLOGDIR='$(CURDIR)/tmp_check/log' \
+ TESTDATADIR='$(CURDIR)/tmp_check' \
+ PATH="$(bindir):$(CURDIR):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -477,7 +481,9 @@ echo "+++ tap check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)/tmp_check' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
+ TESTLOGDIR='$(CURDIR)/tmp_check/log' $(with_temp_install) \
+ TESDATATDIR='$(CURDIR)/tmp_check' $(with_temp_install) \
+ PGPORT='6$(DEF_PGPORT)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index da152da8e5f..5182721eb79 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -291,7 +291,9 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- $ENV{TESTDIR} = "$dir/tmp_check";
+ $ENV{TESTDATADIR} = "$dir/tmp_check";
+ $ENV{TESTLOGDIR} = "$dir/tmp_check/log";
+
my $module = basename $dir;
# add the module build dir as the second element in the PATH
$ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
--
2.37.0.3.g30cc8d0f14
Hi,
On 2022-08-15 20:20:51 -0700, Andres Freund wrote:
On 2022-08-11 11:26:39 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2022-06-01 10:55:28 -0400, Tom Lane wrote:
[...] I'm definitely not happy with the proposed changes to
010_tab_completion.pl. My recollection is that those tests
were intentionally written to test tab completion involving a
directory name, but this change just loses that aspect entirely.How about creating a dedicated directory for the created files, to maintain
that? My goal of being able to redirect the test output elsewhere can be
achieved with just a hunk like this:Sure, there's no need for these files to be in the exact same place that
the output is collected. I just want to keep their same relationship
to the test's CWD.Of course it'd need a comment adjustment etc. It's a bit ugly to use a
otherwise empty tmp_check/ directory just to reduce the diff size, but it's
also not too bad.Given that it's no longer going to be the same tmp_check dir used
elsewhere, maybe we could s/tmp_check/tab_comp_dir/g or something
like that? That'd add some clarity I think.Done in the attached patch (0001).
A bunch of changes (e.g. f4ce6c4d3a3) made since I'd first written that
TESTOUTDIR patch means that we don't need two different variables anymore. So
patch 0002 just moves the addition of /tmp_check from Utils.pm to the places
in which TESTDIR is defined.That still "forces" tmp_check/ to exist when going through pg_regress, but
that's less annoying because pg_regress at least keeps
regression.{diffs,out}/log files/directory outside of tmp_check/.I've also attached a 0003 that splits the log location from the data
location. That could be used to make the log file location symmetrical between
pg_regress (log/) and tap tests (tmp_check/log). But it'd break the
buildfarm's tap test log file collection, so I don't think that's something we
really can do soon-ish?
Oops, 0003 had some typos in it that I added last minute... Corrected patches
attached.
Greetings,
Andres Freund
Attachments:
v2-0001-Create-test-files-for-010_tab_completion.pl-in-de.patchtext/x-diff; charset=us-asciiDownload
From eebc08f6628b15e0e8cc1a84dec9bbbd9931696b Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 11 Aug 2022 08:57:58 -0700
Subject: [PATCH v2 1/3] Create test files for 010_tab_completion.pl in
dedicated directory
This is mainly motivated by the meson patchset, which wants to put the log /
data for tests in a different place than the autoconf
build. 010_tab_completion.pl is the only test hardcoding the tmp_check/
directory, thereby mandating that directory name.
It's also a bit cleaner independently, because it doesn't intermingle the
files with more important things like the log/ directory.
---
src/bin/psql/t/010_tab_completion.pl | 33 ++++++++++++++--------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index 2eea515e871..cb36e8e4811 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -68,7 +68,7 @@ delete $ENV{LS_COLORS};
# In a VPATH build, we'll be started in the source directory, but we want
# to run in the build directory so that we can use relative paths to
-# access the tmp_check subdirectory; otherwise the output from filename
+# access the tab_comp_dir subdirectory; otherwise the output from filename
# completion tests is too variable.
if ($ENV{TESTDIR})
{
@@ -76,17 +76,18 @@ if ($ENV{TESTDIR})
}
# Create some junk files for filename completion testing.
+mkdir "tab_comp_dir";
my $FH;
-open $FH, ">", "tmp_check/somefile"
- or die("could not create file \"tmp_check/somefile\": $!");
+open $FH, ">", "tab_comp_dir/somefile"
+ or die("could not create file \"tab_comp_dir/somefile\": $!");
print $FH "some stuff\n";
close $FH;
-open $FH, ">", "tmp_check/afile123"
- or die("could not create file \"tmp_check/afile123\": $!");
+open $FH, ">", "tab_comp_dir/afile123"
+ or die("could not create file \"tab_comp_dir/afile123\": $!");
print $FH "more stuff\n";
close $FH;
-open $FH, ">", "tmp_check/afile456"
- or die("could not create file \"tmp_check/afile456\": $!");
+open $FH, ">", "tab_comp_dir/afile456"
+ or die("could not create file \"tab_comp_dir/afile456\": $!");
print $FH "other stuff\n";
close $FH;
@@ -272,16 +273,16 @@ clear_query();
# check filename completion
check_completion(
- "\\lo_import tmp_check/some\t",
- qr|tmp_check/somefile |,
+ "\\lo_import tab_comp_dir/some\t",
+ qr|tab_comp_dir/somefile |,
"filename completion with one possibility");
clear_query();
# note: readline might print a bell before the completion
check_completion(
- "\\lo_import tmp_check/af\t",
- qr|tmp_check/af\a?ile|,
+ "\\lo_import tab_comp_dir/af\t",
+ qr|tab_comp_dir/af\a?ile|,
"filename completion with multiple possibilities");
# broken versions of libedit require clear_line not clear_query here
@@ -291,15 +292,15 @@ clear_line();
# note: broken versions of libedit want to backslash the closing quote;
# not much we can do about that
check_completion(
- "COPY foo FROM tmp_check/some\t",
- qr|'tmp_check/somefile\\?' |,
+ "COPY foo FROM tab_comp_dir/some\t",
+ qr|'tab_comp_dir/somefile\\?' |,
"quoted filename completion with one possibility");
clear_line();
check_completion(
- "COPY foo FROM tmp_check/af\t",
- qr|'tmp_check/afile|,
+ "COPY foo FROM tab_comp_dir/af\t",
+ qr|'tab_comp_dir/afile|,
"quoted filename completion with multiple possibilities");
# some versions of readline/libedit require two tabs here, some only need one
@@ -307,7 +308,7 @@ check_completion(
# the quotes might appear, too
check_completion(
"\t\t",
- qr|afile123'? +'?(tmp_check/)?afile456|,
+ qr|afile123'? +'?(tab_comp_dir/)?afile456|,
"offer multiple file choices");
clear_line();
--
2.37.0.3.g30cc8d0f14
v2-0002-Don-t-hardcode-tmp_check-as-test-directory-for-ta.patchtext/x-diff; charset=us-asciiDownload
From 675b62575456e1696913545695e99770835fc224 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 11 Aug 2022 09:41:54 -0700
Subject: [PATCH v2 2/3] Don't hardcode tmp_check/ as test directory for tap
tests
This is motivated by the meson patchset, which wants to put the log / data for
tests in a different place than the autoconf build. Right now log files for
tap tests have to be inside $TESTDIR/tmp_check, whereas log files for
pg_regress/isolationtester are outside of tmp_check. This change doesn't fix
the latter, but is a prerequisite.
---
src/test/perl/PostgreSQL/Test/Utils.pm | 2 +-
src/Makefile.global.in | 6 +++---
src/tools/msvc/vcregress.pl | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 1ca2cc59170..88a472f2442 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -192,7 +192,7 @@ INIT
# Determine output directories, and create them. The base path is the
# TESTDIR environment variable, which is normally set by the invoking
# Makefile.
- $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}/tmp_check" : "tmp_check";
+ $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}" : "tmp_check";
$log_path = "$tmp_check/log";
mkdir $tmp_check;
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 0625b60c434..3cc40d117d0 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -454,7 +454,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTDIR='$(CURDIR)/tmp_check' PATH="$(bindir):$(CURDIR):$$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) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -465,7 +465,7 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTDIR='$(CURDIR)/tmp_check' PATH="$(bindir):$(CURDIR):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -477,7 +477,7 @@ echo "+++ tap check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
+ TESTDIR='$(CURDIR)/tmp_check' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index c3729f6be5e..da152da8e5f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -291,7 +291,7 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- $ENV{TESTDIR} = "$dir";
+ $ENV{TESTDIR} = "$dir/tmp_check";
my $module = basename $dir;
# add the module build dir as the second element in the PATH
$ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
--
2.37.0.3.g30cc8d0f14
v2-0003-Split-TESTDIR-into-TESTLOGDIR-and-TESTDATADIR.patchtext/x-diff; charset=us-asciiDownload
From 208cc45e1a078351a9e84dc3d6c62ff6f496fd38 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 15 Aug 2022 19:49:44 -0700
Subject: [PATCH v2 3/3] Split TESTDIR into TESTLOGDIR and TESTDATADIR
The motivation for this is twofold. For one the meson patchset would like to
have more control over the logfiles. For another, the log file location for
tap tests (tmp_check/log) is not symmetric to the log location for
pg_regress/isolation tests (log/).
This commit does not change the default location for log files for tap tests,
as that'd break the buildfarm log collection, it just provides the
infrastructure for doing so.
---
src/bin/psql/t/010_tab_completion.pl | 4 ++--
src/test/perl/PostgreSQL/Test/Utils.pm | 10 +++++-----
src/Makefile.global.in | 12 +++++++++---
src/tools/msvc/vcregress.pl | 4 +++-
4 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index cb36e8e4811..4aa6dd5fe13 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -70,9 +70,9 @@ delete $ENV{LS_COLORS};
# to run in the build directory so that we can use relative paths to
# access the tab_comp_dir subdirectory; otherwise the output from filename
# completion tests is too variable.
-if ($ENV{TESTDIR})
+if ($ENV{TESTDATADIR})
{
- chdir $ENV{TESTDIR} or die "could not chdir to \"$ENV{TESTDIR}\": $!";
+ chdir $ENV{TESTDATADIR} or die "could not chdir to \"$ENV{TESTDATADIR}\": $!";
}
# Create some junk files for filename completion testing.
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 88a472f2442..99d33451064 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -189,11 +189,11 @@ INIT
# test may still fail, but it's more likely to report useful facts.
$SIG{PIPE} = 'IGNORE';
- # Determine output directories, and create them. The base path is the
- # TESTDIR environment variable, which is normally set by the invoking
- # Makefile.
- $tmp_check = $ENV{TESTDIR} ? "$ENV{TESTDIR}" : "tmp_check";
- $log_path = "$tmp_check/log";
+ # Determine output directories, and create them. The base paths are the
+ # TESTDATADIR / TESTLOGDIR environment variables, which are normally set
+ # by the invoking Makefile.
+ $tmp_check = $ENV{TESTDATADIR} ? "$ENV{TESTDATADIR}" : "tmp_check";
+ $log_path = $ENV{TESTLOGDIR} ? "$ENV{TESTLOGDIR}" : "log";
mkdir $tmp_check;
mkdir $log_path;
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 3cc40d117d0..c765ab262f1 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -454,7 +454,9 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)/tmp_check' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTLOGDIR='$(CURDIR)/tmp_check/log' \
+ TESTDATADIR='$(CURDIR)/tmp_check' \
+ PATH="$(bindir):$(CURDIR):$$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) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -465,7 +467,9 @@ echo "+++ tap install-check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)/tmp_check' PATH="$(bindir):$(CURDIR):$$PATH" \
+ TESTLOGDIR='$(CURDIR)/tmp_check/log' \
+ TESTDATADIR='$(CURDIR)/tmp_check' \
+ PATH="$(bindir):$(CURDIR):$$PATH" \
PGPORT='6$(DEF_PGPORT)' top_builddir='$(top_builddir)' \
PG_REGRESS='$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
@@ -477,7 +481,9 @@ echo "+++ tap check in $(subdir) +++" && \
rm -rf '$(CURDIR)'/tmp_check && \
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
cd $(srcdir) && \
- TESTDIR='$(CURDIR)/tmp_check' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
+ TESTLOGDIR='$(CURDIR)/tmp_check/log' $(with_temp_install) \
+ TESTDATADIR='$(CURDIR)/tmp_check' $(with_temp_install) \
+ PGPORT='6$(DEF_PGPORT)' \
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
endef
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index da152da8e5f..5182721eb79 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -291,7 +291,9 @@ sub tap_check
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
$ENV{REGRESS_SHLIB} = "$topdir/src/test/regress/regress.dll";
- $ENV{TESTDIR} = "$dir/tmp_check";
+ $ENV{TESTDATADIR} = "$dir/tmp_check";
+ $ENV{TESTLOGDIR} = "$dir/tmp_check/log";
+
my $module = basename $dir;
# add the module build dir as the second element in the PATH
$ENV{PATH} =~ s!;!;$topdir/$Config/$module;!;
--
2.37.0.3.g30cc8d0f14
Andres Freund <andres@anarazel.de> writes:
On 2022-08-11 11:26:39 -0400, Tom Lane wrote:
Given that it's no longer going to be the same tmp_check dir used
elsewhere, maybe we could s/tmp_check/tab_comp_dir/g or something
like that? That'd add some clarity I think.
Done in the attached patch (0001).
I was confused by 0001, because with the present test setup that will
result in creating an extra tab_comp_dir that isn't inside tmp_check,
leading to needing cleanup infrastructure that isn't there. However,
0002 clarifies that: you're redefining TESTDIR. I think 0001 is OK
as long as you apply it after, or integrate it into, 0002.
patch 0002 just moves the addition of /tmp_check from Utils.pm to the places
in which TESTDIR is defined.
I see some references to TESTDIR in src/tools/msvc/ecpg_regression.proj.
It looks like those are not references to this variable but uses of the
<PropertyGroup>
<TESTDIR>..\..\interfaces\ecpg\test</TESTDIR>
thingy at the top of the file. Still, it's a bit confusing --- should
we rename that? Maybe not worth the trouble given the short expected
lifespan of the MSVC test scripts. 0002 seems fine otherwise.
I've also attached a 0003 that splits the log location from the data
location. That could be used to make the log file location symmetrical between
pg_regress (log/) and tap tests (tmp_check/log). But it'd break the
buildfarm's tap test log file collection, so I don't think that's something we
really can do soon-ish?
No particular opinion about 0003 -- as you say, that's going to be
gated by the buildfarm.
regards, tom lane
On 2022-08-15 Mo 23:20, Andres Freund wrote:
I've also attached a 0003 that splits the log location from the data
location. That could be used to make the log file location symmetrical between
pg_regress (log/) and tap tests (tmp_check/log). But it'd break the
buildfarm's tap test log file collection, so I don't think that's something we
really can do soon-ish?
Where would you like to have the buildfarm client search? Currently it
does this:
my @logs = glob("$dir/tmp_check/log/*");
$log->add_log($_) foreach (@logs);
I can add another pattern in that glob expression. I'm intending to put
out a new release pretty soon (before US Labor Day).
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Hi,
On 2022-08-16 11:33:12 -0400, Andrew Dunstan wrote:
On 2022-08-15 Mo 23:20, Andres Freund wrote:
I've also attached a 0003 that splits the log location from the data
location. That could be used to make the log file location symmetrical between
pg_regress (log/) and tap tests (tmp_check/log). But it'd break the
buildfarm's tap test log file collection, so I don't think that's something we
really can do soon-ish?Where would you like to have the buildfarm client search? Currently it
does this:��� my @logs = glob("$dir/tmp_check/log/*");
��� $log->add_log($_) foreach (@logs);
I can add another pattern in that glob expression. I'm intending to put
out a new release pretty soon (before US Labor Day).
$dir/log, so it's symmetric to the location of log files of regress/isolation
tests.
Thanks!
Andres