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

[Junio C Hamano <gitster@pobox.com>]

Lea Wiemann <lewiemann@gmail.com> writes:

> This test uses Test::WWW::Mechanize::CGI to check gitweb's output.  It
> also uses HTML::Lint, XML::Parser, and Archive::Tar (if present, each)
> to validate the HTML/XML/tgz output, and checks all links on the
> tested pages if --long-tests is given.
>
> 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?

> diff --git a/Makefile b/Makefile
> index ca418fc..35779a7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1289,6 +1289,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
>  GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
>  	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
>  	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
> +	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@
>  
>  ### Detect Tck/Tk interpreter path changes
>  ifndef NO_TCLTK
> diff --git a/t/t9503-gitweb-Mechanize.sh b/t/t9503-gitweb-Mechanize.sh
> new file mode 100755
> index 0000000..53f2a8a
> --- /dev/null
> +++ b/t/t9503-gitweb-Mechanize.sh
> @@ -0,0 +1,144 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2008 Jakub Narebski
> +# Copyright (c) 2008 Lea Wiemann
> +#
> +
> +# This test supports the --long-tests option.
> +
> +# This test only runs on Perl 5.8 and later versions, since
> +# Test::WWW::Mechanize::CGI requires Perl 5.8.
> +
> +test_description='gitweb tests (using WWW::Mechanize)
> +
> +This test uses Test::WWW::Mechanize::CGI to test gitweb.'
> +
> +# helper functions
> +
> +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?

> +. ./test-lib.sh
> +
> +# 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' :
> +	test_done
> +	exit
> +}

It may be helpful to say what exactly is lacking (either "Please upgrade
your Perl to 5.8", or "We want decode_utf8() that can CROAK").

> +"$PERL_PATH" -MTest::WWW::Mechanize::CGI -e '' >/dev/null 2>&1 || {
> +	test_expect_success \
> +		'skipping gitweb tests, Test::WWW::Mechanize::CGI not found' :
> +	test_done
> +	exit
> +}

This one is better then the previous one.  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".  Then we can lose test_done and exit from these places.

> +# set up test repository
> +test_expect_success 'set up test repository' '
> ...
> +	test_tick && git pull . b
> +'

That "pull . b" is somewhat old fashioned, but is Ok.

> +# set up gitweb configuration
> +safe_pwd="$("$PERL_PATH" -MPOSIX=getcwd -e 'print quotemeta(getcwd)')"
> +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.

> +test_expect_success 'create file cache directory' \
> +	'mkdir -p "$large_cache_root"'
> +cat >gitweb_config.perl <<EOF
> +# gitweb configuration for tests
> ...
> +our @stylesheets = ("file:///$safe_pwd/../../gitweb/gitweb.css");
> +our \$logo = "file:///$safe_pwd/../../gitweb/git-logo.png";
> +our \$favicon = "file:///$safe_pwd/../../gitweb/git-favicon.png";

These also assume "t/trash directory" not being "t/trash/t9503".

> +test_external \
> +	'test gitweb output' \
> +	"$PERL_PATH" ../t9503/test.pl

So does this, and you have catfile('..', '..', ...) in the perl part of
this test.

> +# Search form
> +
> +# Search commit
> +if (get_summary && $mech->submit_form_ok(
> +	    { form_number => 1, fields => { 's' => 'Initial' } },
> +	    'submit search form (default: commit search)')) {
> +	check_page;
> +	$mech->content_contains('Initial commit',
> +				'content contains commit we searched for');
> +}
> +
> +# Pickaxe
> +if (get_summary && $mech->submit_form_ok(
> +	    { form_number => 1, fields => { 's' => 'pickaxe test string',
> +					    'st' => 'pickaxe' } },
> +	    'submit search form (pickaxe)')) {
> +	check_page;
> +	test_link( { text => 'dir1/file1' }, 'file found with pickaxe' );
> +	$mech->content_contains('A U Thor', 'commit author mentioned');
> +}
> +
> +# Grep
> +# Let's hope the pickaxe test string is still present in HEAD.
> +if (get_summary && $mech->submit_form_ok(
> +	    { form_number => 1, fields => { 's' => 'pickaxe test string',
> +					    'st' => 'grep' } },
> +	    'submit search form (grep)')) {
> +	check_page;
> +	test_link( { text => 'dir1/file1' }, 'file found with 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?

It is great that there are tests for each view we care about, even though
the way the individual views are tested look somewhat sketchy.