Thread: TAP tests are badly named
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&dt=2015-07-30%2012%3A25%3A58&stg=bin-check> Thoughts? cheers andrew
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&dt=2015-07-30%2012%3A25%3A58&stg=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
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&dt=2015-07-30%2012%3A25%3A58&stg=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.
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&dt=2015-07-30%2012%3A25%3A58&stg=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
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&dt=2015-07-30%2012%3A25%3A58&stg=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.
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&dt=2015-07-30%2012%3A25%3A58&stg=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
Attachment
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
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
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
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.
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
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
Attachment
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.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.
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.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
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.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.
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:
>> >http://www.postgresql.org/message-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
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:
>>>> http://www.postgresql.org/message-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