Re: [PATCH v8] gitweb: add test suite with Test::WWW::Mechanize::CGI

[Lea Wiemann <lewiemann@gmail.com>]

Jakub Narebski wrote:
> On Thu, 26 June 2008, Lea Wiemann wrote:
>> It also uses HTML::Lint, XML::Parser, and Archive::Tar (if present, each)
> 
> Wouldn't it be better to use "(each if present)"?
> I am not a native English speaker, though.

Me neither, but I prefer "if present, each" off the top of my head...

>> - Add simple page caching (reduces execution time without --long-tests
>>   by 25%).  That's *really* helpful when you have to run those tests
>>   on a regular basis. ;)
> 
> What do you need caching for?  You check if page was already accessed
> when spidering...

This is actually about the short tests (I don't think it gains much for
spidering, percentage-wise).  The test suite uses some repetitive set-up
code (like get_summary && follow_link 'tree' && ...), so the second and
third times these pages are loaded we can save some time.

> replace $mech->page_links_ok [with] finding all the links [...], 
> then filtering out links which you have checked already, then checking
> selected links using $mech->links_ok

That's basically what it does right now. :)

>> +package OurMechanize;
>> +use base qw( Test::WWW::Mechanize::CGI );
> 
> Why this package is not named WWW::Mechanize::CGI::Cached, I wonder?
> Is it because of corrected cgi_application method?

Yeah, basically. :)  Plus, it's not to be confused with a proper
implementation of a TWM::CGI::Cached package.  (For instance, it uses
the URL rather than the complete request as cache key.  Hence it ignores
headers like Referer; but that's great for the Gitweb tests since TWM
apparently sets the Referer, but Gitweb doesn't check it, so there's no
need to refetch a page because the Referer has changed.)

> By the way, if you want to add a comment to mentioned WM::CGI ticket [...]

Thanks; done.

>> - Follow redirects rather than failing.
> 
> Nice. Where it is?

test_page doesn't use $mech->get_ok anymore, but rather calls $mech->get
and checks that the status is [23][0-9][0-9].  If it's 3xx, it also
follows the redirect.

> I begin to wonder if
> splitting this test into smaller part wouldn't be a good idea...

It's not yet painful (or long-running) enough for me to care. :)  I'm
fine with a 500-lines test module.

>> +# Diff formattting problem.
> 
> (One 't' too many:

Fixed.

>> +			follow_link( { url_abs_regex => qr/a=blob_plain/ },
>> +				     'linked file name');  # bang
>  
> I'll try to investigate; I guess this uses wrong name or wrong hash
> for preimage.

Thanks!

>> - Do not test correctness of line number fragments (#l[0-9]+); they're
>>   broken too often right now.
> 
> What do you mean by broken?

This is only visible with --long-tests -- there are links to line
numbers that don't exist (IOW the fragment doesn't exist in a name or id
attribute on the target page).  I've even seen links to #l0.  Bug fixes
welcome. ;-)

(There are also some other [mostly minor] issues marked with "TODO:" in
the test code; I didn't want to swamp the list with half a dozen bug
reports though, apart from time constraints on my end.)

> Good work!

Thanks! :-)  I'll send v9 in (hopefully) 2-3 days, together with the
first working version of the caching code.

-- Lea