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

Lea Wiemann [off-list ref] 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 <redacted>
Signed-off-by: Lea Wiemann <redacted>

This s-o-b chain is a bit confusing; was this authored by you or Jakub?

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.

Junio C Hamano wrote:

Lea Wiemann [off-list ref] writes:

quoted

Signed-off-by: Jakub Narebski <redacted>
Signed-off-by: Lea Wiemann <redacted>

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?

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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