Thread: MSVC buildfarm critters are not running modules' TAP tests
I noticed $subject while checking to see if commit db4383189's
new test script was behaving properly in the buildfarm. dory,
for one, should be running it but it just isn't.
It looks to me like the reason is that src/tools/msvc/vcregress.pl's
subroutine subdircheck isn't considering the possibility that
subdirectories of src/test/modules contain TAP tests. The
same code is used for contrib, so several existing TAP tests
are being missed there too.
I took a stab at fixing this, but lacking a Windows environment
to test in, I can't be sure if it works. The attached does kinda
sorta work if I run it in a Linux environment --- but I found that
system() doesn't automatically expand "t/*.pl" on Linux. Is that
an expected difference between Linux and Windows perl? I hacked
around that by adding a glob() call in sub tap_check, as seen in
the first hunk below, but I'm not very sure if that hunk should
get committed or not.
For ease of review, I did not re-indent the main part of sub
subdircheck, though that needs to be done before committing.
Anybody with suitable tools care to test/commit this?
regards, tom lane
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 33d8fb5..c2f2695 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -207,7 +207,7 @@ sub tap_check
my $dir = shift;
chdir $dir;
- my @args = ("prove", @flags, "t/*.pl");
+ my @args = ("prove", @flags, glob("t/*.pl"));
# adjust the environment for just this test
local %ENV = %ENV;
@@ -391,27 +391,24 @@ sub plcheck
return;
}
+# Run tests in a specified subdirectory of current directory.
+# Returns 0 if OK, else exit code
sub subdircheck
{
my $module = shift;
- if ( !-d "$module/sql"
- || !-d "$module/expected"
- || (!-f "$module/GNUmakefile" && !-f "$module/Makefile"))
- {
- return;
- }
+ chdir($module) || return 0;
- chdir $module;
- my @tests = fetchTests();
+ my $mstat = 0;
- # Leave if no tests are listed in the module.
- if (scalar @tests == 0)
+ # Look for traditional-style regression tests.
+ if (-d "sql" && -d "expected"
+ && (-f "GNUmakefile" || -f "Makefile"))
{
- chdir "..";
- return;
- }
+ my @tests = fetchTests();
+ if (scalar @tests > 0)
+ {
my @opts = fetchRegressOpts();
# Special processing for python transform modules, see their respective
@@ -437,15 +434,29 @@ sub subdircheck
}
print "============================================================\n";
- print "Checking $module\n";
+ print "Running $module regression tests\n";
my @args = (
"$topdir/$Config/pg_regress/pg_regress",
"--bindir=${topdir}/${Config}/psql",
"--dbname=contrib_regression", @opts, @tests);
print join(' ', @args), "\n";
system(@args);
+ my $status = $? >> 8;
+ $mstat ||= $status;
+ }
+ }
+
+ # Look for TAP tests.
+ if ($config->{tap_tests} && -d "t")
+ {
+ print "============================================================\n";
+ print "Running $module TAP tests\n";
+ my $status = tap_check(getcwd());
+ $mstat ||= $status;
+ }
+
chdir "..";
- return;
+ return $mstat;
}
sub contribcheck
@@ -462,8 +473,7 @@ sub contribcheck
next if ($module =~ /_plpython$/ && !defined($config->{python}));
next if ($module eq "sepgsql");
- subdircheck($module);
- my $status = $? >> 8;
+ my $status = subdircheck($module);
$mstat ||= $status;
}
exit $mstat if $mstat;
@@ -476,8 +486,7 @@ sub modulescheck
my $mstat = 0;
foreach my $module (glob("*"))
{
- subdircheck($module);
- my $status = $? >> 8;
+ my $status = subdircheck($module);
$mstat ||= $status;
}
exit $mstat if $mstat;
## Tom Lane (tgl@sss.pgh.pa.us): > I took a stab at fixing this, but lacking a Windows environment > to test in, I can't be sure if it works. The attached does kinda > sorta work if I run it in a Linux environment --- but I found that > system() doesn't automatically expand "t/*.pl" on Linux. Is that > an expected difference between Linux and Windows perl? At least the behaviour on Linux (or any unix) is expected: if you pass a list to perl's system(), perl does not run the command under a shell (a shell is only invoked if there's only a scalar argument to system() (or if the list has only one element) and that argument contains shell metacharacters). That's a source of no small amount "fun" for perl programms "shelling out", because "sometimes" there is no shell. Perl's system hase some more caveats, "perldoc -f system" has a starter on that topic. Regards, Christoph -- Spare Space
On 9/8/19 12:07 PM, Tom Lane wrote: > I noticed $subject while checking to see if commit db4383189's > new test script was behaving properly in the buildfarm. dory, > for one, should be running it but it just isn't. > > It looks to me like the reason is that src/tools/msvc/vcregress.pl's > subroutine subdircheck isn't considering the possibility that > subdirectories of src/test/modules contain TAP tests. The > same code is used for contrib, so several existing TAP tests > are being missed there too. > > I took a stab at fixing this, but lacking a Windows environment > to test in, I can't be sure if it works. The attached does kinda > sorta work if I run it in a Linux environment --- but I found that > system() doesn't automatically expand "t/*.pl" on Linux. Is that > an expected difference between Linux and Windows perl? I hacked > around that by adding a glob() call in sub tap_check, as seen in > the first hunk below, but I'm not very sure if that hunk should > get committed or not. > > For ease of review, I did not re-indent the main part of sub > subdircheck, though that needs to be done before committing. > > Anybody with suitable tools care to test/commit this? > > Actually, I think vcregress.pl is OK, this is a gap in the buildfarm client's coverage that will be fixed when I make a new release. Any day now I hope. See <https://github.com/PGBuildFarm/client-code/commit/1fc4e81e831fda64d62937de242ecda0ba145901> bowerbird which is already running that code is running the test you refer to: <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=bowerbird&dt=2019-09-08%2017%3A51%3A19&stg=test_misc-check> cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 9/8/19 12:07 PM, Tom Lane wrote:
>> It looks to me like the reason is that src/tools/msvc/vcregress.pl's
>> subroutine subdircheck isn't considering the possibility that
>> subdirectories of src/test/modules contain TAP tests. The
>> same code is used for contrib, so several existing TAP tests
>> are being missed there too.
> Actually, I think vcregress.pl is OK, this is a gap in the buildfarm
> client's coverage that will be fixed when I make a new release.
Hm. Changing the buildfarm script would be an alternative way to
fix the issue so far as the buildfarm is concerned, but it doesn't
seem like it provides any useful way for one to manually invoke
the tests on Windows. Or am I missing something about how that's
usually done?
regards, tom lane
On 9/8/19 5:59 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> On 9/8/19 12:07 PM, Tom Lane wrote:
>>> It looks to me like the reason is that src/tools/msvc/vcregress.pl's
>>> subroutine subdircheck isn't considering the possibility that
>>> subdirectories of src/test/modules contain TAP tests. The
>>> same code is used for contrib, so several existing TAP tests
>>> are being missed there too.
>> Actually, I think vcregress.pl is OK, this is a gap in the buildfarm
>> client's coverage that will be fixed when I make a new release.
> Hm. Changing the buildfarm script would be an alternative way to
> fix the issue so far as the buildfarm is concerned, but it doesn't
> seem like it provides any useful way for one to manually invoke
> the tests on Windows. Or am I missing something about how that's
> usually done?
>
>
The invocation is:
vcregress.pl taptest [ PROVE_FLAGS=xxx ] directory
directory needs to be relative to $topdir, so something like:
vcregress.pl taptest src/test/modules/test_misc
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 9/8/19 5:59 PM, Tom Lane wrote:
>> Hm. Changing the buildfarm script would be an alternative way to
>> fix the issue so far as the buildfarm is concerned, but it doesn't
>> seem like it provides any useful way for one to manually invoke
>> the tests on Windows. Or am I missing something about how that's
>> usually done?
> The invocation is:
> vcregress.pl taptest [ PROVE_FLAGS=xxx ] directory
Sure, I saw that you can run one test that way ... but what do you
do when you want the equivalent of check-world?
(I'm surprised that vcregress hasn't already got a "world" option.
But at least there should be a determinate list of what you need
to run to get all the tests.)
regards, tom lane
On Sun, Sep 08, 2019 at 06:18:33PM -0400, Tom Lane wrote:
> Sure, I saw that you can run one test that way ... but what do you
> do when you want the equivalent of check-world?
I think that it is a good idea to add in subdircheck an extra path to
check after TAP tests and run optionally these on top of the normal
regression tests. I have a couple of comments.
+ # Look for TAP tests.
+ if ($config->{tap_tests} && -d "t")
+ {
+ print
"============================================================\n";
+ print "Running $module TAP tests\n";
+ my $status = tap_check(getcwd());
+ $mstat ||= $status;
+ }
Shouldn't we check after TAP_TESTS in the Makefile?
There is an argument to also check after isolation tests and run
them. It seems to me that we should check after ISOLATION, and run
optionally the tests if there is anything present. So we need
something like fetchTests() and fetchRegressOpts() but for isolation
tests.
The glob() part is a good idea in itself I think. Why not
back-patching it? I could double-check it as well.
--
Michael
Attachment
On 9/8/19 6:18 PM, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >> On 9/8/19 5:59 PM, Tom Lane wrote: >>> Hm. Changing the buildfarm script would be an alternative way to >>> fix the issue so far as the buildfarm is concerned, but it doesn't >>> seem like it provides any useful way for one to manually invoke >>> the tests on Windows. Or am I missing something about how that's >>> usually done? >> The invocation is: >> vcregress.pl taptest [ PROVE_FLAGS=xxx ] directory > Sure, I saw that you can run one test that way ... but what do you > do when you want the equivalent of check-world? > > (I'm surprised that vcregress hasn't already got a "world" option. > But at least there should be a determinate list of what you need > to run to get all the tests.) > > Possibly. The script has not had as much love as it needs. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Michael Paquier <michael@paquier.xyz> writes:
> I think that it is a good idea to add in subdircheck an extra path to
> check after TAP tests and run optionally these on top of the normal
> regression tests. I have a couple of comments.
> Shouldn't we check after TAP_TESTS in the Makefile?
Yeah, perhaps, but I wasn't sure about how to do that easily.
Feel free to add it ...
> There is an argument to also check after isolation tests and run
> them.
Hm, yeah, if there are any such tests in those directories.
> The glob() part is a good idea in itself I think. Why not
> back-patching it? I could double-check it as well.
The whole thing should be back-patched into branches that have
any affected tests. (But, please, not till after beta4 is
tagged.)
regards, tom lane
On Sun, Sep 08, 2019 at 07:44:16PM -0400, Tom Lane wrote: > Yeah, perhaps, but I wasn't sure about how to do that easily. > Feel free to add it ... Feeding the makefile contents and then doing a lookup using =~ should be enough. I think that we should just refactor set of routines for fetchTests() so as it uses the flag to look after as input. fetchRegressOpts() has tweaks for ENCODING and NO_LOCALE which don't apply to the isolation tests so perhaps a different routine would be better. > Hm, yeah, if there are any such tests in those directories. snapshot_too_old is a good example to work with. > The whole thing should be back-patched into branches that have > any affected tests. (But, please, not till after beta4 is > tagged.) Sure. Don't worry about that. I am focused on another thing lately and it does not touch back-branches. -- Michael
Attachment
On Mon, Sep 09, 2019 at 09:43:06AM +0900, Michael Paquier wrote: > On Sun, Sep 08, 2019 at 07:44:16PM -0400, Tom Lane wrote: >> The whole thing should be back-patched into branches that have >> any affected tests. (But, please, not till after beta4 is >> tagged.) > > Sure. Don't worry about that. I am focused on another thing lately > and it does not touch back-branches. As the cease-fire period is over, I have committed the part for glob() and backpatched down to 9.4. The other parts need a closer lookup, and I would not bother with anything older than v12 as TAP_TESTS has been added there in pgxs.mk. -- Michael