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

From: Lea Wiemann <lewiemann@gmail.com>
Date: Jun 29, 2008, 23:39 UTC
Message-ID: <48681D21.1040302@gmail.com>
In-Reply-To: <200806300047.12224.jnareb@gmail.com>

Jakub Narebski wrote:

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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