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 <and...@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
>From eebc08f6628b15e0e8cc1a84dec9bbbd9931696b Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@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
>From 675b62575456e1696913545695e99770835fc224 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@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
>From 208cc45e1a078351a9e84dc3d6c62ff6f496fd38 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@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
