Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 08/16/2015 08:30 PM, Michael Paquier wrote:

On Mon, Aug 17, 2015 at 7:15 AM, Noah Misch  wrote:

On Sun, Aug 16, 2015 at 05:08:56PM -0400, Andrew Dunstan wrote:

On 08/16/2015 02:23 PM, Noah Misch wrote:

-sub tapcheck
+sub tap_check
  {
-   InstallTemp();
+   die "Tap tests not enabled in configuration"
+ unless $config->{tap_tests};

I endorse Heikki's argument for having omitted the configuration flag:
http://www.postgresql.org/message-id/55b90161.5090...@iki.fi


That argument is not correct.  None of the tap tests can be run via make if
--enable-tap-tests is not set. This doesn't just apply to the check-world
target as Heikki asserted. Have a look at the definitions of prove_check and
prove_installcheck in src/Makefile.global.in if you don't believe me.

While that one piece of Heikki's argument was in error, I didn't feel that it
affected the conclusion.  Please reply to the aforementioned email to dispute
the decision; some involved parties may not be following this thread.

FWIW, I agree with Andrew's approach here. We are going to need this
parameter switch once TAP routines are used in another code path than
src/bin to control if a test can be run or not based on the presence
of t/ and the value of this parameter. See for example the patch aimed
at testing dumpable tables with pg_dump which should be part of the
target modulescheck as argued upthread.
My 2c.



I spoke to Heikki about this the other day, and he's fine with using the 
test if there's a need for it. In addition to Michael's point, the 
buildfarm has a need for it - if the flag isn't set it won't run the 
checks, so the flag should be supported.  I'm therefore going to stick 
with the code above.


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Michael Paquier]

On Mon, Aug 17, 2015 at 7:15 AM, Noah Misch  wrote:
> On Sun, Aug 16, 2015 at 05:08:56PM -0400, Andrew Dunstan wrote:
>> On 08/16/2015 02:23 PM, Noah Misch wrote:
>> >>-sub tapcheck
>> >>+sub tap_check
>> >>  {
>> >>-   InstallTemp();
>> >>+   die "Tap tests not enabled in configuration"
>> >>+ unless $config->{tap_tests};
>> >I endorse Heikki's argument for having omitted the configuration flag:
>> >http://www.postgresql.org/message-id/55b90161.5090...@iki.fi
>>
>>
>> That argument is not correct.  None of the tap tests can be run via make if
>> --enable-tap-tests is not set. This doesn't just apply to the check-world
>> target as Heikki asserted. Have a look at the definitions of prove_check and
>> prove_installcheck in src/Makefile.global.in if you don't believe me.
>
> While that one piece of Heikki's argument was in error, I didn't feel that it
> affected the conclusion.  Please reply to the aforementioned email to dispute
> the decision; some involved parties may not be following this thread.

FWIW, I agree with Andrew's approach here. We are going to need this
parameter switch once TAP routines are used in another code path than
src/bin to control if a test can be run or not based on the presence
of t/ and the value of this parameter. See for example the patch aimed
at testing dumpable tables with pg_dump which should be part of the
target modulescheck as argued upthread.
My 2c.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Noah Misch]

On Sun, Aug 16, 2015 at 05:08:56PM -0400, Andrew Dunstan wrote:
> On 08/16/2015 02:23 PM, Noah Misch wrote:
> >>-sub tapcheck
> >>+sub tap_check
> >>  {
> >>-   InstallTemp();
> >>+   die "Tap tests not enabled in configuration"
> >>+ unless $config->{tap_tests};
> >I endorse Heikki's argument for having omitted the configuration flag:
> >http://www.postgresql.org/message-id/55b90161.5090...@iki.fi
> 
> 
> That argument is not correct.  None of the tap tests can be run via make if
> --enable-tap-tests is not set. This doesn't just apply to the check-world
> target as Heikki asserted. Have a look at the definitions of prove_check and
> prove_installcheck in src/Makefile.global.in if you don't believe me.

While that one piece of Heikki's argument was in error, I didn't feel that it
affected the conclusion.  Please reply to the aforementioned email to dispute
the decision; some involved parties may not be following this thread.

> >>+   # Reset those values, they may have been changed by another test.
> >>+   # XXX is this true?
> >>+   local %ENV = %ENV;
> >>$ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
> >>$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
> >>+   $ENV{TESTDIR} = "$dir";
> >The comment pertained only to the TESTDIR environment variable.  Adding the
> >local %ENV is unnecessary.  I think you can just delete the comment; there's
> >nothing noteworthy about setting a different TESTDIR value for each suite.
> >The PERL5LIB and PG_REGRESS updates need happen just once per bincheck, 
> >though
> >keeping them here seems good for the benefit of future TAP targets.
> 
> The local decl is clearly needed. Otherwise repeated invocations of the
> function will result in repeated prepending of $topdir/src/test/perl to
> PERL5LIB. It's incredibly cheap anyway. And as you say, this is a much
> better place to do that than in bincheck. If you prefer, I could dispense
> with the local and instead only set to PERL5LIB conditionally. It's just a
> matter of style.

With the comment gone, the way you've done this section is fine.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 08/16/2015 02:23 PM, Noah Misch wrote:

-sub tapcheck
+sub tap_check
  {
-   InstallTemp();
+   die "Tap tests not enabled in configuration"
+ unless $config->{tap_tests};

I endorse Heikki's argument for having omitted the configuration flag:
http://www.postgresql.org/message-id/55b90161.5090...@iki.fi



That argument is not correct.  None of the tap tests can be run via make 
if --enable-tap-tests is not set. This doesn't just apply to the 
check-world target as Heikki asserted. Have a look at the definitions of 
prove_check and prove_installcheck in src/Makefile.global.in if you 
don't believe me.





+   # Reset those values, they may have been changed by another test.
+   # XXX is this true?
+   local %ENV = %ENV;
$ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
  
+	$ENV{TESTDIR} = "$dir";

The comment pertained only to the TESTDIR environment variable.  Adding the
local %ENV is unnecessary.  I think you can just delete the comment; there's
nothing noteworthy about setting a different TESTDIR value for each suite.
The PERL5LIB and PG_REGRESS updates need happen just once per bincheck, though
keeping them here seems good for the benefit of future TAP targets.




The local decl is clearly needed. Otherwise repeated invocations of the 
function will result in repeated prepending of $topdir/src/test/perl to 
PERL5LIB. It's incredibly cheap anyway. And as you say, this is a much 
better place to do that than in bincheck. If you prefer, I could 
dispense with the local and instead only set to PERL5LIB conditionally. 
It's just a matter of style.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Noah Misch]

On Fri, Aug 14, 2015 at 09:31:43AM -0400, Andrew Dunstan wrote:
> On 08/14/2015 09:27 AM, Andrew Dunstan wrote:
> >The code
> >is rearranged a little, and an incorrect piece of code setting
> >$ENV{PERL5LIB} is fixed (in the case where it was previously empty it
> >would have added a spurious ";" and possibly caused a warning as well).

The PERL5LIB bit is clearly good, but please commit it separately.

> >Instead of looking everywhere in the tree for /t directories, the new
> >bincheck function only looks for them in src/bin.

Check.

> >And the tests would die
> >on the first failure, as we would also expect the checks run under "make"
> >to do.

"make" offers a choice.  If I had to settle for just one of its behaviors, I
would choose -k.  One can emulate "make -S" by killing "make -k" after the
first error, but there's no straightforward way to emulate "make -k" in terms
of "make -S".  Thus, I prefer the ancient vcregress decision in this area.  In
any event, a proposal to change it would need its own thread and a patch that
changes all targets facing this decision.

> >The effect is to remove the target "tapcheck" for which there is no "make"
> >equivalent, and replace it with the target "bincheck", which is the
> >equivalent of "make -C src/bin installcheck", which happens to be what the
> >buildfarm runs.

This "vcregress bincheck" differs from "make -C src/bin installcheck" by using
"check" semantics, and it differs from "make -C src/bin check" by skipping the
pg_upgrade test suite.  (I don't have any point in saying that beyond
clarifying the record.)

> -sub tapcheck
> +sub tap_check
>  {
> - InstallTemp();
> + die "Tap tests not enabled in configuration"
> +   unless $config->{tap_tests};

I endorse Heikki's argument for having omitted the configuration flag:
http://www.postgresql.org/message-id/55b90161.5090...@iki.fi

> + # Reset those values, they may have been changed by another test.
> + # XXX is this true?
> + local %ENV = %ENV;
>   $ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
>   $ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
>  
> + $ENV{TESTDIR} = "$dir";

The comment pertained only to the TESTDIR environment variable.  Adding the
local %ENV is unnecessary.  I think you can just delete the comment; there's
nothing noteworthy about setting a different TESTDIR value for each suite.
The PERL5LIB and PG_REGRESS updates need happen just once per bincheck, though
keeping them here seems good for the benefit of future TAP targets.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 08/14/2015 09:27 AM, Andrew Dunstan wrote:



On 08/14/2015 03:32 AM, Noah Misch wrote:

On Thu, Aug 13, 2015 at 11:17:40AM -0400, Andrew Dunstan wrote:

here's what I propose.
This changes more than the tapcheck name and the suites it could 
run.  Would
you write up the changes you chose to include?  That will help guide 
review.




I don't think it changes anything other than what was discussed. The 
code is rearranged a little, and an incorrect piece of code setting 
$ENV{PERL5LIB} is fixed (in the case where it was previously empty it 
would have added a spurious ";" and possibly caused a warning as 
well). Instead of looking everywhere in the tree for /t directories, 
the new bincheck function only looks for them in src/bin. And the 
tests would die on the first failure, as we would also expect the 
checks run under "make" to do.


The effect is to remove the target "tapcheck" for which there is no 
"make" equivalent, and replace it with the target "bincheck", which is 
the equivalent of "make -C src/bin installcheck", which happens to be 
what the buildfarm runs.





I've just noticed a small error in the patch. Revised version attached.

cheers

andrew


diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index d3d736b..e4fa888 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -34,7 +34,7 @@ if (-e "src/tools/msvc/buildenv.pl")
 
 my $what = shift || "";
 if ($what =~
-/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|tapcheck)$/i
+/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|bincheck)$/i
   )
 {
 	$what = uc $what;
@@ -61,7 +61,14 @@ unless ($schedule)
 	$schedule = "parallel" if ($what eq 'CHECK' || $what =~ /PARALLEL/);
 }
 
-$ENV{PERL5LIB} = "$topdir/src/tools/msvc;$ENV{PERL5LIB}";
+if ($ENV{PERL5LIB})
+{
+	$ENV{PERL5LIB} = "$topdir/src/tools/msvc;$ENV{PERL5LIB}";
+}
+else
+{
+	$ENV{PERL5LIB} = "$topdir/src/tools/msvc";
+}
 
 my $maxconn = "";
 $maxconn = "--max_connections=$ENV{MAX_CONNECTIONS}"
@@ -81,7 +88,7 @@ my %command = (
 	CONTRIBCHECK   => \&contribcheck,
 	MODULESCHECK   => \&modulescheck,
 	ISOLATIONCHECK => \&isolationcheck,
-	TAPCHECK   => \&tapcheck,
+	BINCHECK   => \&bincheck,
 	UPGRADECHECK   => \&upgradecheck,);
 
 my $proc = $command{$what};
@@ -168,44 +175,46 @@ sub isolationcheck
 	exit $status if $status;
 }
 
-sub tapcheck
+sub tap_check
 {
-	InstallTemp();
+	die "Tap tests not enabled in configuration"
+	  unless $config->{tap_tests};
+
+	my $dir = shift;
+	chdir $dir;
 
 	my @args = ( "prove", "--verbose", "t/*.pl");
-	my $mstat = 0;
 
+	# Reset those values, they may have been changed by another test.
+	# XXX is this true?
+	local %ENV = %ENV;
 	$ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
 	$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
 
+	$ENV{TESTDIR} = "$dir";
+
+	system(@args);
+	my $status = $? >> 8;
+	return $status;
+}
+
+sub bincheck
+{
+	InstallTemp();
+
+	my $mstat = 0;
+
 	# Find out all the existing TAP tests by looking for t/ directories
 	# in the tree.
-	my $tap_dirs = [];
-	my @top_dir = ($topdir);
-	File::Find::find(
-		{   wanted => sub {
-/^t\z/s
-  && push(@$tap_dirs, $File::Find::name);
-			  }
-		},
-		@top_dir);
+	my @bin_dirs = glob("$topdir/src/bin/*");
 
 	# Process each test
-	foreach my $test_path (@$tap_dirs)
+	foreach my $dir (@$bin_dirs)
 	{
-		# Like on Unix "make check-world", don't run the SSL test suite
-		# automatically.
-		next if ($test_path =~ /\/src\/test\/ssl\//);
-
-		my $dir = dirname($test_path);
-		chdir $dir;
-		# Reset those values, they may have been changed by another test.
-		$ENV{TESTDIR} = "$dir";
-		system(@args);
-		my $status = $? >> 8;
-		$mstat ||= $status;
+		next unless -d "$dir/t";
+		my $status = tap_check($dir);
+		exit $status if $status;
 	}
-	exit $mstat if $mstat;
 }
 
 sub plcheck

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 08/14/2015 03:32 AM, Noah Misch wrote:

On Thu, Aug 13, 2015 at 11:17:40AM -0400, Andrew Dunstan wrote:

here's what I propose.

This changes more than the tapcheck name and the suites it could run.  Would
you write up the changes you chose to include?  That will help guide review.



I don't think it changes anything other than what was discussed. The 
code is rearranged a little, and an incorrect piece of code setting 
$ENV{PERL5LIB} is fixed (in the case where it was previously empty it 
would have added a spurious ";" and possibly caused a warning as well). 
Instead of looking everywhere in the tree for /t directories, the new 
bincheck function only looks for them in src/bin. And the tests would 
die on the first failure, as we would also expect the checks run under 
"make" to do.


The effect is to remove the target "tapcheck" for which there is no 
"make" equivalent, and replace it with the target "bincheck", which is 
the equivalent of "make -C src/bin installcheck", which happens to be 
what the buildfarm runs.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Noah Misch]

On Thu, Aug 13, 2015 at 11:17:40AM -0400, Andrew Dunstan wrote:
> here's what I propose.

This changes more than the tapcheck name and the suites it could run.  Would
you write up the changes you chose to include?  That will help guide review.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Michael Paquier]

On Fri, Aug 14, 2015 at 1:18 AM, Andrew Dunstan  wrote:
>
>
> On 08/13/2015 12:03 PM, Michael Paquier wrote:
>>
>> On Fri, Aug 14, 2015 at 12:17 AM, Andrew Dunstan wrote:
>>>
>>> here's what I propose.
>>
>> This patch does not take into account that there may be other code
>> paths than src/bin/ that may have TAP tests (see my pending patch to
>> test pg_dump with extensions including dumpable relations for
>> example). I guess that it is done on purpose, now what are we going to
>> do about the following things:
>> - for src/test/ssl, should we have a new target in vcregress? Like
>> ssltest?
>> - for the pending patch I just mentioned, what should we do then?
>> Should we expect it to work under modulescheck?
>
>
>
> Of course it takes it into account.

Yes, sorry. I misread your patch, reading code late in the evening is
not a good idea :)

> What it does is let you add extra checks
> easily. But I am not going to accept the divergence in vcregress.pl from the
> way we run tests using the standard tools.

OK, this sounds fair to keep up with. And sorry for actually breaking
vcregress.pl regarding this consistency. Shouldn't we remove
upgradecheck and move it under the banner bincheck then? Perhaps
testing the upgrade made sense before but now it is located under
src/bin so it sounds weird to do things separately.

> Yes, ssl tests should have a new
> target, as should any other new set of tests. We don't have a single make
> target for tap tests and neither should we in vcregress.pl.

Actually it is not only ssl, I would think that all those targets
should run under the same banner:
ALWAYS_SUBDIRS = examples locale thread ssl
But that's a separate discussion...

+die "Tap tests not enabled in configuration"
+  unless $config->{tap_tests};
I think that you are missing to update a couple of places with this
parameter, one being getfakeconfig...@solution.pm, a second being
config_default.pl. Also, I would think that it is saner to simply
return here and not die, then log a message to user to tell him that
the test has been skipped. This is thinking about the module having
TAP tests that should be located under src/test/modules
Regards,
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 08/13/2015 12:03 PM, Michael Paquier wrote:

On Fri, Aug 14, 2015 at 12:17 AM, Andrew Dunstan wrote:

here's what I propose.

This patch does not take into account that there may be other code
paths than src/bin/ that may have TAP tests (see my pending patch to
test pg_dump with extensions including dumpable relations for
example). I guess that it is done on purpose, now what are we going to
do about the following things:
- for src/test/ssl, should we have a new target in vcregress? Like ssltest?
- for the pending patch I just mentioned, what should we do then?
Should we expect it to work under modulescheck?



Of course it takes it into account. What it does is let you add extra 
checks easily. But I am not going to accept the divergence in 
vcregress.pl from the way we run tests using the standard tools. Yes, 
ssl tests should have a new target, as should any other new set of 
tests. We don't have a single make target for tap tests and neither 
should we in vcregress.pl.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Michael Paquier]

On Fri, Aug 14, 2015 at 12:17 AM, Andrew Dunstan wrote:
> here's what I propose.

This patch does not take into account that there may be other code
paths than src/bin/ that may have TAP tests (see my pending patch to
test pg_dump with extensions including dumpable relations for
example). I guess that it is done on purpose, now what are we going to
do about the following things:
- for src/test/ssl, should we have a new target in vcregress? Like ssltest?
- for the pending patch I just mentioned, what should we do then?
Should we expect it to work under modulescheck?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 08/01/2015 07:13 PM, Andrew Dunstan wrote:


On 08/01/2015 04:44 PM, Noah Misch wrote:



--enable-tap-tests is a reasonable configuration setting, because it's
about whether or not we have a TAP testing framework available, but I
think we should stop calling the bin tests "TAP tests" and we should
change the test name in vcregress.pl to a more appropriate name. In 
the
buildfarm I'm calling the step "bin-check": 



Thoughts?
While lack of granularity in vcregress.pl is hostile, with so much 
about MSVC

development being hostile, this one is below my noise floor.



Speaking with my buildfarm hat on, it's well above mine. And these 
changes make the situation worse quite gratuitously. Anyway, I'll fix it.







here's what I propose.

cheers

andrew


diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index d3d736b..b50a160 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -34,7 +34,7 @@ if (-e "src/tools/msvc/buildenv.pl")
 
 my $what = shift || "";
 if ($what =~
-/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|tapcheck)$/i
+/^(check|installcheck|plcheck|contribcheck|modulescheck|ecpgcheck|isolationcheck|upgradecheck|bincheck)$/i
   )
 {
 	$what = uc $what;
@@ -61,7 +61,14 @@ unless ($schedule)
 	$schedule = "parallel" if ($what eq 'CHECK' || $what =~ /PARALLEL/);
 }
 
-$ENV{PERL5LIB} = "$topdir/src/tools/msvc;$ENV{PERL5LIB}";
+if ($ENV{PERL5LIB})
+{
+	$ENV{PERL5LIB} = "$topdir/src/tools/msvc;$ENV{PERL5LIB}";
+}
+else
+{
+	$ENV{PERL5LIB} = "$topdir/src/tools/msvc";
+}
 
 my $maxconn = "";
 $maxconn = "--max_connections=$ENV{MAX_CONNECTIONS}"
@@ -81,7 +88,7 @@ my %command = (
 	CONTRIBCHECK   => \&contribcheck,
 	MODULESCHECK   => \&modulescheck,
 	ISOLATIONCHECK => \&isolationcheck,
-	TAPCHECK   => \&tapcheck,
+	BINCHECK   => \&bincheck,
 	UPGRADECHECK   => \&upgradecheck,);
 
 my $proc = $command{$what};
@@ -168,44 +175,46 @@ sub isolationcheck
 	exit $status if $status;
 }
 
-sub tapcheck
+sub tap_check
 {
-	InstallTemp();
+	die "Tap tests not enabled in configuration"
+	  unless $config->{tap_tests};
+
+	my $dir = shift;
+	chdir $dir;
 
 	my @args = ( "prove", "--verbose", "t/*.pl");
-	my $mstat = 0;
 
+	# Reset those values, they may have been changed by another test.
+	# XXX is this true?
+	local %ENV = %ENV;
 	$ENV{PERL5LIB} = "$topdir/src/test/perl;$ENV{PERL5LIB}";
 	$ENV{PG_REGRESS} = "$topdir/$Config/pg_regress/pg_regress";
 
+	$ENV{TESTDIR} = "$dir";
+
+	system(@args);
+	my $status = $? >> 8;
+	return $status;
+}
+
+sub bincheck
+{
+	InstallTemp();
+
+	my $mstat = 0;
+
 	# Find out all the existing TAP tests by looking for t/ directories
 	# in the tree.
-	my $tap_dirs = [];
-	my @top_dir = ($topdir);
-	File::Find::find(
-		{   wanted => sub {
-/^t\z/s
-  && push(@$tap_dirs, $File::Find::name);
-			  }
-		},
-		@top_dir);
+	my @bin_dirs = glob("$topdir/src/bin/*");
 
 	# Process each test
-	foreach my $test_path (@$tap_dirs)
+	foreach my $dir (@$tap_dirs)
 	{
-		# Like on Unix "make check-world", don't run the SSL test suite
-		# automatically.
-		next if ($test_path =~ /\/src\/test\/ssl\//);
-
-		my $dir = dirname($test_path);
-		chdir $dir;
-		# Reset those values, they may have been changed by another test.
-		$ENV{TESTDIR} = "$dir";
-		system(@args);
-		my $status = $? >> 8;
-		$mstat ||= $status;
+		next unless -d "$dir/t";
+		my $status = tap_check($dir);
+		exit $status if $status;
 	}
-	exit $mstat if $mstat;
 }
 
 sub plcheck

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Noah Misch]

On Sat, Aug 01, 2015 at 07:13:04PM -0400, Andrew Dunstan wrote:
> On 08/01/2015 04:44 PM, Noah Misch wrote:
> >>>--enable-tap-tests is a reasonable configuration setting, because it's
> >>>about whether or not we have a TAP testing framework available, but I
> >>>think we should stop calling the bin tests "TAP tests" and we should
> >>>change the test name in vcregress.pl to a more appropriate name. In the
> >>>buildfarm I'm calling the step "bin-check": 
> >>>
> >>>
> >>>Thoughts?
> >While lack of granularity in vcregress.pl is hostile, with so much about MSVC
> >development being hostile, this one is below my noise floor.
> 
> 
> Speaking with my buildfarm hat on, it's well above mine. And these changes
> make the situation worse quite gratuitously. Anyway, I'll fix it.

Sounds good.  I'll be interested to read your design.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 08/01/2015 04:44 PM, Noah Misch wrote:



--enable-tap-tests is a reasonable configuration setting, because it's
about whether or not we have a TAP testing framework available, but I
think we should stop calling the bin tests "TAP tests" and we should
change the test name in vcregress.pl to a more appropriate name. In the
buildfarm I'm calling the step "bin-check": 


Thoughts?

While lack of granularity in vcregress.pl is hostile, with so much about MSVC
development being hostile, this one is below my noise floor.



Speaking with my buildfarm hat on, it's well above mine. And these 
changes make the situation worse quite gratuitously. Anyway, I'll fix it.



cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Noah Misch]

On Thu, Jul 30, 2015 at 07:54:27PM -0400, Andrew Dunstan wrote:
> On 07/30/2015 12:40 PM, Andrew Dunstan wrote:
> >We should describe test sets by what they test, not by how they test. TAP

I agree with that philosophy.  I also respect the practicality of grouping by
test harness as a shorthand.  Tests sharing a harness tend to share
dependencies, harness bugs, logging mechanisms, etc.

> >is a testing tool/protocol. The current set of tests we have test the
> >programs in src/bin, and we should really name the test set by a name that
> >reflects that, rather than the fact that we are using TAP tools to run the
> >tests. What if we decide to test something else using TAP? Would we call
> >that set of tests TAP tests too?

Yes.  "TAP test" refers to any test in a suite written for the "prove"
harness, including the existing suite outside src/bin:

$ find . -name t
./src/bin/pg_controldata/t
./src/bin/scripts/t
./src/bin/pg_rewind/t
./src/bin/pg_basebackup/t
./src/bin/pg_ctl/t
./src/bin/pg_config/t
./src/bin/initdb/t
./src/test/ssl/t

> >--enable-tap-tests is a reasonable configuration setting, because it's
> >about whether or not we have a TAP testing framework available, but I
> >think we should stop calling the bin tests "TAP tests" and we should
> >change the test name in vcregress.pl to a more appropriate name. In the
> >buildfarm I'm calling the step "bin-check": 
> >
> >
> >Thoughts?

While lack of granularity in vcregress.pl is hostile, with so much about MSVC
development being hostile, this one is below my noise floor.

> In fact, looking more closely at the changes that have been made to
> vcregress.pl, I don't really like the way this has been done. I'm putting
> together some changes to bring things more into line with how the Makefiles
> work.

The commit history of vcregress.pl shows that doing good here is difficult,
and the rewards are low.  If you wish to write tightly-focused patches to
align vcregress.pl with the experience of testing with GNU make, I am
cautiously optimistic.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TAP tests are badly named

[Andrew Dunstan]

On 07/30/2015 12:40 PM, Andrew Dunstan wrote:


We should describe test sets by what they test, not by how they test. 
TAP is a testing tool/protocol. The current set of tests we have test 
the programs in src/bin, and we should really name the test set by a 
name that reflects that, rather than the fact that we are using TAP 
tools to run the tests. What if we decide to test something else using 
TAP? Would we call that set of tests TAP tests too?


--enable-tap-tests is a reasonable configuration setting, because it's 
about whether or not we have a TAP testing framework available, but I 
think we should stop calling the bin tests "TAP tests" and we should 
change the test name in vcregress.pl to a more appropriate name. In 
the buildfarm I'm calling the step "bin-check": 



Thoughts?





In fact, looking more closely at the changes that have been made to 
vcregress.pl, I don't really like the way this has been done. I'm 
putting together some changes to bring things more into line with how 
the Makefiles work.


cheers

andrew





--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] TAP tests are badly named

[Andrew Dunstan]

We should describe test sets by what they test, not by how they test. 
TAP is a testing tool/protocol. The current set of tests we have test 
the programs in src/bin, and we should really name the test set by a 
name that reflects that, rather than the fact that we are using TAP 
tools to run the tests. What if we decide to test something else using 
TAP? Would we call that set of tests TAP tests too?


--enable-tap-tests is a reasonable configuration setting, because it's 
about whether or not we have a TAP testing framework available, but I 
think we should stop calling the bin tests "TAP tests" and we should 
change the test name in vcregress.pl to a more appropriate name. In the 
buildfarm I'm calling the step "bin-check": 



Thoughts?

cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers