Re: [PATCH 1/3 v10] gitweb: add test suite with Test::WWW::Mechanize::CGI

[Lea Wiemann <lewiemann@gmail.com>]

Junio C Hamano wrote:
> Lea Wiemann <lewiemann@gmail.com> writes:
> 
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>> Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
> 
> This s-o-b chain is a bit confusing; was this authored by you or Jakub?

Jakub started it, I extended it.  Should we have different SOB lines?

>> +safe_chmod () {
>> +	chmod "$1" "$2" &&
>> +	if [ "$(git config --get core.filemode)" = false ]
>> +	then
>> +		git update-index --chmod="$1" "$2"
>> +	fi
>> +}
> 
> You have this in t9500 as well.  Perhaps it can go to test-lib?

Will do in the next version of this patch.

>> +# check if test can be run
>> +"$PERL_PATH" -MEncode -e 'decode_utf8("", Encode::FB_CROAK)' >/dev/null 2>&1 || {
>> +	test_expect_success \
>> +		'skipping gitweb tests, perl version is too old' :
> 
> It may be helpful to say what exactly is lacking

Right.  Since Encode doesn't run on older Perl versions anyway, I'm
changing it to

"$PERL_PATH" -e 'use 5.008' >/dev/null 2>&1 || {
	test_expect_success \
		'skipping gitweb tests, Perl 5.8 or newer required' :

> t3300, t4000, t5540, t9113,
> t9113, t9600, and t9700 use "say" (or say_color), t3902, t4016, t5000, and
> t7004 just use "echo", and t9200, t9400, t9401 and t9500 do this phoney
> "success".  We should standardize these by introducing "test_stop_early
> $msg".

Yup; maybe "test_skip_all" is clearer though.  I think this should be
done in a separate patch.

>> +	test_tick && git pull . b
> 
> That "pull . b" is somewhat old fashioned, but is Ok.

Is "git merge b" equivalent?  (The test still passes with it.)

>> +large_cache_root="../t9503/large_cache.tmp"
> 
> Please use $TEST_DIRECTORY without relying on the location of "t/trash
> directory"; it was painful to fix all of them.

Ok, fixed all of those.  I'll also move the cache-setup code to patch 3
(gitweb caching), since it doesn't belong here as long as caching isn't
implemented.

>> +# Grep
> 
> With these search oriented tests, making sure that you would find what you
> expect to find is obviously important, but shouldn't you be also making
> sure that irrelevant entries are not found?

Technically yes, but I'm not inclined at the moment to write that test
(at least while I'm not hacking the search part of gitweb).  The test is
basically only there to exercise the code and make sure it returns
*something* sensible, which is where most breakages would occur.

Thanks for all your feedback!  I'll wait with sending a new patch series
until I've collected all feedback.

-- Lea