Perl's not the only TAP source

[Steve Purkis]

On 18 May 2009, at 00:09, David E. Wheeler wrote:

> On May 17, 2009, at 5:34 AM, Steve Purkis wrote:
>
>> No problem... thanks again for the feedback (and the patience!), it
>> helps to see how others are using the module when designing a
>> solution.  It seems you've run into similar problems to the ones I've
>> run into, which is good in a way - validates the need for a solution.
>
> Yah. Now I’ll try to remember what I looked at before when I made my
> original comments. :-)

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


[snip]
>> So already you can pass in things that aren't files, it's just
>> TAP::Parser doesn't know what to do with them.  Hence the
>> detectors...  all an SQL detector would need to do is look for SQL
>> statements, and then cast its vote.
>
> Yes, but my point is that sometimes what I’ll pass in *will* be files,
> and they won’t be Perl scripts. So there needs to be some way to
> determine what to do with them.

Yup.  You can already do that with what I've got so far.  Though  
judging by the points you make below, I think it'll be the least of  
our worries ;-).


>> Which brings me to a question:  What do people think about the voting
>> system?  [snip!]
>
> 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.


>>> Also, how would a source class collect options from the user? For
>>> example, pgTAP needs a database name username, and possibly a host
>>> name and port to connect to.
>>
>> Good point... I don't think it should be the source detector's
>> problem, but should be solved at the plugin level.  We just need to
>> make sure there's enough flexibility in TAP::Parser to allow for
>> different configurations...
>
> Well, there are two parts of this, essentially:
>
> 1. Determining what the source is
> 2. Getting TAP from the source
>
> For raw TAP, 2 obviously just returns the tap. For a file name, it
> needs to determine what type of file it is, and thus how it should be
> executed. So if that’s not the detector’s job, maybe there’s an
> executor method in each detector class?

Funny that, there's actually a set of TAP::Parser::Iterator classes  
that act as executors of a kind...  The difference between them &  
sources is fuzzy to me, I was actually wondering if they should be  
merged to make sub-classing easier.

Anyway, I'll answer this one in a sec.  read on...

[snip]
>> http://search.cpan.org/~andya/Test-Harness-3.17/lib/App/Prove.pm#PLUGINS
>
> It says that plugins can’t process command-line arguments, which is a
> major missing feature.

Yeah.  Last time I tried to convince everyone about that I failed to  
gain enough momentum:

http://www.hexten.net/pipermail/tapx-dev/2009-February/002522.html

This is a conversation for a separate thread really.


> 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.

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.

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


>> I wrote this specifically to get around the T::F::H issues above, but
>> it should be generic enough for other plugin authors to use... let us
>> know if not!
>
> I’m not clear on what I’d do with it. Would I use it to set up stuff
> to be passed to `exec`? Because if I have both Perl and pgTAP tests in
> a test suite, each test needs to be checked to see how it should be
> executed. Can that be done with a plugin? And can I specify the plugin
> commands in a Build.PL file somehow?

It would only work from prove, but I was thinking:

	prove -PpgTAP=host,bob,dbname,bill,user,mary,pass,gill

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.

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.

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 ;-).


>> I'd like that to be the case... If three of us already ran into the
>> same problem, it's a common use case that we'll have to solve &
>> document.
>
> 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 ;-)


[snip - why sources & detectors are different]
>> So unless anyone's got any big problems, I'm happy to combine 'em so
>> you only need to write 1 class.
>
> Yeah, I think that’d be good, then it’ll be easier to document how to
> write new ones by just saying what methods they should have and what
> those methods should return.

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.

(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)


>> 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.


>> The problem with integrating this into TAP::Parser is that 'sources'
>> and 'execs' are treated differently.  They shouldn't be.
>
> Perhaps that should be normalized, then.

That's where I left off yesterday... tricky to maintain backwards  
compat here, but I think it's do-able.


>>> It looks good, I'm thrilled you're taking this on, and I look  
>>> forward
>>> to putting it to good use!
>>
>> Thanks, all the more reason to do it then :)
>
> Yah. I see you’ve been hacking today, too, so if anything I’m saying
> is moot, don’t hesitate to say so. After all, you’re actually
> contributing code, and I’m just some asshole who wants it to work for
> him and so is yammering on about it. :-)

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


>>>   my $build = $class->new(
>>>        ...
>>>       tap_harness_args   => { psql => [qw( psql -d try -f )] },
>>>   );
>>>
>>> Where the `psql` parameter is something that the pgTAP source could
>>> then use to run its tests, but that the Perl source would ignore.
>>
>> I see what you're getting at... I must admit I don't know how  
>> feasible
>> that is right now.  There are two big issues:
>>
>> 1. TAP::Harness doesn't support custom args.
>
> Nor should it. But the `exec` parameter should be modifiable. What my
> Module::Build subclass actually has is:
>
>     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.

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...

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.

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?

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

There's a couple of options I suggested earlier, let's talk about them  
below.


>> 2. M::B doesn't use App::Prove, and hence the entire plugin suite.   
>> So
>> how can you load things that use custom sources?
>
> I don’t think that App::Prove is the right place to be detecting
> sources. The harness is.

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.


>> Here are a few options for getting around #1:
>>
>> (A) Use environment vars.
>> Certainly the easiest option at the moment.  Not the cleanest,
>> perhaps.
>
> Bleh.

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.

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


>> (B) Sub-class the Harness.
>> As you've done, I guess.  But does M::B let you use different harness
>> classes?
>
> No, I’ve not subclassed the harness.  [snip]

Good - I don't think you should have to either.


> 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.


>> (B) Let TAP::Harness take user-defined config, eg:
>>
>> 	my $harness = TAP::Harness->new({ config => { ... } });
>>
>> The sources (and other classes?) could then be given access to that
>> config.
>
> 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’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?

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


>> #2 is a much bigger problem.  The only way I can see around this is  
>> by
>> going back to the idea of auto-loaded source detectors that load a
>> plugin when they match.  The downside is that this complicates the
>> implementation, and could open up TAP::Harness to performance &
>> security issues.  To mitigate, we could do some fancy stuff like
>> defining source detection rules, storing them in YAML and only  
>> loading
>> them initially...  again, it would complicate the implementation
>> though, perhaps unnecessarily.
>
> Yeah, this seems like overkill to me.

Yeah.  But is the


> 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.


> 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)] },
             },
         },
     }



> 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.

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


> Each source would be able to look at the string passed to it (whether
> a file name or just a string), and return a stream object that
> TAP::Parser can read. I’d be able to pass options to each source, as
> appropriate. The detector would know whether or not it could do
> something with a source, and would be able to use the arguments I
> passed to construct the appropriate stream object.
>
> Does that make sense?

Yes it does, thanks.


>> 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.

Cheers,
+--
   Steve Purkis



-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.hexten.net/pipermail/tapx-dev/attachments/20090519/fb96abd8/attachment-0001.htm