Re: [WIP/PATCH v3] gitweb: add test suite with Test::WWW::Mechanize::CGI

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 20 Jun 2008, Lea Wiemann wrote:

> From: Jakub Narebski <jnareb@gmail.com>

If you have rewritten the patch I won't mind if you change authorship
to you (remove this From: line); signoff is IMHO enough.

> ---
> I haven't gotten around to merging Jakub's recent XML validation patch
> yet, but I think it'd be good to have some review now; I'll merge it
> tomorrow and send another patch.

First, it is not XML validation[*1*], but check if XML is well-formed.
Second, I think it would be better if adding XML checks (RSS, Atom,
OPML) would be left as separate commit.
 
[*1*] There is W3C feed validation service, http://validator.w3.org/feed/
which provides also standalone Python script for feed validation, and
also SOAP interface with WebService::Validator::Feed::W3C wrapper (but
it would require having Internet connection), or we can try validate
against XML Schema Definition (there exists one for RSS, and there is
one for Atom derived from Relax-NG schema).

> diff --git a/t/t9503-gitweb-Mechanize.sh b/t/t9503-gitweb-Mechanize.sh
[...]
> +# set up test repository
> +test_expect_success 'set up test repository' '
[cut]

I have created this part as a copy of older t9500 gitweb test, thinking
about what we might want to test, but the WIP of Mechanize based t9503
doesn't have yet tests for those specific features.

> diff --git a/t/t9503/test.pl b/t/t9503/test.pl
[...]
> +# We don't count properly when skipping, so no_plan is necessary.
> +use Test::More qw(no_plan);

Actually it is not that we cannot could properly when skipping, because
there are two ways to have skipped tests and test count upfront, both
implemented in my initial version (v1) of t/t9503/test.pl.

You can set number of tests upfront depending if some feature is
available, e.g. "plan tests => <n> - $use_lint*<m2> - $check_xml*<m2>"
or just "if ($use_lint) { plan tests => <m> } else ...".

Or you can explicitly skip tests using SKIP: BLOCK (see Test::More(3pm)).


What we have here is that we don't know number of tests upfront,
because it is complicated, and that's what I'd like to see in the
comment instead (but I don't have idea for exact wording).

> +my @revisions = map { substr $_, 0, 10 } split /\s/, `git-rev-list --first-parent HEAD`;

Why do you use shortened SHA1 identifier, instead of full SHA1? Links
in gitweb use full SHA1.

> +my $head = $revisions[-1];

Errr, HEAD would be $revisions[0], $revisions[-1] would be $root.

> +my $gitweb = File::Spec->catfile('..','..','gitweb','gitweb.cgi');
> +# the followin two lines of code are workaround for bug in
> +# Test::WWW::Mechanize::CGI::cgi_application version up to 0.3
> +# (http://rt.cpan.org/Ticket/Display.html?id=36654)
> +# for pathnames with spaces (because of "trash directory")
> +$gitweb = File::Spec->rel2abs($gitweb);
> +$gitweb = Cwd::abs_path($gitweb);
> +
> +my $mech = new Test::WWW::Mechanize::CGI;
> +$mech->cgi_application($gitweb);

Another solution would be to copy relevant parts of cgi_application
(without all the checks for example), and use $mech->cgi( sub { ... } );
here (without the cgi_application bug).

> +our $baseurl = "http://localhost";
> +our($params, $url, $pagedesc, $status);

I think we can use 'my' here; gitweb uses 'our' only to be able to run
it correctly as "legacy CGI" mod_perl script.

> +our($params, $url, $pagedesc, $status);

Style: I would write "our (", with space between keyword and opening
parenthesis.

> +# test_page ( <params>, <page_description>, <expected_status> )
> +# Example:
> +# if (test_page '?p=.git;a=summary', 'repository summary') {
> +#     $mech->...;
> +#     $mech->...;
> +# }

Style: I would use function call form, i.e. "test_page(...)", not
command-like (or script-like) form.


As to the rest of the test: I think as preliminary test it is a good
thing to have.  We can think about what tests to add later (unless you
rather have exhaustive test suite now...).

-- 
Jakub Narebski
Poland