Perl's not the only TAP source

[Steve Purkis]

On 14 Apr 2009, at 02:03, David E. Wheeler wrote:

> On Apr 13, 2009, at 3:22 PM, Steve Purkis wrote:
>
>> There's a workable draft in trunk.  Before you panic, don't: it's
>> not yet used by TAP::Parser, I just wanted to commit something as a
>> discussion point.
>
> Nice, thanks!

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.


>> If this is the right way, to finish this I reckon we'd need to:
>>
>> 1. make TAP::Parser::Source more generic
>> * not everything's a cmd anymore
>> * so subclassing 'pgTAP' would make sense
>
> Yeah, this is obviously something I'd be interested in contributing.
> On a quick look, it appears that you essentially rely on file names to
> decide what to do with things, yes? So would we need to standardize on
> some kind of file extension for that? Is that really the position we
> want to take?

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

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.

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.


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

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

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!


> The current implementation of [pg_prove]
> [] collects this information, assembles a command, and then simply
> passes it via the `exec` parameter. How would I go about using a
> source to do the same stuff?
>
> [pg_prove]: http://github.com/theory/pgtap/raw/master/bin/pg_prove
>
> Maybe my questions will be mooted by the subclassing guidee.

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.


> However, I'm also not convinced that this needs to be quite so, um,
> complex. Do I really have to implement two different classes (Source
> and SourceDetector) to add support for a new source? Why not just one
> module or class with the necessary goodies in it?

Yeah, dunno really.

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.

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.


>> 2. integrate the SourceFactory into TAP::Parser
>> * replace all the source stuff in _initialize() with it
>> * maybe replace the 'exec' stuff too?
>
> I guess you'd add an Exec source?

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.


>> ** if not, introduce an 'exec_source_class'
>>
>> 3. update docs
>> * subclassing guide
>>
>> All of this requires a deprecation cycle... which means a release.
>> I suppose all the work could be done in a sub-class of TAP::Parser.
>
> What backwards compatibility would be broken?

The source_class & perl_source_class & related methods in TAP::Parser.


>> What do people think - is this the right way forward?
>
> 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 :)

> It should greatly simplify how I set this stuff up in a Build.PL file,
> which right now looks like this:
[snip!]
>     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.
2. M::B doesn't use App::Prove, and hence the entire plugin suite.  So  
how can you load things that use custom sources?

Here are a few options for getting around #1:

(A) Use environment vars.
Certainly the easiest option at the moment.  Not the cleanest, perhaps.

(B) Sub-class the Harness.
As you've done, I guess.  But does M::B let you use different harness  
classes?

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

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

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.

Cheers,
-Steve