Perl's not the only TAP source

[David E. Wheeler]

On May 18, 2009, at 7:10 PM, Steve Purkis wrote:

> heh, I'm trying to reply quicker this time :-)

I'm at PGCon in Ottawa, travelled all day today, but I'll try to  
respond in a timely way, too.

>>
>> It seemed like overkill to me, but minor increase in complexity  
>> aside,
>> it’s not a big deal. Since you’ve already done the work to add it, it
>> seems fine to me to leave it that way.
>
> Yeah, maybe guilty of developer gold plating.  But it's the easiest  
> way I could think of having default detectors that can be overridden.

Yeah, I think it's fine. What do you think Andy? Paging Andy…

>> Furthermore, this can’t be limited just to
>> prove: I need to be able to configure for multiple sources in a
>> Build.PL file, for example.
>
> Right, that's the key, and the really hard bit.

I don't think it has to be hard.

> The plugin framework for TAP::Parser is really based around  
> App::Prove.  You essentially want to be able to customize it from  
> your Build.PL, without writing any code.  I must admit, that's not a  
> use case I'd thought of before.

Yes, I don't think they're the right fit for this.

> There are a few proposed solutions below, will talk about them there.

So will I. :-)

> It would only work from prove, but I was thinking:
>
> 	prove -PpgTAP=host,bob,dbname,bill,user,mary,pass,gill

Ew.

> Let's disregarding the fact that looks like crap for the mo.  This  
> is what it would do:
>
> The App::Prove::pgTAP plugin would load a custom  
> TAP::Parser::Source::pgTAP class, set up the db connection params  
> (either directly, or with env vars), and maybe also find the psql  
> binary (or use DBI?).  The source class would look out for *.s files  
> (and vote higher than the Perl source for them), and would thus be  
> able to execute the right command to generate the tap stream, or  
> fail gracefully.

Yeah, but I don't actually need this for prove. I wrote pg_prove to  
just pass the right stuff. No problems at all, and no need for  
anything fancy. It's the Build.PL/Makefile.PL support I really need.

> So that meets your first requirement:
> * both Perl and pgTAP tests in a test suite
>
> Unfortunately it doesn't meet the second:
> * supporting it from your Build.PL.

Right.

> Why?  Because the custom source class wouldn't be loaded anywhere.   
> That's why I was thinking of auto-loading source classes in an over- 
> engineered way ;-).

Heh.

>> Yes, and provide a number of default implementations (pgTAP, Perl,
>> RubyTAP, PyTAP, etc.)
>
> Agreed.  But I'm not volunteering - thinking about one  
> implementation is taking up enough time atm ;-)

Oh, that's fine. As long as you writ the Perl one, and it's easy to  
write new sources, other folks will do it. I'll provide the pgTAP  
source in a flash.

> Agreed, and it'll be pretty easy to do.  I've not done it yet  
> because I want to get the implementation working first.  Refactoring  
> it won't be a problem.  It might be good to simplify the use of  
> Iterators too, I'll have a think about that.

Makes sense.

> (as an aside: the Source & SourceDetector interfaces could still be  
> kept separate, but just roll the implementations & documentation  
> into single classes, might give us flexibility if we change our  
> minds in the future)

You mean two classes in a single file? I still don't follow why they'd  
need to be separate, but maybe I'm just not thinking fare enough along.

>>> There's already one... and it's currently the base class -
>>> TAP::Parser::Source.  In my first point I was proposing moving it
>>> around, ala:
>>>
>>> 	TAP::Parser::Source	(abstract base class)
>>> 		::Executable	(from above)
>>> 		::Perl		(now inherit from Executable)
>>> 		::RawTAP	(needed?)
>>> 		::Archive	(example)
>>> 		::Pg		(example)
>
> I've done this bit, btw.

Excellent. So ::Ruby, ::Python, ::PHP, and ::pgTap would all inherit  
from ::Executable. Fine.

> Your yammering is quite apt, so I won't feed you to the dogs just  
> yet ;-)

Arrrrrooooooooow!

>
>>     sub tap_harness_args {
>>         my $self = shift;
>>         return {
>>             exec => sub {
>>                 my ( $harness, $test_file ) = @_;
>>                 # Let Perl tests run.
>>                 return undef if $test_file =~ /[.]t$/;
>>                 return [ @{ $self->psql_test }, '-f', $test_file ]
>>                     if $test_file =~ /[.]s$/;
>>             },
>>         };
>>     }
>>
>> So I’m using it to override the `exec` parameter by looking at file
>> names. This should work for all kinds of tests where some external
>> program needs to be run to collect the TAP, whether it be Ruby or
>> Python or psql -- or Perl for that matter! Perl is just a specialized
>> case of `exec`, too. Or should be.

This, BTW, was why I added the coderef support for `exec`. It solved a  
world of problems, though it's still kind of tricky to use, since I  
still have to subclass Module::Build to make it work right. Still,  
it's do-able the way it is, and flexible enough to support anything.  
It's just not intuitive to use. It should be about configuration, not  
writing code.

> Yep - Perl is just a special case of Excec.  Of course, it's a bit  
> more complicated than that when you get into the guts of things, as  
> --exec is obviously an exec, whereas --source could be anything.  I  
> digress...

Yeah, there're hysterical reasons for that, I expect. I blame Ovid.

> One thought just struck me:
>
> If the test is a file with an extension, we could check if a module  
> for it exists ala:
>
> 	TAP::Parser::Source::File::$extension
>
> If it does, you load it and use it...  If one class per extension is  
> overkill, we could use MIME types.

How do you identify the MIME type of a file other than by looking at  
its suffix?

> Of course, now we get back into the security & performance issues  
> again.  But I'm starting to wonder if they even matter.  Andy, any  
> thoughts?

What are those issues again? I must've missed that bit.

> Of course, that doesn't _completely_ solve your problem yet: how  
> would you pass arguments to it once it was loaded?

Right.

> Yep, my point was App::Prove is currently the only place where  
> plugins are loaded.
>
> I think we're agreeing that this is another problem that needs  
> solving.

Steve++.

> Yes, but is it really that horrible?  For pgTAP I'm thinking  
> database connection params only.  The pgTAP source class should be  
> clever enough to figure out where psql is & piece together the exec  
> command.

I don't want to make users set environment variables in Build.PL/ 
Makefile.PL files to run tests when there are perfectly good Perl  
variables to be had.

> Question is: does M::B remember env vars?

Not sure. Probably.

>> The harness just creates a stream
>> to pass to the parser, which is perfect. What we need to do is have a
>> way to detect a source, and then create an appropriate stream for  
>> that
>> source. Really this just means passing the appropriate arguments to
>> `exec` for each test file. Nothing more.
>
> True for pgTAP, but I'm also trying to solve the general case where  
> more logic may be needed.

Sure. Either way, though, it's just, "What source class knows how to  
create an appropriate stream given the source argument?" And "How can  
we get configuration information to that class?" Simple as that.

>> Here’s what I need in a Build.PL (or Makefile.PL, for that matter,
>> should it ever get added):
>>
>> A way to say, “Here is a list of my test scripts, and here’s how to
>> determine what to do with each of them.” Even better would be if we
>> established some conventions so that common mixtures of tests  
>> wouldn’t
>> require anything.
>
> That summarizes it perfectly.  I'm hoping the sub-classable sources  
> that are auto-loaded depending on extension/mime-type would solve  
> your problem.  But it is late - am I on crack?

I don't know. What's in the pipe? What you're saying seems perfectly  
sane to me.

>> I’m not seeing a need for a user-config at this point.
>
> As above, the only bit of user config I can see for pgTAP's case is  
> database connection params. For others, who knows?

Oh, that kind of config. Sorry, got confused.

> Part of me prefers this solution as it's cleaner than the  
> environment vars approach.  Thoughts?

+1

>>
>> Yeah, this seems like overkill to me.
>
> Yeah.  But is the

Now the crack kicks in. ;-)

>> Ultimately, a source can be one of two things:
>>
>> 1. Raw TAP
>> 2. Something else
>>
>> So the code just has to determine what to do with it. If it’s Raw  
>> TAP,
>> it just hands it off to the parser. Otherwise, it should pass the
>> source off to a list of source detectors, and whichever one wins
>> should then be asked for a stream object. That’s it.
>
> Agreed, and that's largely what happens with my changes so far.

Awesome.

>> So in my ideal world, I wouldn’t have to subclass Module::Build. I
>> could just put something like this into my Build.PL:
>>
>>     tap_harness_args => {
>>         sources => {
>>             perl  => { exec => 'perl' },
>>             pgTAP => { exec => [qw(psql -d foo -f)] },
>>         },
>>     }
>
> Yeah, I think we're on a similar train of though here.  I would just  
> consider that config for the sources, and would phrase it like:
>
>     tap_harness_args => {
>         config => {
>             sources => {
>                 perl  => { exec => 'perl' },
>                 pgTAP => { exec => [qw(psql -d foo -f)] },
>             },
>         },
>     }

That's great, although the extra level of hash seems unnecessary to  
me. Why not `sources`, especially if, in your example, `sources` is  
the only supported key in the `config` hash?

>> So I’m telling TAP::Harness that the possible sources in my module  
>> are
>> Perl and pgTAP. This means that TAP::Harness would only check these
>> two sources for any given file passed to it. The `sources` option
>> would default to
>>
>>     { perl => { exec => 'perl' }, raw => undef }
>>
>> Or maybe these two always load and any others have to be specified,  
>> so
>> then they're only loaded if they're needed.
>
> I like that idea... it's an easy way of solving the 'how do you know  
> which sources to load?' question, and gets around the performance,  
> security (non-?) issue.  And I can easily see there being a default  
> set of sources always loaded.

Yes, the module maintainer should know what kinds of tests he runs,  
and therefore what kinds of source configurations are required. This  
would be my preferred approach.

> So question is: auto-load on extension/mime-type, or user  
> configurable ala above?  or both?

My original suggestion for this, before I sent the patch for the  
coderef arg to `exec`, but there was resistance to a formalized list  
of filename extensions. I think that MIME types have the same problem.  
For example, .sql or text/sql both mean SQL scripts, but are they for  
PostgreSQL, MySQL, SQLite, SQL Server, Oracle, or what?

So probably just the user specification. I'm fine with a mapping of  
suffixes myself, but as I said, there has been some resistance. Not  
finding it now, unfortunately…oh, wait, here it is, starting here:

   http://testanything.org/pipermail/tap-l/2008-June/000238.html

But I could see supporting a standard mapping, such as:

.pl - Perl
.p6 - Perl 6
.pasm - Parrot
.rb - Ruby
.php - PHP
.py - Python
.pg - pgTAP
.my - MyTAP
.lt - SQLite
.js - JavaScript

Or something like that. Perl could also still be .t, of course, for  
hysterical reasons.

>> Does that make sense?
>
> Yes it does, thanks.

Oh, good. :-)

>>> To keep things simple, I propose finishing off the source  
>>> detection as
>>> above, and then reviewing it & deciding where to go from there.   
>>> If it
>>> doesn't solve the M::B plugin issue, it will at least provide a
>>> framework on which to solve it.
>>
>> Thanks, I appreciate the hard work you've put into this.
>
> No worries, and likewise - your ideas have been really useful.
>
> And they've kept me up far too late ;-)
>
> I think we're closing in on a solution, if you've more ideas lemme  
> know.  Will look at putting something together later in the week.

Sounds good.

Best,

David