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

[Jakub Narebski <jnareb@gmail.com>]

On Mon, 23 Jun 2008, Lea Wiemann wrote:
> Jakub Narebski wrote:
>> Lea Wiemann wrote:
>> 
>>> +# We don't count properly when skipping, so no_plan is necessary.
>>> +use Test::More qw(no_plan);
>> 
>> I'd rather either skip this comment all together, or change it [...]
> 
> I'll just remove it.  Anybody trying to add a test count will give up 
> within 30 seconds. ;-)

I just think that not having comment is better than have comment which
is not fully true, or five lines of explanations which could be
expanded further.

Simply: planning (coming with number of tests upfront) is hard, and for
this situation it doesn't give us anything.
 
>>> +our $long_tests = $ENV{GIT_TEST_LONG};
>> 
>> Here also it could be "my $long_tests";
> 
> This one is to make "local $long_tests = 0" work -- it wouldn't work 
> with a 'my' declaration.

Hmmmm...

Siednote: some time ago I though that I understood difference between
lexical scoping (my) and dynamical scoping (local)...
 
>>> +chomp(my @heads = map { (split('/', $_))[2] } 
>>> +	`git-for-each-ref --sort=-committerdate refs/heads`); 
>> 
>> Wouldn't be easier to use '--format=%(refname)' in git-for-each-ref
> 
> *checks*  No, since I want to strip the leading refs/heads/ anyway (in 
> order to test the page text, which mentions heads and tags without the 
> directory prefix) -- hence the 'split' wouldn't go away with 
> --format=%(refname).

Well, you could have replaced 'split' by s!^refs/!! ;-)))

>>> +# Thus subroutine was copied (and modified to work with blanks in the
>>> +# application path) from WWW::Mechanize::CGI 0.3, which is licensed
>>> +# 'under the same terms as perl itself' and thus GPL compatible.
> 
> That's a tyop: s/Thus/This/ (fixed).  It refers to the following "my 
> $cgi = sub".  Does the comment make sense now?

It makes sense now, thanks.

Although I guess it would be nice to have link to the ticket to the bug
(report) in WWW::Mechanize::CGI, so we could use cgi_application()
"again" if it gets fixed...

> +	my $status = system $ENV{GITPERL}, $gitweb;

...if not for the trick with explicitly calling Perl (GITPERL), to
allow testing different Perl versions with gitweb.
 
>>> +		# WebService::Validator::Feed::W3C would be nice to
>>> +		# use, but it doesn't support direct input (as opposed
>>> +		# to URIs) long enough for our feeds.
>> 
>> Could you tell us here what are the limitations?  Are those limits
>> of W3C SOAP interface for web validation, or the CPAN package mentioned?
> 
> No idea.  I tried it and it told me that the URL was too long -- I 
> suspect it's the W3C server that rejected it.  I wouldn't spend more 
> effort trying to get online validation to work; it's probably easier to 
> just occasionally validate manually. ;)  XML well-formedness tests 
> should catch most occasional breakages just fine.

Right.
 
Well, W3C feed validator provides SOAP interface we could use, using
POST of course and not GET (but it requires net connection), and also
standalone CLI validator (unfortunately in Python).

>>> +	# Broken link in Atom/RSS view -- cannot spider:
>>> +	# http://mid.gmane.org/485EB333.5070108@gmail.com
>>> +	local $long_tests = 0;
>>> +	test_page('?p=.git;a=atom', 'Atom feed');
>> 
>> This I think should be written using Test::More equivalent of
>> test_expect_failure in t/test-lib.sh, namely TODO block,
> 
> Right -- here's my new version (which still fails if the feeds die or 
> are not well-formed XML -- I'll want that when I change gitweb):
> 
> # RSS/Atom/OPML view
> # Simply retrieve and verify well-formedness, but don't spider.
> $mech->get_ok('?p=.git;a=atom', 'Atom feed') and _verify_page;
> $mech->get_ok('?p=.git;a=rss', 'RSS feed') and _verify_page;
> TODO: {
> 	# Now spider -- but there are broken links.
> 	# http://mid.gmane.org/485EB333.5070108@gmail.com
> 	local $TODO = "fix broken links in Atom/RSS feeds";
> 	test_page('?p=.git;a=atom', 'Atom feed');
> 	test_page('?p=.git;a=rss', 'RSS feed');
> }
> test_page('?a=opml', 'OPML outline');

Does it make t9503-gitweb.Mechanize.sh fail?
 
>>> [in t/test-lib.sh:]
>>> +GITPERL=${GITPERL:-perl}
>>> +export GITPERL
>>> [The idea is to easily run the test suite with different perl versions.]
>> 
>> How it is different from PERL_PATH?
> 
> Right, I didn't think of that.  PERL_PATH isn't available in the tests 
> though, it's only used internally by the Makefile to generate (among 
> other things) gitweb.cgi. [...]

That's what I was asking about.

-- 
Jakub Narebski
Poland