<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On 18 May 2009, at 00:09, David E. Wheeler wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>On May 17, 2009, at 5:34 AM, Steve Purkis wrote:<br><br><blockquote type="cite">No problem... thanks again for the feedback (and the patience!), it<br></blockquote><blockquote type="cite">helps to see how others are using the module when designing a<br></blockquote><blockquote type="cite">solution. It seems you've run into similar problems to the ones I've<br></blockquote><blockquote type="cite">run into, which is good in a way - validates the need for a solution.<br></blockquote><br>Yah. Now I’ll try to remember what I looked at before when I made my <br>original comments. :-)</div></blockquote><div><br></div><div>heh, I'm trying to reply quicker this time :-)</div><div><br></div><div><br></div>[snip]</div><div><blockquote type="cite"><div><blockquote type="cite">So already you can pass in things that aren't files, it's just<br></blockquote><blockquote type="cite">TAP::Parser doesn't know what to do with them. Hence the<br></blockquote><blockquote type="cite">detectors... all an SQL detector would need to do is look for SQL<br></blockquote><blockquote type="cite">statements, and then cast its vote.<br></blockquote><br>Yes, but my point is that sometimes what I’ll pass in *will* be files, <br>and they won’t be Perl scripts. So there needs to be some way to <br>determine what to do with them.</div></blockquote><div><br></div><div>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 ;-).</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite">Which brings me to a question: What do people think about the voting<br></blockquote><blockquote type="cite">system? [snip!]</blockquote></div></blockquote><blockquote type="cite"><div><br>It seemed like overkill to me, but minor increase in complexity aside, <br>it’s not a big deal. Since you’ve already done the work to add it, it <br>seems fine to me to leave it that way.</div></blockquote><div><br></div><div>Yeah, maybe guilty of developer gold plating. But it's the easiest way I could think of having default detectors that can be overridden.</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite"><blockquote type="cite">Also, how would a source class collect options from the user? For<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">example, pgTAP needs a database name username, and possibly a host<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">name and port to connect to.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Good point... I don't think it should be the source detector's<br></blockquote><blockquote type="cite">problem, but should be solved at the plugin level. We just need to<br></blockquote><blockquote type="cite">make sure there's enough flexibility in TAP::Parser to allow for<br></blockquote><blockquote type="cite">different configurations...<br></blockquote><br>Well, there are two parts of this, essentially:<br><br>1. Determining what the source is<br>2. Getting TAP from the source<br><br>For raw TAP, 2 obviously just returns the tap. For a file name, it <br>needs to determine what type of file it is, and thus how it should be <br>executed. So if that’s not the detector’s job, maybe there’s an <br>executor method in each detector class?</div></blockquote><div><br></div><div>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.</div><div><br></div><div>Anyway, I'll answer this one in a sec. read on...</div><div><br></div>[snip]<br><blockquote type="cite"><div><blockquote type="cite"><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; "><a href="http://search.cpan.org/~andya/Test-Harness-3.17/lib/App/Prove.pm#PLUGINS">http://search.cpan.org/~andya/Test-Harness-3.17/lib/App/Prove.pm#PLUGINS</a></span></blockquote><br>It says that plugins can’t process command-line arguments, which is a <br>major missing feature.</div></blockquote><div><br></div><div>Yeah. Last time I tried to convince everyone about that I failed to gain enough momentum:</div><div><br></div><div><a href="http://www.hexten.net/pipermail/tapx-dev/2009-February/002522.html">http://www.hexten.net/pipermail/tapx-dev/2009-February/002522.html</a></div><div><br></div><div>This is a conversation for a separate thread really.</div><div><br></div><br><blockquote type="cite"><div> Furthermore, this can’t be limited just to <br>prove: I need to be able to configure for multiple sources in a <br>Build.PL file, for example.</div></blockquote><div><br></div>Right, that's the key, and the really hard bit.</div><div><br></div><div>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.</div><div><br></div><div>There are a few proposed solutions below, will talk about them there.</div><div><br></div><div><br><blockquote type="cite"><div><blockquote type="cite">I wrote this specifically to get around the T::F::H issues above, but<br></blockquote><blockquote type="cite">it should be generic enough for other plugin authors to use... let us<br></blockquote><blockquote type="cite">know if not!<br></blockquote><br>I’m not clear on what I’d do with it. Would I use it to set up stuff <br>to be passed to `exec`? Because if I have both Perl and pgTAP tests in <br>a test suite, each test needs to be checked to see how it should be <br>executed. Can that be done with a plugin? And can I specify the plugin <br>commands in a Build.PL file somehow?</div></blockquote><div><br></div><div>It would only work from prove, but I was thinking:</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>prove -PpgTAP=host,bob,dbname,bill,user,mary,pass,gill<br></div><div><br></div><div>Let's disregarding the fact that looks like crap for the mo. This is what it would do:</div><div><br></div><div>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.</div><div><br></div><div>So that meets your first requirement:</div><div>* both Perl and pgTAP tests in a test suite</div><div><br></div><div>Unfortunately it doesn't meet the second:</div><div>* supporting it from your Build.PL.</div><div><br></div><div>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 ;-).</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite">I'd like that to be the case... If three of us already ran into the<br></blockquote><blockquote type="cite">same problem, it's a common use case that we'll have to solve &<br></blockquote><blockquote type="cite">document.<br></blockquote><br>Yes, and provide a number of default implementations (pgTAP, Perl, <br>RubyTAP, PyTAP, etc.)</div></blockquote><div><br></div><div>Agreed. But I'm not volunteering - thinking about one implementation is taking up enough time atm ;-)</div><div><br></div><div><br></div><div>[snip - why sources & detectors are different]</div><blockquote type="cite"><div><blockquote type="cite"><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">So unless anyone's got any big problems, I'm happy to combine 'em so</span></blockquote><blockquote type="cite">you only need to write 1 class.<br></blockquote><br>Yeah, I think that’d be good, then it’ll be easier to document how to <br>write new ones by just saying what methods they should have and what <br>those methods should return.</div></blockquote><div><br></div><div>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.</div><div><br></div><div>(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)</div><div><br></div><div><br></div><blockquote type="cite"><div><blockquote type="cite">There's already one... and it's currently the base class -<br></blockquote><blockquote type="cite">TAP::Parser::Source. In my first point I was proposing moving it<br></blockquote><blockquote type="cite">around, ala:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span>TAP::Parser::Source<span class="Apple-tab-span" style="white-space:pre"> </span>(abstract base class)<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>::Executable<span class="Apple-tab-span" style="white-space:pre"> </span>(from above)<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>::Perl<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>(now inherit from Executable)<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>::RawTAP<span class="Apple-tab-span" style="white-space:pre"> </span>(needed?)<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>::Archive<span class="Apple-tab-span" style="white-space:pre"> </span>(example)<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>::Pg<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>(example)</blockquote></div></blockquote><div><br></div>I've done this bit, btw.</div><div><br></div><div><br><blockquote type="cite"><div><blockquote type="cite"><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">The problem with integrating this into TAP::Parser is that 'sources'</span></blockquote><blockquote type="cite">and 'execs' are treated differently. They shouldn't be.<br></blockquote><br>Perhaps that should be normalized, then.</div></blockquote><div><br></div>That's where I left off yesterday... tricky to maintain backwards compat here, but I think it's do-able.</div><div><br></div><div><br></div><div><blockquote type="cite"><div><blockquote type="cite"><blockquote type="cite">It looks good, I'm thrilled you're taking this on, and I look forward<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">to putting it to good use!<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Thanks, all the more reason to do it then :)<br></blockquote><br>Yah. I see you’ve been hacking today, too, so if anything I’m saying <br>is moot, don’t hesitate to say so. After all, you’re actually <br>contributing code, and I’m just some asshole who wants it to work for <br>him and so is yammering on about it. :-)</div></blockquote><div><br></div><div>Your yammering is quite apt, so I won't feed you to the dogs just yet ;-)</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite"><blockquote type="cite"> my $build = $class->new(<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> ...<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> tap_harness_args => { psql => [qw( psql -d try -f )] },<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> );<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">Where the `psql` parameter is something that the pgTAP source could<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">then use to run its tests, but that the Perl source would ignore.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I see what you're getting at... I must admit I don't know how feasible<br></blockquote><blockquote type="cite">that is right now. There are two big issues:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">1. TAP::Harness doesn't support custom args.<br></blockquote><br>Nor should it. But the `exec` parameter should be modifiable. What my <br>Module::Build subclass actually has is:<br><br> sub tap_harness_args {<br> my $self = shift;<br> return {<br> exec => sub {<br> my ( $harness, $test_file ) = @_;<br> # Let Perl tests run.<br> return undef if $test_file =~ /[.]t$/;<br> return [ @{ $self->psql_test }, '-f', $test_file ]<br> if $test_file =~ /[.]s$/;<br> },<br> };<br> }<br><br>So I’m using it to override the `exec` parameter by looking at file <br>names. This should work for all kinds of tests where some external <br>program needs to be run to collect the TAP, whether it be Ruby or <br>Python or psql -- or Perl for that matter! Perl is just a specialized <br>case of `exec`, too. Or should be.</div></blockquote><div><br></div><div>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...</div><div><br></div><div>One thought just struck me:</div><div><br></div><div>If the test is a file with an extension, we could check if a module for it exists ala:</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>TAP::Parser::Source::File::$extension<br></div><div><br></div><div>If it does, you load it and use it... If one class per extension is overkill, we could use MIME types.</div><div><br></div><div>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?</div><div><br></div><div>Of course, that doesn't _completely_ solve your problem yet: how would you pass arguments to it once it was loaded?</div><div><br></div><div>There's a couple of options I suggested earlier, let's talk about them below.</div><div><br></div><div><br></div><blockquote type="cite"><div><blockquote type="cite"><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">2. M::B doesn't use App::Prove, and hence the entire plugin suite. So</span></blockquote><blockquote type="cite">how can you load things that use custom sources?<br></blockquote><br>I don’t think that App::Prove is the right place to be detecting <br>sources. The harness is.</div></blockquote><div><br></div><div>Yep, my point was App::Prove is currently the only place where plugins are loaded.</div><div><br></div><div>I think we're agreeing that this is another problem that needs solving.</div><div><br></div><div><br></div><blockquote type="cite"><div><blockquote type="cite">Here are a few options for getting around #1:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">(A) Use environment vars.<br></blockquote><blockquote type="cite">Certainly the easiest option at the moment. Not the cleanest, <br></blockquote><blockquote type="cite">perhaps.<br></blockquote><br>Bleh.</div></blockquote><div><br></div><div>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.</div><div><br></div><div>Question is: does M::B remember env vars?</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite">(B) Sub-class the Harness.<br></blockquote><blockquote type="cite">As you've done, I guess. But does M::B let you use different harness<br></blockquote><blockquote type="cite">classes?<br></blockquote><br>No, I’ve not subclassed the harness. [snip]</div></blockquote><div><br></div><div><div>Good - I don't think you should have to either.</div><div><br></div></div><br><blockquote type="cite"><div> The harness just creates a stream <br>to pass to the parser, which is perfect. What we need to do is have a <br>way to detect a source, and then create an appropriate stream for that <br>source. Really this just means passing the appropriate arguments to <br>`exec` for each test file. Nothing more.</div></blockquote><div><br></div><div>True for pgTAP, but I'm also trying to solve the general case where more logic may be needed.</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite">(B) Let TAP::Harness take user-defined config, eg:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space:pre"> </span>my $harness = TAP::Harness->new({ config => { ... } });<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">The sources (and other classes?) could then be given access to that<br></blockquote><blockquote type="cite">config.<br></blockquote><br>Here’s what I need in a Build.PL (or Makefile.PL, for that matter, <br>should it ever get added):<br><br>A way to say, “Here is a list of my test scripts, and here’s how to <br>determine what to do with each of them.” Even better would be if we <br>established some conventions so that common mixtures of tests wouldn’t <br>require anything.</div></blockquote><div><br></div><div>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?</div><div><br></div><div><br></div><blockquote type="cite"><div>I’m not seeing a need for a user-config at this point.</div></blockquote><div><br></div><div>As above, the only bit of user config I can see for pgTAP's case is database connection params. For others, who knows?</div><div><br></div><div>Part of me prefers this solution as it's cleaner than the environment vars approach. Thoughts?</div><div><br></div><div><br></div><blockquote type="cite"><div><blockquote type="cite">#2 is a much bigger problem. The only way I can see around this is by<br></blockquote><blockquote type="cite">going back to the idea of auto-loaded source detectors that load a<br></blockquote><blockquote type="cite">plugin when they match. The downside is that this complicates the<br></blockquote><blockquote type="cite">implementation, and could open up TAP::Harness to performance &<br></blockquote><blockquote type="cite">security issues. To mitigate, we could do some fancy stuff like<br></blockquote><blockquote type="cite">defining source detection rules, storing them in YAML and only loading<br></blockquote><blockquote type="cite">them initially... again, it would complicate the implementation<br></blockquote><blockquote type="cite">though, perhaps unnecessarily.<br></blockquote><br>Yeah, this seems like overkill to me.</div></blockquote><div><br></div><div>Yeah. But is the </div><div><br></div><br><blockquote type="cite"><div> Ultimately, a source can be one of two things:<br><br>1. Raw TAP<br>2. Something else<br><br>So the code just has to determine what to do with it. If it’s Raw TAP, <br>it just hands it off to the parser. Otherwise, it should pass the <br>source off to a list of source detectors, and whichever one wins <br>should then be asked for a stream object. That’s it.</div></blockquote><div><br></div>Agreed, and that's largely what happens with my changes so far.</div><div><br></div><div><br><blockquote type="cite"><div>So in my ideal world, I wouldn’t have to subclass Module::Build. I <br>could just put something like this into my Build.PL:<br><br> tap_harness_args => {<br> sources => {<br> perl => { exec => 'perl' },<br> pgTAP => { exec => [qw(psql -d foo -f)] },<br> },<br> }</div></blockquote><div><br></div>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:</div><div><br></div><div><div> tap_harness_args => {<br> config => {</div><div> sources => {<br> perl => { exec => 'perl' },<br> pgTAP => { exec => [qw(psql -d foo -f)] },<br> },<br> },<br> }</div><div><br></div><div><br></div></div><div><br><blockquote type="cite"><div>So I’m telling TAP::Harness that the possible sources in my module are <br>Perl and pgTAP. This means that TAP::Harness would only check these <br>two sources for any given file passed to it. The `sources` option <br>would default to<br><br> { perl => { exec => 'perl' }, raw => undef }<br><br>Or maybe these two always load and any others have to be specified, so <br>then they're only loaded if they're needed.</div></blockquote><div><br></div><div>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.</div><div><br></div><div>So question is: auto-load on extension/mime-type, or user configurable ala above? or both?</div><div><br></div><br><blockquote type="cite"><div>Each source would be able to look at the string passed to it (whether <br>a file name or just a string), and return a stream object that <br>TAP::Parser can read. I’d be able to pass options to each source, as <br>appropriate. The detector would know whether or not it could do <br>something with a source, and would be able to use the arguments I <br>passed to construct the appropriate stream object.<br><br>Does that make sense?</div></blockquote><div><br></div>Yes it does, thanks.</div><div><br></div><div><br><blockquote type="cite"><div><blockquote type="cite">To keep things simple, I propose finishing off the source detection as<br></blockquote><blockquote type="cite">above, and then reviewing it & deciding where to go from there. If it<br></blockquote><blockquote type="cite">doesn't solve the M::B plugin issue, it will at least provide a<br></blockquote><blockquote type="cite">framework on which to solve it.<br></blockquote><br>Thanks, I appreciate the hard work you've put into this.</div></blockquote><div><br></div><div>No worries, and likewise - your ideas have been really useful.</div><div><br></div><div>And they've kept me up far too late ;-)</div><div><br></div><div>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.</div><div><br></div></div><div><div><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; ">Cheers,<br class="Apple-interchange-newline">+--</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><font face="Helvetica" size="3" style="font: normal normal normal 12px/normal Helvetica; "> Steve Purkis</font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; "><br></div></div></div></div></div></div><br><div></div></div><div><br></div></body></html>