Jakub Narebski wrote:
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.
Sure; FWIW I'm generally in favor of having a large initial commit for
new independent files (and since we don't need to worry about conflicts
I'm not fervent about getting this into the next branch ASAP), but we
can definitely add XML checking separately.
+# set up test repository
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.
I was thinking about that. Right now the tests are so generic that you
can replace the test repository with anything else as long as it has
some commits (and later some tags, etc.). That's kinda nice.
+# 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,
We're skipping tests if a page-load fails to prevent a slew of failure,
using constructs like if(test_page '?...') { ... inspect the page ...}.
It's my impression that we shouldn't end up with a wrong test count even
if one test fails; but then we'd have to replace those ifs with
cumbersome skip blocks.
I'm generally not in favor of maintaining any test count plans; they're
an unnecessary failure source, and I don't think they buy us much, if
anything -- correct me if I'm missing something Perl-specific here.
Why do you use shortened SHA1 identifier, instead of full SHA1?
Links in gitweb use full SHA1.
That's for readability in the test output; links get checked anyway,
don't they? If you think we should be testing against full SHA1s,
that's fine too.
Errr, HEAD would be $revisions[0], $revisions[-1] would be $root.
Fixed, thanks.
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).
ISTR that using cgi(sub ...) gives us problems with untrappable exits in
gitweb.cgi (and possibly more things), right? I'm fine with the
workarounds we have in place, they don't seem brittle.
+our $baseurl = "http://localhost";;
+our($params, $url, $pagedesc, $status);
I think we can use 'my' here;
They're used in subroutines, so I believe 'our' is correct here.
Style: I would write "our (", with space [...]
+# if (test_page '?p=.git;a=summary', 'repository summary') {
Style: I would use function call form, i.e. "test_page(...)", not
command-like (or script-like) form.
Both fixed, thanks.
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...).
I'll be writing tests as I go and change parts of gitweb. I won't be
able to write exhaustive tests, but I at least want to make sure that
the code I'm changing is covered somehow.
-- Lea
--
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
