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

[Lea Wiemann <lewiemann@gmail.com>]

Jakub Narebski wrote:
> 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.

Sure; FWIW I'm generally in favor of having a large initial commit for 
new independent files (and since we don't need to worry about conflicts 
I'm not fervent about getting this into the next branch ASAP), but we 
can definitely add XML checking separately.

>> +# set up test repository
> 
> 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.

I was thinking about that.  Right now the tests are so generic that you 
can replace the test repository with anything else as long as it has 
some commits (and later some tags, etc.).  That's kinda nice.

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

We're skipping tests if a page-load fails to prevent a slew of failure, 
using constructs like if(test_page '?...') { ... inspect the page ...}. 
It's my impression that we shouldn't end up with a wrong test count even 
if one test fails; but then we'd have to replace those ifs with 
cumbersome skip blocks.

I'm generally not in favor of maintaining any test count plans; they're 
an unnecessary failure source, and I don't think they buy us much, if 
anything -- correct me if I'm missing something Perl-specific here.

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

That's for readability in the test output; links get checked anyway, 
don't they?  If you think we should be testing against full SHA1s, 
that's fine too.

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

Fixed, thanks.

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

ISTR that using cgi(sub ...) gives us problems with untrappable exits in 
gitweb.cgi (and possibly more things), right?  I'm fine with the 
workarounds we have in place, they don't seem brittle.

>> +our $baseurl = "http://localhost";
>> +our($params, $url, $pagedesc, $status);
> 
> I think we can use 'my' here;

They're used in subroutines, so I believe 'our' is correct here.

> Style: I would write "our (", with space [...]

>> +# if (test_page '?p=.git;a=summary', 'repository summary') {
> 
> Style: I would use function call form, i.e. "test_page(...)", not
> command-like (or script-like) form.

Both fixed, thanks.

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

I'll be writing tests as I go and change parts of gitweb.  I won't be 
able to write exhaustive tests, but I at least want to make sure that 
the code I'm changing is covered somehow.

-- Lea