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

[Lea Wiemann <lewiemann@gmail.com>]

[Open issue about PERL_PATH at the bottom!]

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

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

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

>> +chomp(my @files = map { (split("\t", $_))[1] } `git-ls-tree HEAD`);
> 
> Wouldn't it be easier to use "git ls-tree --names-only HEAD"?

Yup ('--name-only' though).  Wonder why I didn't use it -- fixed.

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

>> +my $cgi = sub {
>> +	# Use exec, not the shell, to support blanks in the path.
> 
> blanks = embedded whitespace?  path = path to gitweb?

Yup.  Improved comment, and simplified system call:

-	# Use exec, not the shell, to support blanks in the path.
-	my $status = system { $ENV{GITPERL} } $ENV{GITPERL}, $gitweb;
+	# Use exec, not the shell, to support embedded whitespace in
+	# the path to gitweb.cgi.
+	my $status = system $ENV{GITPERL}, $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.

>> +	# 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');

>> [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.  This means that while we can control under 
which Perl version gitweb.cgi runs, we cannot control under which Perl 
version the test suite runs (at least without $PATH trickery).  Does 
this bother us?

If yes, I'd suggest we keep GITPERL but rename it to GIT_TEST_PERL, 
because that's what it's about.  If not, I'll rip it out and simply call 
'perl' in the test shell script, whatever version it may be.

-- Lea