Split out the template tests from bug 368097 and use the automated testing framework. Some of the tests fail currently due to known bugs, or ones that still need to be investigated, so these tests are a work in progress.

also don't include the tests for non-xul and trees yet, as they don't work fully yet due to a couple of minor bugs.

Comment on attachment 272967 [details] [diff] [review] add todos for a few tests that aren't working Sorry, I don't really understand this, although some of your XXX look as if they should be TODO ;-)

What's keeping these tests, or a newer set, from being committed?

(In reply to comment #3) > What's keeping these tests, or a newer set, from being committed? I plan on simplifying this test framework.

Can you do the simplification in followup work after an initial commit? Every day these aren't in, even in a preliminary state where they test but aren't quite as pretty as they could be, is another day for some functionality used in the tests to regress. Also, if the tests aren't in, nobody except you knows what doesn't work in the tests.

The simplification is needed so someone has a chance of reviewing it without going insane trying to figure out what its doing.

Neil or Gavin, are either of you ok with reviewing these tests?

That's a lot of tests :-( Would it make sense to do tests that N templates using different data sources produce the same output? Would it make sense to test trees by comparing the views with and without the dont-build-content flag? Would it make sense to compare serialisations first for speed?

(In reply to comment #10) > That's a lot of tests :-( > > Would it make sense to do tests that N templates using different data sources > produce the same output? Could be useful for some tests. > > Would it make sense to test trees by comparing the views with and without the > dont-build-content flag? Many of the tree tests could be done like that. Due to some bugs (which are I filed somewhere) in tree templates, a noticable number of the tree tests are marked as 'todo', so it's hard to get useful tests for trees currently. I think it would make sense to improve the tests once the bugs get fixed. > Would it make sense to compare serialisations first for speed? You mean compare a serialized dom instead of iterating over the tree? That would likely be faster, although there are certain characteristics such as ids which aren't always the same, elements in indeterminate order, and so forth. Also, comparing iteratively allows better error messages.

I think it is important to include this tests in the trunk. The patch failed on the Makefile.in. I'm will provide a new patch, and I will add some tests for template with sqlite (perhaps there are some regressions with sqlite templates, I investigate on bug 467775) One question : why store this tests in the toolkit directory ? Why not in content/xul/templates/tests ? I think it could be more consistent.

I don't think it matters really, feel free to move the tests if you desire.

Here is a new version of the patch. I moved all tests into content/xul/templates/tests, and I added some tests on templates with sqlite. These new tests pass only if the Neil's patch for bug 467775 is applied :-)

Note to change the 'debug' variable in template_shared.js to false before checking in.

Comment on attachment 351559 [details] [diff] [review] Include new tests on template+storage (tests don't need explicit approval, fwiw!)

Comment on attachment 351559 [details] [diff] [review] Include new tests on template+storage Ah, I missed this comment "These new tests pass only if the Neil's patch for bug 467775 is applied :-)"

+function test_storage_template() +{ + var root = document.getElementById("root"); + expectedOutput = <output></output>; + checkResults(root, 0); + + document.getElementById("species-id").textContent = '2'; + root.builder.rebuild(); + setTimeout(test_storage_template2, 500); // we're waiting after the rebuild of the template +} Why 500ms timeout? Would 0 be enough? Or even better if there is some callback which is called when rebuild is done. nsIXULBuilderListener?

I added this timeout to wait after the rebuild of the template, since it is rebuild asynchronously and we haven't events to know when the rebuild is done (I think). Unless there is a new way to know when the build is done ?

Oh sorry, I didn't see this nsIXULBuilderListener interface. I take a look on it.

Just so you know, using timeouts to work around asynchrony is bad as tests run on heavily overloaded VMs. The timeout you might think is more than sufficient may not be, then, turning the tree orange. As a practical matter, it's best never to use timeouts in Mochitest. :-\

I changed the test by using a listener on the builder and it's ok. However, test_tmpl_menuelementrecursive.xul fails on my machine (I haven't the latest trunk on my machine because of bug 468845). I will investigate later...

(In reply to comment #22) > However, test_tmpl_menuelementrecursive.xul fails on my machine (I haven't the > latest trunk on my machine because of bug 468845). I will investigate later... Works for me here. Do you have time to investigate? I'd like to get these tests checked in.

I retry with the latest trunk. Now, most of template tests fail because of a very strang javascript error :-/ JavaScript error: chrome://mochikit/content/chrome/content/xul/templates/tests/chrome/templates_shared.js, line 223: var attr is not a function At this line: // loop through the attributes in the expected node and compare their // values with the corresponding attribute on the actual node var expectedAttrs = expected.attributes(); --> for each (var attr in expectedAttrs) { var expectval = "" + attr; // skip checking the id when anyid="true", however make sure to // ensure that the id is actually present. if (attr.name() == "anyid" && expectval == "true") { I really don't know why. Is there a regression in E4X or in the javascript engine ?

I filed bug 482266 on the javascript bug.

I changed the script a bit to workaround the javascript issues. There's a lot of template code being tested by these new tests. Let's just get these checked in.

Comment on attachment 373839 [details] [diff] [review] rework some code a bit for the js issues >diff --git a/content/xul/templates/Makefile.in b/content/xul/templates/Makefile.in >--- a/content/xul/templates/Makefile.in >+++ b/content/xul/templates/Makefile.in >@@ -49,5 +49,9 @@ > TOOL_DIRS += tests > endif > >+ifdef ENABLE_TESTS >+TOOL_DIRS += tests >+endif >+ Why is this change needed? Makefile.in seems to have the same thing already. >diff --git a/toolkit/toolkit-makefiles.sh b/toolkit/toolkit-makefiles.sh >--- a/toolkit/toolkit-makefiles.sh >+++ b/toolkit/toolkit-makefiles.sh >@@ -222,6 +222,8 @@ > content/xul/document/src/Makefile > content/xul/templates/public/Makefile > content/xul/templates/src/Makefile >+ content/xul/templates/tests/Makefile >+ content/xul/templates/tests/chrome/Makefile > content/xbl/Makefile > content/xbl/public/Makefile > content/xbl/src/Makefile I have no idea why this is needed, but I know pretty much nothing about toolkit/ I really want to see the tests in trunk, not only in bugzilla. rubberstamp=me. I did run the tests on 64 bit linux.

The tests might be failing randomly: 140 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/content/xul/templates/tests/chrome/test_tmpl_bindingsextendedsyntax.xul | bindings - extended syntax 141 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/content/xul/templates/tests/chrome/test_tmpl_bindingsextendedsyntax.xul | bindings - extended syntax dynamic step 1 142 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/content/xul/templates/tests/chrome/test_tmpl_bindingsextendedsyntax.xul | bindings - extended syntax dynamic step 2 1013 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/content/xul/templates/tests/chrome/test_tmpl_xmlquerysimple.xul | xml query simple http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1241504362.1241509277.9313.gz OS X 10.5.2 mozilla-central unit test on 2009/05/04 23:19:22 The previous couple of cycles were green on that builder, and the changes in the cycle that failed don't look obviously related.

Had another green cycle or two, then another orange: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1241511559.1241516430.18767.gz OS X 10.5.2 mozilla-central unit test on 2009/05/05 01:19:19 I ran the tests locally on my OS X machine and they ran fine, too.

This was checked in but seems to fail on Linux and Mac sometimes, Windows seems ok. Likely an asynchronous loading issue. I am investigating, but I'll need to disable these tests again if I can't figure it out soon.

(In reply to comment #30) > This was checked in but seems to fail on Linux and Mac sometimes, Windows seems > ok. Likely an asynchronous loading issue. I am investigating, but I'll need to > disable these tests again if I can't figure it out soon. It is failing on windows sometimes as well

OK, I disabled these tests on all platforms for now.

Comment on attachment 351559 [details] [diff] [review] Include new tests on template+storage (clearing flag for query-sanity on the driving side; this didn't need a+ anyway as it's just tests)

Re-enable the tests and add some code to load the datasources.

Comment on attachment 382310 [details] [diff] [review] Load rdf and xml datasources before starting the test Could you test this on tryserver first.

This worked fine on the tryserver, so I checked it in and have seen no problems so far.