647323 - Import and wrap HTMLWG's testharness.js

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Henri Sivonen (:hsivonen)]

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.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And again without my broken patches below it...

https://tbpl.mozilla.org/?tree=Try&rev=256c4a2f434e

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Document the setup

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b (1): Implement test harness reporting code

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b (2): Import testharness code from the HTML WG repository

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Implement test importing code

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part d: Import tests Mozilla contributed to the HTML WG test suite

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part e: Import Opera's getElementsByClassName tests from the WebApps WG repository

Comment 17

[Jeff Hammel]

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 18

[Jeff Hammel]

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 19

[Jeff Hammel]

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 20

[Jeff Hammel]

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 21

[Jeff Hammel]

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 22

[Jeff Hammel]

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 23

[Jeff Hammel]

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

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Document the setup (v2)

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.)

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b (3): Generate testharnessreport.js during the build

Comment 27

[James Graham [:jgraham]]

(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

Comment 28

[Gervase Markham [:gerv]]

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

Comment 29

[fantasai]

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.

Comment 30

[Gervase Markham [:gerv]]

No!

http://hg.mozilla.org/mozilla-central/file/a30fd69f1e0c/other-licenses/README

What made you think that directory was the correct one?

Gerv

Comment 31

[fantasai]

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

Comment 32

[Gervase Markham [:gerv]]

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

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 34

[cmtalbert]

(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 35

[(not currently active) Ted Mielczarek]

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.

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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