The attached patch supports running JS-only WPT tests in the jstests harness. It includes Python changes to fetch a list of enabled WPT tests from sub-directories specified in tests/wpt/wpt_config.py and a shell.js file containing shims required for running at least the streams tests (see [gecko root]/testing/web-platform/tests/streams/). Tests as retrieved from the WPT harness get filtered to remove not only all disabled tests, but also those that have any metadata at all. WPT metadata contains result expectations, but is completely absent for test files that are expected to pass in their entirety. For now I think it makes sense to be conservative here so as not to have our jstests start failing after a WPT update from upstream. In theory it wouldn't be too hard to add support for interpreting test expectations, but it does add further complexity. Apart from already adding complexity to the harness, this has the downside of adding about 1.5s (on my machine) to the jstests startup time. I don't see how I could do anything about that. Ms2ger offered to look into upstreaming parts of this into the WPT harness itself, which would be splendid. Setting a needinfo as a reminder.

Great! IIUC, this would allow running wpt tests in a directory that's not the usual web-platform tests directory. Is that true? If so, I'm very much tempted to mark this as blocking bug 1332691, where we are creating a shared test suite of WPT cases that we'd like to put under js/.

(In reply to Benjamin Bouvier [:bbouvier] from comment #1) > Great! IIUC, this would allow running wpt tests in a directory that's not > the usual web-platform tests directory. Is that true? It's not true with the patch as it is right now, but it can be made true quite easily. > If so, I'm very much > tempted to mark this as blocking bug 1332691, where we are creating a shared > test suite of WPT cases that we'd like to put under js/. Makes sense to me.

This is a slightly improved version of the previous patch that now fully works for the - since updated - streams tests. Asking for review because I think we could just land this and do uplifting of parts of it into WPT itself later. Steve, one issue that I think is unavoidable is that parsing the WPT tests manifest file takes about a second (well, on my machine). That's annoying and quite noticeable, but in the end probably acceptable.

Comment on attachment 8834037 [details] [diff] [review] WPT test support, v2 Review of attachment 8834037 [details] [diff] [review]: ----------------------------------------------------------------- I actually had misinterpreted what this patch would do. What we'd want for wasm is to run a subset of pure-JS *and HTML* WPT tests that are located in a different directory. After going through this patch and played with it locally, I could make it work for printing the list of pure JS tests that are located in a different WPT sub-tree. I've spent some time looking at the wpt source files too, and it seems running HTML WPT tests can be done by using only the primitives from wptrunner.py. That being said, this part remains of interest for pure-JS wasm test cases. Thanks for doing this! ::: js/src/tests/wpt/wpt.py @@ +10,5 @@ > +import sys > + > +sys.path.insert(0, abspath(normpath("../../../testing/web-platform/harness"))) > +sys.path.insert(0, abspath(normpath("../../../testing/web-platform/tests/tools"))) > +sys.path.insert(0, abspath(normpath("../../../testing/mozbase/mozlog/"))) These relative paths make it impossible to run in another directory than js/src/tests, could we change these to make them work all the time using relative paths from __file__, os.path.join'd? @@ +12,5 @@ > +sys.path.insert(0, abspath(normpath("../../../testing/web-platform/harness"))) > +sys.path.insert(0, abspath(normpath("../../../testing/web-platform/tests/tools"))) > +sys.path.insert(0, abspath(normpath("../../../testing/mozbase/mozlog/"))) > + > +from manifest import manifest nit: unused (unless it's implicit) @@ +27,5 @@ > + self.ssl_enabled = True > + > +def get_tests(base_path, include_paths): > + base_path = abspath(base_path) > + tests_base = joinpath(base_path, "tests") nit: unused @@ +69,5 @@ > + return True > + > +def get_js_path(test): > + (dirname, filename) = splitpath(test.abs_path) > + return joinpath(dirname, filename[0 : filename.find('.')]) + '.js' This is incorrect: for instance, there is a file called Math.max.html in the tests/js/builtins/ WPT directory, so this would yield Math.js for this file. There might be a more straight way to do this, but this works: '.'.join(filename.split('.')[0:-1]) (of course, this doesn't still work on the Math.max example, because the JS file corresponding is called Math.maxmin.js -_-') @@ +73,5 @@ > + return joinpath(dirname, filename[0 : filename.find('.')]) + '.js' > + > +def run_test(shell, test_path, test_info): > + print(test_path) > + # subprocess.call([shell, '-f', 'whatwg/shell.js', '-e', 'runWPTTest("' + test_path + '")']) nit: debug comment? to be enabled in the future? (it's the only use of subprocess, in case you'd remove it) @@ +91,5 @@ > + > + for test_path, test in tests: > + run_test(args.shell, test_path, test) > + > +if __name__ == '__main__': Since this can be executed as a top-level script, it might be nice to add a shebang at the top and chmod +x it.

Comment on attachment 8834037 [details] [diff] [review] WPT test support, v2 Review of attachment 8834037 [details] [diff] [review]: ----------------------------------------------------------------- I tried running with this patch, but couldn't get it to find any tests. I had to import the localpaths module to even find most of the modules -- maybe your virtualenv has them or something? I ended up doing -from os.path import exists, abspath, relpath, normpath, split as splitpath, join as joinpath +from os.path import dirname, exists, abspath, relpath, normpath, split as splitpath, join as joinpath import subprocess import sys -sys.path.insert(0, abspath(normpath("../../../testing/web-platform/harness"))) -sys.path.insert(0, abspath(normpath("../../../testing/web-platform/tests/tools"))) -sys.path.insert(0, abspath(normpath("../../../testing/mozbase/mozlog/"))) +jstests_dir = abspath(os.path.join(dirname(__file__), "..")) +testing_dir = abspath(os.path.join(jstests_dir, '..', '..', '..', 'testing')) + +sys.path.insert(0, os.path.join(testing_dir, "web-platform/harness")) +sys.path.insert(0, os.path.join(testing_dir, "web-platform/tests/tools")) +sys.path.insert(0, os.path.join(testing_dir, "web-platform/mozbase/mozlog")) + +import localpaths That was enough for it to load, but then I ended up with "no tests selected". I'm running it with ./jstest.py $JS wpt Is there some other way to ask for the wpt tests? When stepping through, it looks like the problem is that all of the tests appear to have metadata, so they pass the has_any_metadata check, which discards them. Am I doing something wrong? ::: js/src/tests/lib/manifest.py @@ +230,4 @@ > def make_manifests(location, test_gen): > _emit_manifest_at(location, '', test_gen, 0) > > +def _find_all_js_files(location, wpt_tests): Ugh, this seems pretty weird. It seems like none of this is really needed for count_tests, since you could just add in the number of wpt tests. For load_reftests, I'm not sure there's enough in common between wpt tests and regular tests to go to the effort of sharing the loop. You might as well split out a _make_testcase that does the creation, _apply_external_manifests, and _parse_test_header. (Except I'm not convinced the creation should be fully shared -- see below.) ::: js/src/tests/lib/tests.py @@ +140,5 @@ > self.jitflags = [] # [str]: JIT flags to pass to the shell > self.test_reflect_stringify = None # str or None: path to > # reflect-stringify.js file to test > # instead of actually running tests > + self.is_wpt_test = is_wpt_test This seems like it would be better as a class WPTRefTest(RefTest): def get_command(self, prefix): return prefix + self.jitflags + self.options + ['-f', ...] or if the repetition bothers you, factor out the jitflags+options piece. @@ +170,5 @@ > > class RefTestCase(RefTest): > """A test case consisting of a test and an expected result.""" > + def __init__(self, path, is_wpt_test): > + RefTest.__init__(self, path, is_wpt_test) ...even though it means making a dummy WPTRefTestCase subclass here as well. ::: js/src/tests/wpt/wpt.py @@ +27,5 @@ > + self.ssl_enabled = True > + > +def get_tests(base_path, include_paths): > + base_path = abspath(base_path) > + tests_base = joinpath(base_path, "tests") (This is an exported function, used from jstests.py, but perhaps it deserves a comment.)

This is the stuff required in the shell to make running WPT tests work, as discussed on IRC.

Comment on attachment 8845336 [details] [diff] [review] Part 1: Add shell builtin functions required for running WPT tests Review of attachment 8845336 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.h @@ +6075,2 @@ > * > + * If a the embedding has hidden the scripted caller for the frame's activation "a the" @@ +6078,5 @@ > */ > extern JS_PUBLIC_API(bool) > DescribeScriptedCaller(JSContext* cx, AutoFilename* filename = nullptr, > + unsigned* lineno = nullptr, unsigned* column = nullptr, > + unsigned frameOffset = 0); Personally, I think of this as more of a "depth" than an "offset", but perhaps that terminology is used elsewhere. Doesn't really matter. (And one could argue that it's a height, not a depth, since we say "up the callstack".) ::: js/src/shell/OSObject.cpp @@ +709,5 @@ > > + JS_FN_HELP("getSeparator", ospath_getSeparator, 0, 0, > +"getSeparator()", > +" Get the operating system's path separator."), > + I've been wanting a way to easily expose getters for the shell, so this could be os.path.separator yet still show up in help, but no point in blocking this on that. ::: js/src/shell/js.cpp @@ +4334,5 @@ > static bool > +GetFilenameForScriptedFrame(JSContext* cx, unsigned argc, Value* vp) > +{ > + CallArgs args = CallArgsFromVp(argc, vp); > + if (args.length() != 1) { if (args.get(0).isUndefined()) to be a little more like things from the spec. @@ +4340,5 @@ > + "getFilenameForScriptedFrame", "0", "s"); > + return false; > + } > + > + if (!args[0].isNumber()) { I might argue that this should be !ToNumber(...), but we do exact type checks other places, so I'm ok with having it here.

Till, are you still working on this bug?

Fwiw, I completely forgot about this bug, but I started working on it independently to support my work on wasm tests. Feel free to assign to me; I'm trying to have something up for review in the next few weeks.

This depends on <https://github.com/w3c/web-platform-tests/pull/10650> being sync'ed into m-c, and <https://github.com/w3c/web-platform-tests/pull/10900> landing upstream and being sync'ed.

Comment on attachment 8974646 [details] [diff] [review] Patch v1 Review of attachment 8974646 [details] [diff] [review]: ----------------------------------------------------------------- Looks like there's some information loss when computing the results, but I guess that's fine.

Excited to see this progress! Since I'm not really familiar with the test harness code, could you summarize at a high level how things will work with this patch, including what the workflow looks like for writing and running tests from the JS shell?

WPT has a mechanism for running the same test in several global scopes (windows and various workers), which I extended so that tests can opt in to running in a shell (this is the `// META` line in the example patch attached here). <https://web-platform-tests.org/writing-tests/testharness.html#multi-global-tests> has some documentation. By default, we'll run the test in the full browser in a window and a dedicated worker. Once you add such a test, you need to run ./mach wpt-manifest-update, which will add the new test to the manifest, and then you can run it by passing a path to the ./jstests.py invocation (relative to the copy of WPT in m-c), so e.g. `./jstests.py bin/js wasm/` will run all shell tests in testing/web-platform/tests/wasm/. Failing tests (or, more likely, subtests) can be marked as such in an .ini file in the parallel folder in testing/web-platform/meta/. The harness will report a regression if a test that should pass fails, as well as if one that should fail passes, or if a failing tests starts crashing, etc.. It should be possible to limit expected results by platform / JS_DEBUG / ..., though I haven't tested that in the jsshell harness. Let me know if you have any more questions; I'll answer them when I get back in the office next week.

Thanks; that's sounding great! So when I run `./jstests.py bin/js foo`, atm, afaics, jstests.py does a simple pattern match of "foo" against the test files' paths. With your patch, does the pattern match extend to all tests in testing/web-platform/meta/MANIFEST.json (I guess, only those declared as shell)? In that case, is there nothing special about the 'wasm' subdir of web-platform/tests and jstests.py can run any shell test in any web-platform subdir? If so, that all sounds great. All my other questions were answered by reading https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests :)

Comment on attachment 8974646 [details] [diff] [review] Patch v1 Benjamin has actually hacked on the test harness code before, so perhaps he should take a quick look, but this all lgtm!

Oh one other question: with this patch, will the .any.js tests marked as '// META: global=jsshell' be run by default by both the browser wpt runner and shell test runner (in normal automation runs)? I think that would be ideal.

(In reply to Luke Wagner [:luke] from comment #15) > Thanks; that's sounding great! > > So when I run `./jstests.py bin/js foo`, atm, afaics, jstests.py does a > simple pattern match of "foo" against the test files' paths. With your > patch, does the pattern match extend to all tests in > testing/web-platform/meta/MANIFEST.json (I guess, only those declared as > shell)? In that case, is there nothing special about the 'wasm' subdir of > web-platform/tests and jstests.py can run any shell test in any web-platform > subdir? If so, that all sounds great. Right, I just needed to pick any subdir to put my example test in. (In reply to Luke Wagner [:luke] from comment #17) > Oh one other question: with this patch, will the .any.js tests marked as '// > META: global=jsshell' be run by default by both the browser wpt runner and > shell test runner (in normal automation runs)? I think that would be ideal. Yep, and the browser harness will run it twice: once in a window, and once in a worker.

Beautiful, thanks!

Comment on attachment 8974646 [details] [diff] [review] Patch v1 Review of attachment 8974646 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks. ::: js/src/tests/jstests.py @@ +324,5 @@ > + test_manifests = testloader.ManifestLoader(test_paths).load() > + > + run_info = wpttest.get_run_info(kwargs["metadata_root"], "firefox", > + debug=debug) > + # TODO: Figure out why this is necessary. nit: TODO in code; please address or remove or link against a bug number to investigate more. @@ +349,5 @@ > + loader = get_wpt_test_loader(requested_paths, excluded_paths, debug) > + count = 0 > + for _ in loader.iter_tests(): > + count += 1 > + return count suggestion: return len(list(loader.iter_tests())) ::: js/src/tests/lib/results.py @@ +61,5 @@ > + """ > + from wptrunner.executors.base import testharness_result_converter > + > + out, err, rc = output.out, output.err, output.rc > + if rc: since the condition right below tests for a numeric value, can you make this one more explicit, like `if rc != 0`, if that's what you meant? It seems error prone to rely on the implicit boolean conversion here. @@ +73,5 @@ > + for line in output.out.split("\n"): > + if line.startswith("WPT OUTPUT: "): > + msg = line[len("WPT OUTPUT: "):] > + break > + if msg: Is it falsy if `msg` contains an empty string? Can it happen? (if not, we could fuse this `if` body with the one in the loop)

Comment on attachment 8974646 [details] [diff] [review] Patch v1 Review of attachment 8974646 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/jstests.py @@ +324,5 @@ > + test_manifests = testloader.ManifestLoader(test_paths).load() > + > + run_info = wpttest.get_run_info(kwargs["metadata_root"], "firefox", > + debug=debug) > + # TODO: Figure out why this is necessary. jgraham: ideas on this? Will just file a bug otherwise. @@ +349,5 @@ > + loader = get_wpt_test_loader(requested_paths, excluded_paths, debug) > + count = 0 > + for _ in loader.iter_tests(): > + count += 1 > + return count AFAICT, the entire count/load split exists only to avoid allocating such a list. It might be pretty small now, but it should hopefully get a lot bigger.

> jgraham: ideas on this? Will just file a bug otherwise. Because you're loading the expectation data for Firefox. The run_info is the input to the conditionals used to set per-platform expectations, and we still have non-e10s configurations.

Now using the new filtering API.

Lost a part.

Pushed by Ms2ger@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/27d38e893ea9 Support running specific WPT tests in the JS shell; r=luke,bbouvier,jgraham

Pushed by Ms2ger@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/87b1eaea27ed Followup: fix a couple of flake8 failures on a CLOSED TREE.

Backed out 2 changesets (bug 1333800) for spidermonkey build bustages Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/51c5667299a2bdae23145308aa621340f92832aa Pushes with the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=27d38e893ea9d1bcd7efa01762a858832a0aa8b3 https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=87b1eaea27ed35bc40342630bdedb7a59a3a57b0 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186608819&repo=mozilla-inbound&lineNumber=150219

Bob: blame claims you added this test; feel free to redirect :)

See the interdiff.

Comment on attachment 8990214 [details] [diff] [review] Patch v3 Review of attachment 8990214 [details] [diff] [review]: ----------------------------------------------------------------- So this just does nothing if we're in the wrong directory? I guess that's reasonable but it naively seems there ought to be a better way to handle it.

Comment on attachment 8990199 [details] [diff] [review] Part a: Make non262/extensions/regress-50447-1.js more robust against changes in the filenames. Review of attachment 8990199 [details] [diff] [review]: ----------------------------------------------------------------- rs=me

Pushed by Ms2ger@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a197a960866b Part a: Make non262/extensions/regress-50447-1.js more robust against changes in the filenames; rs=Waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/0f277c17a06f Part b: Support running specific WPT tests in the JS shell; r=luke,bbouvier,jgraham

https://hg.mozilla.org/mozilla-central/rev/a197a960866b https://hg.mozilla.org/mozilla-central/rev/0f277c17a06f