1519901 - Move core wasm tests to jstests/wpt

Status: RESOLVED WONTFIX

(Core :: JavaScript: WebAssembly, task, P4)

Comment 1

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

Attachment: Part a: Move tests

jgraham: please check the change in testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py; better approach is welcome :)

Comment 2

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

Attachment: Part b: Remove the tests in testing/web-platform/mozilla/

Comment 3

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

Attachment: Part c: Remove two copies of wast.js

Comment 4

[Lars T Hansen [:lth]]

I'd like to block this bug until we've discussed the repercussions for test coverage in detail. Given how slow it is to run jstests, and given the fact that we're apparently losing various ways to control these tests with directives.txt, we will get significantly less testing and significantly worse testing if we move these tests. (Again, this is my superficial understanding.)

Comment 5

[James Graham [:jgraham]]

Comment on attachment 9036383 [details] [diff] [review]
Part a: Move tests

Review of attachment 9036383 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py
@@ +149,5 @@
>          return False
>  
>      return {"e10s": kwargs["gecko_e10s"],
>              "wasm": kwargs.get("wasm", True),
> +            "arm": kwargs.get("arm", False),

I think there has been a recent move against kwargs that don't correspond to command line arguments, since they tend to cause confusion with people thinking that they're unused. Maybe we should have a generic command line arg that allows overriding runinfo properties like --run-info='arm=true'

Comment 6

[James Graham [:jgraham]]

(In reply to Lars T Hansen [:lth] from comment #4)

I'd like to block this bug until we've discussed the repercussions for test coverage in detail. Given how slow it is to run jstests, and given the fact that we're apparently losing various ways to control these tests with directives.txt, we will get significantly less testing and significantly worse testing if we move these tests. (Again, this is my superficial understanding.)

If this is true, we should probably talk about how to restore that coverage with wpt tests.

Comment 7

[Luke Wagner [:luke]]

For my understanding: what's the advantage to running the wasm wpts via jstests.py vs. jit_tests.py?

(In reply to James Graham [:jgraham] from comment #6)
For more details: we're able to catch a ton of bugs, and get a lot more mileage out of each wpt, by running the same tests with various wasm-specific shell flags that make sure we hit all the interesting jitting/tiering combinations. Losing this would be a big coverage regression, so that's definitely a hard blocker.

Lars: you also pointed out the ease/speed of running the unit tests. Already, jit-tests takes to long (at least for me) to run all the time, so I always run jit-tests with jit_test.py $(SHELL) wasm to filter down to just the relevant tests. Trying locally, it looks like jstests.py $(SHELL) wasm works reasonably well. (It actually took a really long time the first time, but on subsequent runs it's much faster; I guess we generate some list the first time?) Does that work?

Comment 8

[Lars T Hansen [:lth]]

Sure, I mostly just test in the wasm subtree too, but my experience with jstests.py does not quite match yours, here it usually sits and churns for a while before it figures out which tests I'm asking for. It's possible I don't run it frequently enough to have a warm cache though.

Comment 9

[James Graham [:jgraham]]

So the jstests.py implementation of running wpt tests just uses wpt stuff to find and load the tests; the actual test running is all using the existing jstests.py machinary, so anything in terms of flags and so on should already work there as best as I can tell (idk if there are further differences with jit_tests.py). It's also true that loading wpt tests is a noticable regression in startup time for jstests.py, so they were made opt-in in Bug 1515039.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9036383 [details] [diff] [review]
Part a: Move tests

Review of attachment 9036383 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, we shouldn't reduce testing coverage. Adding support for testing directives (directives.txt like files would be perfect, so we can easily add/remove directives) is a prerequisite for this change.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9036384 [details] [diff] [review]
Part b: Remove the tests in testing/web-platform/mozilla/

Review of attachment 9036384 [details] [diff] [review]:
-----------------------------------------------------------------

Let's figure out the coverage situation first.

But let me confirm my understanding: once we move all the tests that live in jit-tests/tests/wasm/spec to the WPT harness, they can be both run in the shell *and* in the browser. If that's not true, then I expect we get a system to run in browser as well (no need to be able to set prefs here). This has helped catch a few browser-only bugs in the past.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9036385 [details] [diff] [review]
Part c: Remove two copies of wast.js

Review of attachment 9036385 [details] [diff] [review]:
-----------------------------------------------------------------

From a quick searchfox glance, it seems unused even as of now. If that's the case, feel free to land this one now; otherwise we'll need to figure out the situation for other patches before.

Comment 13

[James Graham [:jgraham]]

But let me confirm my understanding: once we move all the tests that live in jit-tests/tests/wasm/spec to the WPT harness, they can be both run in the shell and in the browser

This is correct.

Comment 14

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

(In reply to Benjamin Bouvier [:bbouvier] from comment #10)

Comment on attachment 9036383 [details] [diff] [review]
Part a: Move tests

Review of attachment 9036383 [details] [diff] [review]:

Yeah, we shouldn't reduce testing coverage. Adding support for testing
directives (directives.txt like files would be perfect, so we can easily
add/remove directives) is a prerequisite for this change.

Can you give me a pointer to the directives and how they work / what they do?

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

It's a good question, we don't have much documentation about it. Basically, if there's a directives.txt file in a directory, it's applied to all the tests in the same directory, according to this. Then the test-also directives are stored and later reused to add more test runs for a given source file. Very concretely, if I've got a test file with |jit-test| test-also=--wasm-compiler=baseline; test-also=--wasm-compiler=ion;, it will run the tests in the file in three different modes:

• nothing special (i.e. no extra shell switches)
• ran with the --wasm-compiler=baseline flag, in addition to other switches (note some flags are overriden by the shell according to their order in the run line, so be careful that this can happen)
• ran with the --wasm-compiler=ion flag

We'd like something like this for the web platform tests too before we can fully migrate there.

Comment 16

[Lars T Hansen [:lth]]

An alternative is that we provide a --wasm-tbpl switch to tests/jstests.py to mirror --tblp; this would request a set of flags that's appropriate for broader wasm testing. It's a little tricky because directives.txt are not the same in all directories; for example in wasm/gc we test with --wasm-gc, but we don't do that elsewhere. Running the standard tests with --wasm-gc is not useful, it will just suck up CPU time.

That's said, it's a bit open how much longer --wasm-gc will remain useful and we should consider whether the differences in directives.txt in the wasm subdir is a red herring.

Comment 17

[Lars T Hansen [:lth]]

Looks stalled?

Comment 18

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

Yeah. There's also a stalled attempt from the Chromium side: https://github.com/web-platform-tests/wpt/pull/16321 . I don't have a clue when I'd be able to come back to it.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 20

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 22

[Ryan Hunt [:rhunt] (on leave until early May)]

We now use a custom wast -> js converter that uses shell-only builtins for many things like NaN's, v128, and GC types. Making this work in WPT is not likely to be feasible. There is some movement around getting spec tests into upstream WPT, which could solve this for us, but may have some limitations [1].

Closing this now as I don't think we'll likely work on this.

[1] https://github.com/web-platform-tests/interop/issues/545