1301029 - wasm: september testing cleanups

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.remove-prefix.patch

This just removes the basic- prefix from tests: I think the motivation was to have basic-* files be the one test cases we hand write, and everything coming out of fuzzers would go to random-* files. At some point Lars created the regress/ directory which contained fuzzbugs test cases, and we started using this (it's cleaner). So we can remove the historical basic- prefix now.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.hoist-evalText-textToBinary.patch

This hoists evalText and textToBinary in the wasm.js lib.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.disable-memory-tests-basicjs.patch

These tests have been dormant all the time in basic.js, and I think memory exports are correctly tested in import-exports.js nowadays, so we can just remove these.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.full-testing.patch

A preview of what full testing would look like: this validates a module, then runs it and checks the expected result, then does the binary -> text -> binary projection and re-runs the same checks.

This would mean either rewriting a lot of tests, or implementing new ones that use this API instead. I think this would provide a nice framework anyway. Funnily, as of today we can't even pass a simple module full-cycle test with the new-format, because binary->ast doesn't know about the new export format. (refactoring to make this happen is in other bugs)

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8788854 [details] [diff] [review]
4.full-testing.patch

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

Yeah, definitely like the idea here.

Btw, Dan is fixing WasmBinaryTo* as part of his 0xc patch.  It's my fault for not adding a newFormat path to WasmBinaryTo* when I added all the newFormat stuff.

::: js/src/jit-test/lib/wasm.js
@@ +106,5 @@
> +
> +// Fully test a module:
> +// - ensure it validates.
> +// - ensure it compiles and produces the expected result.
> +// - ensure textToBinary(binaryToText) is a projection.

I think "is a projection" isn't quite the right mathetmatical term.  You might say that "textToBinary ∘ binaryToText is the identity function", but we're only testing one particular point, so how about just "textToBinary(binaryToText(binary)) = binary"

@@ +109,5 @@
> +// - ensure it compiles and produces the expected result.
> +// - ensure textToBinary(binaryToText) is a projection.
> +// Preconditions:
> +// - the binary must use the new-format.
> +// - the binary module must export a function called "run".

How about using the start function?

@@ +110,5 @@
> +// - ensure textToBinary(binaryToText) is a projection.
> +// Preconditions:
> +// - the binary must use the new-format.
> +// - the binary module must export a function called "run".
> +function wasmFullPass(binary, expected, maybeImports) {

Could you factor one step further and pass the text?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1425c4b569
Remove the basic- prefix from wasm test cases; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/7092faec0016
Hoist textToBinary and evalText in lib/wasm.js; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/77399867b286
Remove disabled memory exports tests in basic.js; r=luke

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9b1425c4b569
https://hg.mozilla.org/mozilla-central/rev/7092faec0016
https://hg.mozilla.org/mozilla-central/rev/77399867b286

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4-full-testing.patch

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8797951 [details] [diff] [review]
4-full-testing.patch

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

::: js/src/jit-test/lib/wasm.js
@@ +118,5 @@
> +// - ensure it validates.
> +// - ensure it compiles and produces the expected result.
> +// - ensure textToBinary(binaryToText(binary)) = binary
> +// Preconditions:
> +// - the binary must use the new-format.

'new-format' is the only format now :)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a8cce819d2
Add wasm full testing; r=luke

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11a8cce819d2

Comment 12

[Gerry Chang [:gchang]]

Mark 51 won't fix as wasm will be shipped in 52.