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

From: Lea Wiemann <lewiemann@gmail.com>
Date: Jun 20, 2008, 13:49 UTC
Message-ID: <485BB578.3040605@gmail.com>
In-Reply-To: <200806201408.05254.jnareb@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.

Show 5 quoted lines

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

Show 5 quoted lines

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