MSVC buildfarm critters are not running modules' TAP tests

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

MSVC buildfarm critters are not running modules' TAP tests

Tom Lane-2
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;
Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Christoph Moench-Tegeder
## Tom Lane ([hidden email]):

> 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


Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Andrew Dunstan-8
In reply to this post by Tom Lane-2

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



Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Tom Lane-2
Andrew Dunstan <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Andrew Dunstan-8

On 9/8/19 5:59 PM, Tom Lane wrote:

> Andrew Dunstan <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Tom Lane-2
Andrew Dunstan <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Andrew Dunstan-8
In reply to this post by Tom Lane-2

On 9/8/19 6:18 PM, Tom Lane wrote:

> Andrew Dunstan <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Tom Lane-2
In reply to this post by Michael Paquier-2
Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MSVC buildfarm critters are not running modules' TAP tests

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment