Re: [RFC/PATCH (WIP)] gitweb: Use Test::WWW::Mechanize::CGI to test gitweb output

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 14, 2008, 18:59 UTC
Message-ID: <200806142059.52373.jnareb@gmail.com>
In-Reply-To: <48540E70.4030507@gmail.com>

Lea Wiemann wrote:

Show 10 quoted lines

> Jakub Narebski wrote:
>> Lea Wiemann wrote:
>>
>>> I think that gitweb should not be writing stuff to stderr unless an
>>> internal or serious error occurs
>>
>> There is no option to git-rev-list to not write any output to stderr
> 
> Okay, this one will go away with the new API I'm writing, which uses 
> cat-file --batch-check instead of rev-list.

Won't work. Well, it would work for p=commit;h=non-existent, but it would not work for p=log;h=non-existent, where git-rev-list (or git-log) is really needed. And I'd rather have parse_commit() (used in 'commit' view) and parse_commits() (used in log-like views) share the same commit parser, parse_commit_text(), so it's rev-list also for git_commit, not cat-file.

But using git-cat-file --batch-check would improve other cases.

Show 5 quoted lines

>                                             In the meantime (and in  
> other cases) I guess diverting stderr in the test code is fine.  (I 
> wouldn't want to ignore stderr in all cases, even where you're not 
> expecting any output on stderr, since that might actually indicate an 
> error.)

Well, I'd divert stderr in tests cases (or use simply 'test_external'), or better filter stderr, only in those cases where there is spurious but not dangerous thing on stderr.

But again, I think the solution would be either to add feature to Git.pm, something like command_output_pipe_no_stderr, which would redirect stderr to /dev/null, or modify git wrapper to redirect stderr to /dev/null in scripts / when calling script, and redefine die and warn for built-ins (or close stderr).

Show 5 quoted lines

>> [snip]  It should work.  test-lib.sh sets up $PATH to have 'git' binary
>> (just compiled git binary) in it...
> 
> Since you're accessing http://localhost/ URLs, the web server's PATH is 
> in effect,...

It isn't. http://localhost/ is just access convention, and WWW::Mechanize::CGI actually calls system()[*1*] on provided path made absolute (hence error when it contains whitespace), setting CGI environmental variables before calling it.

[*1*] it would be nice to have perl_application in WWW::Mechanize::CGI, which would simply setup %ENV and use do() instead of system() on provided application. Perhaps it would be better in meantime to simply craft $mech->cgi( sub { ... } ), and do not use generic $mech->cgi_application($path); we could do without all the checking, too.

-- 
Jakub Narebski
Poland