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

I just think that having this separate could help bisectability in
the case of errors.

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

Well, I've tried to put the cases where something can go wrong, and to
cover wide range of possibilities.  Rename, copy, typechange, merge
commit for testing *diff views, symlink to test 'tree' view, different
types of tags and tagged objects...

What could be added is different types of stage output: filenames with
'*', '+', '=', ':', '?', whitespace, etc.  Checking if submodules
doesn't trip gitweb would be good idea too.
 
> 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.

While they can detect otherwise unnoticed errors, I think most of time
they are error in test counting... so I can agree with that.
 
>> 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.

This would reduce number of operations when crawling gitweb output.

 
[about workaround bug in TWM::CGI when path to application contains
 embedded space].

> ISTR that using cgi(sub ...) gives us problems with untrappable exits in 
> gitweb.cgi (and possibly more things), right?

Actually ->cgi_application(<path>) is implemented using ->cgi(<sub>)
in TWM::CGI.  The bug is that it uses straight "system($application)",
without any quoting, after ensuring that $application is absolute path
(so quoting it beforehand won't work).

> I'm fine with the workarounds we have in place, they don't seem brittle.

They work around the fact that we use 'trash directory', but they
would fail if you run test from the directory which contains spaces,
for example "/home/Lea Wiemann/git/t" (I think this has greater
probability happening on operating systems other than Linux, for
example MS Windows with "My Documents" or "Program Files").

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

Actually 'my' would work here too; the problem with gitweb being
forced to use 'our' to make it work with mod_perl isn't about the
fact that they are global variables, but with initializing them,
as mod_perl wraps whole gitweb in a block (processing requests).
 
>> 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.

Write test when we notice something breaking seems a common theme
in git development... ;-)

-- 
Jakub Narebski
Poland