wasm: september testing cleanups

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
No description provided.
Assignee

Comment 1

3 years ago
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.
Attachment #8788850 - Flags: review?(luke)
Assignee

Comment 2

3 years ago
This hoists evalText and textToBinary in the wasm.js lib.
Attachment #8788852 - Flags: review?(luke)
Assignee

Comment 3

3 years ago
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.
Attachment #8788853 - Flags: review?(luke)
Assignee

Comment 4

3 years ago
Posted patch 4.full-testing.patch (obsolete) — Splinter Review
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)
Attachment #8788854 - Flags: feedback?(luke)

Updated

3 years ago
Attachment #8788850 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8788852 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8788853 - Flags: review?(luke) → review+
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?
Attachment #8788854 - Flags: feedback?(luke) → feedback+
Assignee

Updated

3 years ago
Keywords: leave-open

Comment 6

3 years ago
Pushed by bbouvier@mozilla.com:
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
Assignee

Comment 8

3 years ago
Attachment #8788854 - Attachment is obsolete: true
Attachment #8797951 - Flags: review?(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 :)
Attachment #8797951 - Flags: review?(luke) → review+
Assignee

Updated

3 years ago
Keywords: leave-open
Priority: -- → P1

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/11a8cce819d2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 won't fix as wasm will be shipped in 52.
You need to log in before you can comment on or make changes to this bug.