testharness.{js,css} are imported directly from <http://dvcs.w3.org/hg/html>; the tests are ones I wrote, and submitted to the XHR and HTML test suites.

Sorry for the delay getting to this -- I'm glad you're doing this. At some point I think we're likely going to want to try to: * use as much of the w3c test suite as possible * track changes in it That suggests that we'd probably want to import its directory structure, if possible, inside a parent directory that has metadata (e.g., the version of the test suite we're using) and the glue we use (is that testharnessreport.js?). That said, I think it makes sense to start with just a few files to make sure the import works. Before I send you off doing that, though, I'm curious what the others I've cc:ed think.

If possible, I'd like to keep W3C directory structure and have some kind of list of known failures that get reported as todos for Mochitest purposes. That's how the html5lib test suite integration works.

I somehow managed to not see the bugmail here; sorry about that. Agreed with all your points; my main goal here was to get tests written with testharness.js to run. I haven't yet worked on importing the actual test suites, but I believe smaug is importing some of Opera's tests that use this harness as well. The expected failures are currently listed in testharnessreport.js; I'm not entirely happy about that, but it seemed the safest way to get the list before the test started.

Comment on attachment 541952 [details] [diff] [review] Patch v1 Could you revise this per the feedback above and then request review again? (Also, please include a useful commit message in the patch. Right now it starts with "TODO:" and looks like a todo list.)

Two points to note, as discussed a little while ago in #whatwg: 1) For files that run lots and lots and lots of tests, outputting the test results to the default <div id=log> might take a long time, like 30+ seconds. This output is useless here, since the log isn't being used; the mochitest runner is keeping track of the passes/fails. Some feature should be added to testharness.js so that our testharnessreport.js can suppress the log (so that the results aren't added to the DOM at all). 2) testharness.js allows multiple asserts per test, like test(function() { assert_equals(foo(), foo, "foos aren't equal"); assert_equals(bar(), bar, "bars aren't equal"); }, "Test equality of foos and bars"); The current testharnessreport.js here tracks expected fails on a test level, without regard to which assert failed. So if the first assert previously passed but the second failed, and then there's a regression so the first assert also fails, this won't pick it up. Discussion in #whatwg concluded that such tests should be rewritten to have fewer asserts per test, so only closely related asserts are grouped together into one test. jgraham says that was his intent in writing the harness. So nothing needs to change here -- just be aware that if a test has many asserts, it's going to be useless for regression tracking unless all the asserts pass.

I pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=aa7690ab1308 One issue I need to fix is that we stringify objects differently in debug builds.

https://tbpl.mozilla.org/?tree=Try&rev=21a8090a79ff with a generic fails-in-debug-builds annotation I'll try to get those patches in a reviewable state this week.

And again without my broken patches below it... https://tbpl.mozilla.org/?tree=Try&rev=256c4a2f434e

Comment on attachment 607331 [details] [diff] [review] Part b (1): Implement test harness reporting code orientation \ sessionstorage \ storageevent \ + w3c \ indentation? +#! /usr/bin/python `/usr/bin/env python` is the prefered shebang to my knowledge Does this file require being run from its current directory to function? +wget "https://dvcs.w3.org/hg/html/raw-file/tip/tests/resources/testharness.js" -O testharness.js +wget "https://dvcs.w3.org/hg/html/raw-file/tip/tests/resources/testharness.css" -O testharness.css +wget "https://dvcs.w3.org/hg/html/raw-file/tip/tests/resources/idlharness.js" -O idlharness.js +wget "https://dvcs.w3.org/hg/html/raw-file/tip/tests/resources/WebIDLParser.js" -O WebIDLParser.js A for loop would be better +* testharnessreport.js + Generated from testharnessreport.js.in and the JSON files for repositories + listed in failures.txt by writeReporter.py. + MPL + +* testharnessreport.js.in Why check the .js file in? Why not just invoke writeReporter.py in the build system? r+ if you can address these concerns

Comment on attachment 607332 [details] [diff] [review] Part b (2): Import testharness code from the HTML WG repository + * (TODO: write this in Python or something so that it can be done from the + * command line instead.) Can you file a follow-up bug once this is landed? (Unless its already filed upstream, in which case Mozilla should have a tracking bug)

Comment on attachment 607332 [details] [diff] [review] Part b (2): Import testharness code from the HTML WG repository + * Sometimes tests require non-trivial setup that may fail. Why? + //Don't use document.title to work around an Opera bug in XHTML documents should have link to the bug I didn't really review the generated WebIDLParser.js code and have only done a cursory review of idlharness.js and testharness.js. Overall, it seems fine, and if its upstream code there's probably no point in picking nits here at least until its in the tree. Overall, wfm, assuming its been tested (which, given the bug context, I'm assuming it has).

Comment on attachment 607334 [details] [diff] [review] Part c: Implement test importing code diff --git a/dom/tests/mochitest/w3c/importTestsuite.py b/dom/tests/mochitest/w3c/importTestsuite.py Should probably have a docstring, a la """ + Imports a test suite from a remote repository. Takes one argument, a file in + the format described under webapps.txt. + Note: removes both source and destination directory before starting. Do not + use with outstanding changes in either directory. """ from https://bug647323.bugzilla.mozilla.org/attachment.cgi?id=607329 It would also be nice to fix the items under "Note" here as well +import sys, os PEP-8 prefers these on two separate lines try: finally: only works on 2.5 or later. I think all of the testing machines run 2.5 or later (except windows+talos) but we might want to confirm this. +def maybeCreateDir(path): + if not os.path.exists(path): + subprocess.check_call(["mkdir", path]) + +def cp(src, dest): + subprocess.check_call(["cp", src, dest]) Unless there is a reason not to, these should use the python stdlib functions, os.makedirs and shutil.copy. If there is a reason not to, it should be documented. + for part in d.split("/"): etc; this works on windows? I would prefer os.path.sep (etc) unless again there's a reason not to do this. +if __name__ == "__main__": + if len(sys.argv) < 2: + print "Need one argument." I'd prefer to check len(sys.argv) == 2 to ensure that it is invoked with exactly one argument. The file should also have a shebang, #!/usr/bin/env python diff --git a/dom/tests/mochitest/w3c/parseManifest.py b/dom/tests/mochitest/w3c/parseManifest.py I hate to introduce yet another manifest parser and format to the tree :( + chunks = line.split(" ") intentionally on a single space? or is line.split() better r+ with issues addressed

Comment on attachment 607334 [details] [diff] [review] Part c: Implement test importing code also do these files require being run from the directory they are in? If so, that should be fixed

Comment on attachment 607336 [details] [diff] [review] Part d: Import tests Mozilla contributed to the HTML WG test suite +include $(srcdir)/html.mk + kill the whitespace line +# THIS FILE IS AUTOGENERATED - DO NOT EDIT This should probably be a better message detailing how its generated and what generates it (see also https://bugzilla.mozilla.org/attachment.cgi?id=607334) Other than that, looks good

Comment on attachment 607337 [details] [diff] [review] Part e: Import Opera's getElementsByClassName tests from the WebApps WG repository Looks fine. I can't say I understand all of the testing nuances, but trusting that they do the right thing

(In reply to Jeff Hammel [:jhammel] from comment #17) > Comment on attachment 607331 [details] [diff] [review] > Part b (1): Implement test harness reporting code > > orientation \ > sessionstorage \ > storageevent \ > + w3c \ > indentation? Tabs :/. Fixed. > +#! /usr/bin/python > `/usr/bin/env python` is the prefered shebang to my knowledge OK. > Does this file require being run from its current directory to function? Yeah, but that's fine. > +wget > "https://dvcs.w3.org/hg/html/raw-file/tip/tests/resources/testharness.js" -O > testharness.js > +wget > "https://dvcs.w3.org/hg/html/raw-file/tip/tests/resources/testharness.css" > -O testharness.css > +wget > "https://dvcs.w3.org/hg/html/raw-file/tip/tests/resources/idlharness.js" -O > idlharness.js > +wget > "https://dvcs.w3.org/hg/html/raw-file/tip/tests/resources/WebIDLParser.js" > -O WebIDLParser.js > > A for loop would be better Removed the file. > +* testharnessreport.js > + Generated from testharnessreport.js.in and the JSON files for repositories > + listed in failures.txt by writeReporter.py. > + MPL > + > +* testharnessreport.js.in > > Why check the .js file in? Why not just invoke writeReporter.py in the > build system? Will do. > r+ if you can address these concerns Thanks. (In reply to Jeff Hammel [:jhammel] from comment #18) > Comment on attachment 607332 [details] [diff] [review] > Part b (2): Import testharness code from the HTML WG repository > > + * (TODO: write this in Python or something so that it can be done from the > + * command line instead.) > > Can you file a follow-up bug once this is landed? (Unless its already filed > upstream, in which case Mozilla should have a tracking bug) Aryeh, is there a bug somewhere? (In reply to Jeff Hammel [:jhammel] from comment #19) > Comment on attachment 607332 [details] [diff] [review] > Part b (2): Import testharness code from the HTML WG repository > > + * Sometimes tests require non-trivial setup that may fail. > > Why? > > + //Don't use document.title to work around an Opera bug in XHTML > documents > should have link to the bug > > I didn't really review the generated WebIDLParser.js code and have only done > a cursory review of idlharness.js and testharness.js. Overall, it seems > fine, and if its upstream code there's probably no point in picking nits > here at least until its in the tree. Overall, wfm, assuming its been tested > (which, given the bug context, I'm assuming it has). This is directly from upstream, so I'm not going to change it. (In reply to Jeff Hammel [:jhammel] from comment #20) > Comment on attachment 607334 [details] [diff] [review] > Part c: Implement test importing code > > diff --git a/dom/tests/mochitest/w3c/importTestsuite.py > b/dom/tests/mochitest/w3c/importTestsuite.py > > Should probably have a docstring, a la > > """ > + Imports a test suite from a remote repository. Takes one argument, a file > in > + the format described under webapps.txt. > + Note: removes both source and destination directory before starting. Do > not > + use with outstanding changes in either directory. > """ > > from https://bug647323.bugzilla.mozilla.org/attachment.cgi?id=607329 Done. > It would also be nice to fix the items under "Note" here as well Feature, not a bug :) > +import sys, os > > PEP-8 prefers these on two separate lines > > try: finally: only works on 2.5 or later. I think all of the testing > machines run 2.5 or later (except windows+talos) but we might want to > confirm this. I'm happy to require anybody who wants to import/update test suites (me and Aryeh, in other words) to have python 2.5. > +def maybeCreateDir(path): > + if not os.path.exists(path): > + subprocess.check_call(["mkdir", path]) > + > +def cp(src, dest): > + subprocess.check_call(["cp", src, dest]) > > Unless there is a reason not to, these should use the python stdlib > functions, os.makedirs and shutil.copy. If there is a reason not to, it > should be documented. That reason is my ignorance. > + for part in d.split("/"): > etc; this works on windows? I would prefer os.path.sep (etc) unless again > there's a reason not to do this. > > +if __name__ == "__main__": > + if len(sys.argv) < 2: > + print "Need one argument." > > I'd prefer to check len(sys.argv) == 2 to ensure that it is invoked with > exactly one argument. OK > The file should also have a shebang, #!/usr/bin/env python Done. > diff --git a/dom/tests/mochitest/w3c/parseManifest.py > b/dom/tests/mochitest/w3c/parseManifest.py > > I hate to introduce yet another manifest parser and format to the tree :( > > + chunks = line.split(" ") > intentionally on a single space? or is line.split() better Intentionally, yes. > r+ with issues addressed (In reply to Jeff Hammel [:jhammel] from comment #21) > Comment on attachment 607334 [details] [diff] [review] > Part c: Implement test importing code > > also do these files require being run from the directory they are in? If > so, that should be fixed Don't think that's worth fixing. (In reply to Jeff Hammel [:jhammel] from comment #22) > Comment on attachment 607336 [details] [diff] [review] > Part d: Import tests Mozilla contributed to the HTML WG test suite > > +include $(srcdir)/html.mk > + > > kill the whitespace line OK > +# THIS FILE IS AUTOGENERATED - DO NOT EDIT > > This should probably be a better message detailing how its generated and > what generates it (see also > https://bugzilla.mozilla.org/attachment.cgi?id=607334) Pointed to importTestsuite.py. > Other than that, looks good (In reply to Jeff Hammel [:jhammel] from comment #23) > Comment on attachment 607337 [details] [diff] [review] > Part e: Import Opera's getElementsByClassName tests from the WebApps WG > repository > > Looks fine. I can't say I understand all of the testing nuances, but > trusting that they do the right thing Thanks.

Gerv, I'm importing MIT and W3C Test Suite License / W3C 3-clause BSD License code here; could you confirm that's fine? (None of this is shipped, it's all just test code.)

(In reply to Jeff Hammel [:jhammel] from comment #19) > I didn't really review the generated WebIDLParser.js code and have only done > a cursory review of idlharness.js and testharness.js. Overall, it seems > fine, and if its upstream code there's probably no point in picking nits > here at least until its in the tree. FWIW if you do want to make review comments on these files, I am very open to making improvements; one good place to do such review might be in the github repo [1] [1] https://github.com/w3c/testharness.js

ms2ger: I don't see a problem with importing W3C stuff; I strongly suspect we've done this before. And if we aren't shipping it, we don't need to copy the license text anywhere. But fantasai or dbaron can probably correct me if I'm wrong. Gerv

I don't see a problem with this either, however they should go into the other-licenses/ directory, no? That's what we're planning to do with the CSS tests in bug 691950.

No! http://hg.mozilla.org/mozilla-central/file/a30fd69f1e0c/other-licenses/README What made you think that directory was the correct one? Gerv

I don't remember. Probably something to do with the license grant requested by W3C for test contributions.

Am I correct in believing that W3C tests are under an open source license (3-clause BSD is one of the options)? If so, then they can go anywhere in the tree. If the tests are not open source (!) we need to have a longer conversation. But what I was asking you really was: "from where did you get your ideas about appropriate use of the other-licenses directory?" If there's some document somewhere which says it should be used for anything non-MPLed, I want to fix it. Gerv

(In reply to Gervase Markham [:gerv] from comment #32) > Am I correct in believing that W3C tests are under an open source license > (3-clause BSD is one of the options)? Most are under the 3-clause BSD, some under MIT. > If so, then they can go anywhere in the tree. OK, thanks.

(In reply to Gervase Markham [:gerv] from comment #30) > No! > > http://hg.mozilla.org/mozilla-central/file/a30fd69f1e0c/other-licenses/README > > What made you think that directory was the correct one? Thanks for the clarification, Gerv. I think the confusion stemmed from the name of the directory being 'other-licenses' and this code having a license other than the MPL. I appreciate the clarification.

Comment on attachment 608822 [details] [diff] [review] Part b (3): Generate testharnessreport.js during the build Review of attachment 608822 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/w3c/Makefile.in @@ +21,5 @@ > idlharness.js \ > WebIDLParser.js \ > $(NULL) > > +testharnessreport.js: $(srcdir)/writeReporter.py $(srcdir)/testharnessreport.js.in You shouldn't need to specify $(srcdir) here, that's what VPATH is for. @@ +22,5 @@ > WebIDLParser.js \ > $(NULL) > > +testharnessreport.js: $(srcdir)/writeReporter.py $(srcdir)/testharnessreport.js.in > + $(PYTHON_PATH) $(srcdir)/writeReporter.py $(srcdir) This would feel nicer if it passed testharnessreport.js.in in as a parameter. If you made it the first dependency you could just use $<. ::: dom/tests/mochitest/w3c/writeReporter.py @@ +11,5 @@ > except ImportError: > import simplejson as json > > +def writeReporter(src): > + src = src.replace("/", os.sep) + os.sep This shouldn't be necessary. Windows Python handles paths like c:/foo/bar just fine.

Gerv, I'm assuming you don't object to landing this?

https://hg.mozilla.org/mozilla-central/rev/fd2549d5d384 https://hg.mozilla.org/mozilla-central/rev/bb0cc19274c5 https://hg.mozilla.org/mozilla-central/rev/cd07e609e481 https://hg.mozilla.org/mozilla-central/rev/c5201306217e https://hg.mozilla.org/mozilla-central/rev/f6955126d2c5