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

[Jakub Narebski <jnareb@gmail.com>]

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@gmail.com
> +	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