On Mon, Nov 26, 2018 at 12:41:21PM -0500, Andrew Dunstan wrote: > It's not the buildfarm that is broken. Both contribcheck() and > modulescheck() in vcregress.pl run in installcheck mode, i.e. with an > already running database. That makes the tests run faster, because setting > up and breaking down instances is expensive, especially on Windows. We've > got far too profligate with that in recent years, to the extent that some of > my Windows buildfarm animals take many hours to complete full runs these > days.
I have noticed the performance impact when switching to check for those targets. It was bad. > Note that TAP tests are not a problem here. It's up to the TAP scripts to > set up and break down postgres instances they need, with whatever config > options are required, such as shared preload libraries. It's only modules > using pg_regress that could be a problem. Indeed. > A simple way to proceed might be to have vcregress.pl honor the > NO_INSTALLCHECK flag. Currently this is only used by pg_stat_statements, but > we could add it anywhere required. In vcregress.pl, I would probably do this > by having fetchTests() check for the flag and return an empty set of tests > if it's found, and in turn have subdircheck() do nothing if it has an empty > set of tests. Okay, let's do so by supporting correctly NO_INSTALLCHECK. My other refactoring work can also live with that. Returning an empty list via fetchTests() and bypass a run if nothing is present looks fine to me. One extra thing is that modules/commit_ts and modules/test_rls_hooks are missing NO_INSTALLCHECK, so we would need to add that part to be completely correct. I would also be inclined to back-patch both parts, however for my stuff I could live with this patch only on HEAD, and anybody willing to use installcheck on commit_ts and test_rls_hooks may be surprised to not be able to do that anymore after the minor release. It still looks incorrect to me though to be able to run installcheck on those modules though... Attached is a proposal of patch, which works as I would expect with modulescheck and contribcheck. How does that look? -- Michael
diff --git a/src/test/modules/commit_ts/Makefile b/src/test/modules/commit_ts/Makefile
index 6d4f3be358..6a9c3971fb 100644
--- a/src/test/modules/commit_ts/Makefile
+++ b/src/test/modules/commit_ts/Makefile
@@ -2,6 +2,9 @@
REGRESS = commit_timestamp
REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/commit_ts/commit_ts.conf
+# Disabled because these tests require "track_commit_timestamp = on",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/src/test/modules/test_rls_hooks/Makefile b/src/test/modules/test_rls_hooks/Makefile
index 6b772c4db1..284fdaf095 100644
--- a/src/test/modules/test_rls_hooks/Makefile
+++ b/src/test/modules/test_rls_hooks/Makefile
@@ -9,6 +9,9 @@ EXTENSION = test_rls_hooks
REGRESS = test_rls_hooks
REGRESS_OPTS = --temp-config=$(top_srcdir)/src/test/modules/test_rls_hooks/rls_hooks.conf
+# Disabled because these tests require "shared_preload_libraries=test_rls_hooks",
+# which typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 599b521014..4018313bf2 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -361,6 +361,10 @@ sub plcheck
{
@lang_args = ();
}
+
+ # Move on if no tests are listed.
+ next if (scalar @tests == 0);
+
print
"============================================================\n";
print "Checking $lang\n";
@@ -391,6 +395,14 @@ sub subdircheck
chdir $module;
my @tests = fetchTests();
+
+ # Leave if no tests are listed in the module.
+ if (scalar @tests == 0)
+ {
+ chdir "..";
+ return;
+ }
+
my @opts = fetchRegressOpts();
# Special processing for python transform modules, see their respective
@@ -638,6 +650,8 @@ sub fetchRegressOpts
return @opts;
}
+# Fetch the list of tests by parsing a module's Makefile. An empty
+# list is returned if the module does not need to run anything.
sub fetchTests
{
@@ -651,6 +665,14 @@ sub fetchTests
my $t = "";
$m =~ s{\\\r?\n}{}g;
+
+ # A module specifying NO_INSTALLCHECK does not support installcheck,
+ # so bypass its run by returning an empty set of tests.
+ if ($m =~ /^\s*NO_INSTALLCHECK\s*=\s*\S+/m)
+ {
+ return ();
+ }
+
if ($m =~ /^REGRESS\s*=\s*(.*)$/gm)
{
$t = $1;
signature.asc
Description: PGP signature
