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 to read something like that (I don't have good solution): +# Counting tests upfront is difficult, as number of tests depends +# on existence of several Perl modules, and is next to impossible +# when spidering gitweb output (for --long-test). Additionally, +# having number of tests planned stated at beginning is not necessary, +# as this test is to be run from git test suite, and not to be +# processed further by TAP (Test Anything Protocol) Perl modules. See http://www.perlfoundation.org/perl5/index.cgi?testing for explanation why (and when) 'plan' is useful: TAP output usually starts with a plan line: 1..9 which specifies how many tests are to follow. The above example specifies 9 tests. This line is optional, but recommended. Should a test suite die part-way through, the plan allows the testing framework to recognise this situation, rather than assuming that all tests were completed. > +our $long_tests = $ENV{GIT_TEST_LONG}; Here also it could be "my $long_tests"; but I am not a Perl hacker, and do not know which is preferred. > +eval { require Archive::Tar; }; > +my $archive_tar_installed = !$@ > + or diag('Archive::Tar is not installed; no tests for valid snapshots'); > +chomp(my @heads = map { (split('/', $_))[2] } `git-for-each-ref --sort=-committerdate refs/heads`); > +chomp(my @tags = map { (split('/', $_))[2] } `git-for-each-ref --sort=-committerdate refs/tags`); Wouldn't be easier to use '--format=%(refname)' in git-for-each-ref invocation? Besides, gitweb now uses in link _full_ refname, i.e. "refs/heads/<name>" and "refs/tags/<name>" to avoid branch / tag ambiguity (when there are both branch and head with the same name). > +# files and directories in HEAD root: > +chomp(my @files = map { (split("\t", $_))[1] } `git-ls-tree HEAD`); Wouldn't it be easier to use "git ls-tree --names-only HEAD"? > +sub rev_parse (...) > +sub get_type (...) Nice. > +my $gitweb = abs_path(File::Spec->catfile('..', '..', 'gitweb', 'gitweb.cgi')); > + > +# 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. Errr... I think that without my comment you removed it is hard to understand what this comment talks about. "Thus ..." without any preceding sentence? > +my $cgi = sub { > + # Use exec, not the shell, to support blanks in the path. blanks = embedded whitespace? path = path to gitweb? > + my $status = system { $ENV{GITPERL} } $ENV{GITPERL}, $gitweb; > + my $value = $status >> 8; > + > + croak( qq/Failed to execute application '$gitweb'. Reason: '$!'/ ) > + if ( $status == -1 ); > + croak( qq/Application '$gitweb' exited with value: $value/ ) > + if ( $value > 0 ); > +}; > + > +my $mech = new Test::WWW::Mechanize::CGI; > +$mech->cgi($cgi); Looks good. Thanks. > + # 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? > + return 1 Style: "return 1;" > +# RSS/Atom/OPML view. Simply retrieve and check. > +{ > + # Broken link in Atom/RSS view -- cannot spider: > + # http://mid.gmane.org/485EB333.5070108@xxxxxxxxx > + local $long_tests = 0; > + test_page('?p=.git;a=atom', 'Atom feed'); > + test_page('?p=.git;a=rss', 'RSS feed'); > +} > +test_page('?a=opml', 'OPML outline'); This I think should be written using Test::More equivalent of test_expect_failure in t/test-lib.sh, namely TODO block, either as TODO: { local $TODO = "why"; ...normal testing code... } or if it causes problem with t9503-gitweb-Mechanize.sh failing, perhaps as TODO: { todo_skip "why", <n> if <condition>; ...normal testing code... } (And similarly for other pages). > +# Blob view I like those comments. > diff --git a/t/test-lib.sh b/t/test-lib.sh [...] > +GITPERL=${GITPERL:-perl} > +export GITPERL How it is different from PERL_PATH? -- Jakub Narebski ShadeHawk on #git Thorn, Poland -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
