From: Jakub Narebski <jnareb@gmail.com>
To: Lea Wiemann <lewiemann@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v6] gitweb: add test suite with Test::WWW::Mechanize::CGI
Date: Mon, 23 Jun 2008 15:31:12 +0200 [thread overview]
Message-ID: <200806231531.13082.jnareb@gmail.com> (raw)
In-Reply-To: <1214183688-8544-1-git-send-email-LeWiemann@gmail.com>
Lea Wiemann wrote:
> +# We don't count properly when skipping, so no_plan is necessary.
> +use Test::More qw(no_plan);
I'd rather either skip this comment all together, or change it to
read something like that (I don't have good solution):
+# Counting tests upfront is difficult, as number of tests depends
+# on existence of several Perl modules, and is next to impossible
+# when spidering gitweb output (for --long-test). Additionally,
+# having number of tests planned stated at beginning is not necessary,
+# as this test is to be run from git test suite, and not to be
+# processed further by TAP (Test Anything Protocol) Perl modules.
See http://www.perlfoundation.org/perl5/index.cgi?testing for
explanation why (and when) 'plan' is useful:
TAP output usually starts with a plan line:
1..9
which specifies how many tests are to follow. The above example
specifies 9 tests.
This line is optional, but recommended. Should a test suite die
part-way through, the plan allows the testing framework to recognise
this situation, rather than assuming that all tests were completed.
> +our $long_tests = $ENV{GIT_TEST_LONG};
Here also it could be "my $long_tests"; but I am not a Perl hacker,
and do not know which is preferred.
> +eval { require Archive::Tar; };
> +my $archive_tar_installed = !$@
> + or diag('Archive::Tar is not installed; no tests for valid snapshots');
> +chomp(my @heads = map { (split('/', $_))[2] } `git-for-each-ref --sort=-committerdate refs/heads`);
> +chomp(my @tags = map { (split('/', $_))[2] } `git-for-each-ref --sort=-committerdate refs/tags`);
Wouldn't be easier to use '--format=%(refname)' in git-for-each-ref
invocation? Besides, gitweb now uses in link _full_ refname, i.e.
"refs/heads/<name>" and "refs/tags/<name>" to avoid branch / tag
ambiguity (when there are both branch and head with the same name).
> +# files and directories in HEAD root:
> +chomp(my @files = map { (split("\t", $_))[1] } `git-ls-tree HEAD`);
Wouldn't it be easier to use "git ls-tree --names-only HEAD"?
> +sub rev_parse (...)
> +sub get_type (...)
Nice.
> +my $gitweb = abs_path(File::Spec->catfile('..', '..', 'gitweb', 'gitweb.cgi'));
> +
> +# Thus subroutine was copied (and modified to work with blanks in the
> +# application path) from WWW::Mechanize::CGI 0.3, which is licensed
> +# 'under the same terms as perl itself' and thus GPL compatible.
Errr... I think that without my comment you removed it is hard to
understand what this comment talks about. "Thus ..." without any
preceding sentence?
> +my $cgi = sub {
> + # Use exec, not the shell, to support blanks in the path.
blanks = embedded whitespace?
path = path to gitweb?
> + my $status = system { $ENV{GITPERL} } $ENV{GITPERL}, $gitweb;
> + my $value = $status >> 8;
> +
> + croak( qq/Failed to execute application '$gitweb'. Reason: '$!'/ )
> + if ( $status == -1 );
> + croak( qq/Application '$gitweb' exited with value: $value/ )
> + if ( $value > 0 );
> +};
> +
> +my $mech = new Test::WWW::Mechanize::CGI;
> +$mech->cgi($cgi);
Looks good. Thanks.
> + # WebService::Validator::Feed::W3C would be nice to
> + # use, but it doesn't support direct input (as opposed
> + # to URIs) long enough for our feeds.
Could you tell us here what are the limitations? Are those limits
of W3C SOAP interface for web validation, or the CPAN package mentioned?
> + return 1
Style: "return 1;"
> +# RSS/Atom/OPML view. Simply retrieve and check.
> +{
> + # Broken link in Atom/RSS view -- cannot spider:
> + # http://mid.gmane.org/485EB333.5070108@gmail.com
> + local $long_tests = 0;
> + test_page('?p=.git;a=atom', 'Atom feed');
> + test_page('?p=.git;a=rss', 'RSS feed');
> +}
> +test_page('?a=opml', 'OPML outline');
This I think should be written using Test::More equivalent of
test_expect_failure in t/test-lib.sh, namely TODO block, either
as
TODO: {
local $TODO = "why";
...normal testing code...
}
or if it causes problem with t9503-gitweb-Mechanize.sh failing, perhaps
as
TODO: {
todo_skip "why", <n> if <condition>;
...normal testing code...
}
(And similarly for other pages).
> +# Blob view
I like those comments.
> diff --git a/t/test-lib.sh b/t/test-lib.sh
[...]
> +GITPERL=${GITPERL:-perl}
> +export GITPERL
How it is different from PERL_PATH?
--
Jakub Narebski
ShadeHawk on #git
Thorn, Poland
next prev parent reply other threads:[~2008-06-23 13:32 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-06-14 12:47 [RFC/PATCH (WIP)] gitweb: Use Test::WWW::Mechanize::CGI to test gitweb output Jakub Narebski
2008-06-14 14:40 ` Lea Wiemann
2008-06-14 18:07 ` Jakub Narebski
2008-06-14 18:31 ` Lea Wiemann
2008-06-14 18:59 ` Jakub Narebski
2008-06-14 21:12 ` Lea Wiemann
2008-06-15 8:36 ` Jakub Narebski
2008-06-14 18:18 ` Lea Wiemann
2008-06-14 18:31 ` Jakub Narebski
2008-06-14 23:57 ` [RFC/WIP/PATCH v2] gitweb: add test suite with Test::WWW::Mechanize::CGI Lea Wiemann
2008-06-15 18:01 ` Jakub Narebski
2008-06-15 18:45 ` Lea Wiemann
2008-06-16 0:40 ` Jakub Narebski
2008-06-16 9:10 ` Lea Wiemann
2008-06-16 20:15 ` Jakub Narebski
2008-06-20 3:18 ` [WIP/PATCH v3] " Lea Wiemann
2008-06-20 12:08 ` Jakub Narebski
2008-06-20 13:49 ` Lea Wiemann
2008-06-20 18:03 ` Jakub Narebski
2008-06-20 22:04 ` Lea Wiemann
2008-06-20 22:18 ` [WIP/PATCH v4] " Lea Wiemann
2008-06-23 0:45 ` [PATCH v5] " Lea Wiemann
2008-06-23 1:14 ` [PATCH v6] " Lea Wiemann
2008-06-23 2:30 ` Junio C Hamano
2008-06-23 7:00 ` Lea Wiemann
2008-06-23 13:31 ` Jakub Narebski [this message]
2008-06-23 17:57 ` Lea Wiemann
2008-06-23 22:18 ` Jakub Narebski
2008-06-24 2:01 ` Lea Wiemann
2008-06-24 2:18 ` [PATCH v7] " Lea Wiemann
2008-06-26 13:47 ` [PATCH] " Lea Wiemann
2008-06-26 13:48 ` [PATCH v8] " Lea Wiemann
2008-06-29 22:47 ` Jakub Narebski
2008-06-29 23:39 ` Lea Wiemann
2008-06-29 23:56 ` Jakub Narebski
2008-06-30 0:30 ` Lea Wiemann
2008-06-30 21:55 ` Jakub Narebski
[not found] ` <48681EC8.8000606@gmail.com>
2008-06-30 22:01 ` Jakub Narebski
2008-06-24 4:20 ` [PATCH v6] " Junio C Hamano
2008-06-24 8:37 ` Lea Wiemann
2008-06-24 9:23 ` Jakub Narebski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200806231531.13082.jnareb@gmail.com \
--to=jnareb@gmail.com \
--cc=git@vger.kernel.org \
--cc=lewiemann@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).