On Fri, 20 Jun 2008, Lea Wiemann wrote: > From: Jakub Narebski <jnareb@xxxxxxxxx> If you have rewritten the patch I won't mind if you change authorship to you (remove this From: line); signoff is IMHO enough. > --- > I haven't gotten around to merging Jakub's recent XML validation patch > yet, but I think it'd be good to have some review now; I'll merge it > tomorrow and send another patch. First, it is not XML validation[*1*], but check if XML is well-formed. Second, I think it would be better if adding XML checks (RSS, Atom, OPML) would be left as separate commit. [*1*] There is W3C feed validation service, http://validator.w3.org/feed/ which provides also standalone Python script for feed validation, and also SOAP interface with WebService::Validator::Feed::W3C wrapper (but it would require having Internet connection), or we can try validate against XML Schema Definition (there exists one for RSS, and there is one for Atom derived from Relax-NG schema). > diff --git a/t/t9503-gitweb-Mechanize.sh b/t/t9503-gitweb-Mechanize.sh [...] > +# set up test repository > +test_expect_success 'set up test repository' ' [cut] I have created this part as a copy of older t9500 gitweb test, thinking about what we might want to test, but the WIP of Mechanize based t9503 doesn't have yet tests for those specific features. > diff --git a/t/t9503/test.pl b/t/t9503/test.pl [...] > +# We don't count properly when skipping, so no_plan is necessary. > +use Test::More qw(no_plan); Actually it is not that we cannot could properly when skipping, because there are two ways to have skipped tests and test count upfront, both implemented in my initial version (v1) of t/t9503/test.pl. You can set number of tests upfront depending if some feature is available, e.g. "plan tests => <n> - $use_lint*<m2> - $check_xml*<m2>" or just "if ($use_lint) { plan tests => <m> } else ...". Or you can explicitly skip tests using SKIP: BLOCK (see Test::More(3pm)). What we have here is that we don't know number of tests upfront, because it is complicated, and that's what I'd like to see in the comment instead (but I don't have idea for exact wording). > +my @revisions = map { substr $_, 0, 10 } split /\s/, `git-rev-list --first-parent HEAD`; Why do you use shortened SHA1 identifier, instead of full SHA1? Links in gitweb use full SHA1. > +my $head = $revisions[-1]; Errr, HEAD would be $revisions[0], $revisions[-1] would be $root. > +my $gitweb = File::Spec->catfile('..','..','gitweb','gitweb.cgi'); > +# the followin two lines of code are workaround for bug in > +# Test::WWW::Mechanize::CGI::cgi_application version up to 0.3 > +# (http://rt.cpan.org/Ticket/Display.html?id=36654) > +# for pathnames with spaces (because of "trash directory") > +$gitweb = File::Spec->rel2abs($gitweb); > +$gitweb = Cwd::abs_path($gitweb); > + > +my $mech = new Test::WWW::Mechanize::CGI; > +$mech->cgi_application($gitweb); Another solution would be to copy relevant parts of cgi_application (without all the checks for example), and use $mech->cgi( sub { ... } ); here (without the cgi_application bug). > +our $baseurl = "http://localhost";; > +our($params, $url, $pagedesc, $status); I think we can use 'my' here; gitweb uses 'our' only to be able to run it correctly as "legacy CGI" mod_perl script. > +our($params, $url, $pagedesc, $status); Style: I would write "our (", with space between keyword and opening parenthesis. > +# test_page ( <params>, <page_description>, <expected_status> ) > +# Example: > +# if (test_page '?p=.git;a=summary', 'repository summary') { > +# $mech->...; > +# $mech->...; > +# } Style: I would use function call form, i.e. "test_page(...)", not command-like (or script-like) form. As to the rest of the test: I think as preliminary test it is a good thing to have. We can think about what tests to add later (unless you rather have exhaustive test suite now...). -- Jakub Narebski Poland -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
