TAP tests are badly named

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":
<http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=crake&amp;dt=2015-07-30%2012%3A25%3A58&amp;stg=bin-check&gt;

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

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": <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=crake&amp;dt=2015-07-30%2012%3A25%3A58&amp;stg=bin-check&gt;

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

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": <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=crake&amp;dt=2015-07-30%2012%3A25%3A58&amp;stg=bin-check&gt;

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

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": <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=crake&amp;dt=2015-07-30%2012%3A25%3A58&amp;stg=bin-check&gt;

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

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":
<http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=crake&amp;dt=2015-07-30%2012%3A25%3A58&amp;stg=bin-check&gt;

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

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

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

On Fri, Aug 14, 2015 at 1:18 AM, Andrew Dunstan <andrew@dunslane.net> 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 GetFakeConfigure@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

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

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

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

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:
/messages/by-id/55B90161.5090506@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

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:
/messages/by-id/55B90161.5090506@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

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:
/messages/by-id/55B90161.5090506@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

On Mon, Aug 17, 2015 at 7:15 AM, Noah Misch <noah@leadboat.com> 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:
/messages/by-id/55B90161.5090506@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

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

On Mon, Aug 17, 2015 at 7:15 AM, Noah Misch <noah@leadboat.com> 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:
/messages/by-id/55B90161.5090506@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