Perl's not the only TAP source

[David E. Wheeler]

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

> I agree - it's not the only way we'd want to be able to detect a
> source.  The good news is I wasn't planning on limiting the detectors
> to only use file names.  At the moment, all we can give them is a 'raw
> source' which is a scalar ref that may contain a filename, raw TAP, or
> potentially even an SQL statement (ok, maybe that's a bit insecure,
> say a table name :).

Yeah, that’s the way it has been for a while.

> But for sake of a quick example, currently prove complains when it
> can't determine a source:
>
> 	% prove 'SELECT * FROM foo'
> 	Cannot determine source for SELECT * FROM foo
> 	at /usr/local/lib/perl5/5.8.9/App/Prove.pm line 501
>
> 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.

> Which brings me to a question:  What do people think about the voting
> system?  From the docs I wrote:
>
> 	$detector->can_handle
> 	...
> 	Returns a number between 0 & 1 reflecting how confidently the
> 	source detector can handle the given source.  For example, C<0>
> 	means the detector cannot handle the source, C<0.5> means it
> 	may be able to, and C<1> means it definitely can.
>
> 	The detection algorithm works something like this:
>
> 	  for all registered detectors
> 	    ask them how confident they are about handling this source
> 	  choose the most confident detector
>
> 	Ties are handled by choosing the first detector.
>
> It's been a while, but IIRC one of the reasons I wanted to do that was
> to make it possible to override the default detectors.  Another was to
> default to the perl or exec detector to preserve current  
> functionality.

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.

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

> I had this problem with TAP::Formatter::HTML and solved it by copying
> TAP::Harness::Archive: use ENV vars.  Hardly pretty, but it worked.
> Now that we've refactored App::Prove to handle plugins a bit better,
> it should be possible to set vars on various TAP objects as needed.
> See:
>
> 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. 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.

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

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

> My thoughts on keeping them separate basically went like this:
>
> 1. I'll be able to introduce this gradually, without having to rewrite
> all the source classes.
> 2. You could load the source classes on demand, and so avoid loading
> half of CPAN when it never gets used.
> 3. The source detectors could be complex and so should be separate.
> 4. There may be many detectors for any given type of source.

Well, we need to make it as easy as possible for authors to write new  
detectors and evaluators. If a detector might be complex, perhaps we  
should rethink it to make it simpler.

> But I'm not sure if that applies any more...
> (1) is moot as the source classes need rewriting anyway.
> (2) is potentially moot given the use case (ie: a plugin wouldn't be
> loaded unless the user requested it).
> (3) is the detector's writer's problem
> (4) can be solved by sub-classing
>
> 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.

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

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

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

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

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

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

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

I’m not seeing a need for a user-config at this point.

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

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

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.

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?

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

Best,

David