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

From: Lea Wiemann <lewiemann@gmail.com>
Date: Jun 23, 2008, 17:57 UTC
Message-ID: <485FE3F7.4040102@gmail.com>
In-Reply-To: <200806231531.13082.jnareb@gmail.com>

[Open issue about PERL_PATH at the bottom!]

Jakub Narebski wrote:

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

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