Closed
Bug 1301029
Opened 7 years ago
Closed 6 years ago
wasm: september testing cleanups
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(4 files, 1 obsolete file)
1.94 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
20.17 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
3.74 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
2.25 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 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•7 years ago
|
||
This hoists evalText and textToBinary in the wasm.js lib.
Attachment #8788852 -
Flags: review?(luke)
Assignee | ||
Comment 3•7 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•7 years ago
|
||
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•7 years ago
|
Attachment #8788850 -
Flags: review?(luke) → review+
![]() |
||
Updated•7 years ago
|
Attachment #8788852 -
Flags: review?(luke) → review+
![]() |
||
Updated•7 years ago
|
Attachment #8788853 -
Flags: review?(luke) → review+
![]() |
||
Comment 5•7 years ago
|
||
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•7 years ago
|
Keywords: leave-open
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9b1425c4b569 https://hg.mozilla.org/mozilla-central/rev/7092faec0016 https://hg.mozilla.org/mozilla-central/rev/77399867b286
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #8788854 -
Attachment is obsolete: true
Attachment #8797951 -
Flags: review?(luke)
![]() |
||
Comment 9•6 years ago
|
||
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•6 years ago
|
Keywords: leave-open
Comment 10•6 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/11a8cce819d2 Add wasm full testing; r=luke
Updated•6 years ago
|
Priority: -- → P1
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11a8cce819d2
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 12•6 years ago
|
||
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.
Description
•