Closed
Bug 1491254
Opened 6 years ago
Closed 6 years ago
Share tests between new WebAssembly.Instance() and WebAssembly.instantiate()
Categories
(Core :: JavaScript: WebAssembly, enhancement)
Core
JavaScript: WebAssembly
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: Ms2ger, Assigned: Ms2ger)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
18.57 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #9009044 -
Flags: review?(bbouvier)
Comment 1•6 years ago
|
||
Comment on attachment 9009044 [details] [diff] [review] Patch v1 Review of attachment 9009044 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, as long as the code deletion is discarded from this patch. Thanks. ::: testing/web-platform/tests/wasm/jsapi/constructor/instantiate.any.js @@ +81,5 @@ > +for (const [name, fn] of moduleCreators) { > + promise_test(() => { > + const { buffer, args, exports, verify } = fn(); > + return WebAssembly.instantiate(buffer, ...args).then(result => { > + assert_WebAssemblyInstantiatedSource(result, exports) nit: missing ; @@ +90,5 @@ > + promise_test(() => { > + const { buffer, args, exports, verify } = fn(); > + const module = new WebAssembly.Module(buffer); > + return WebAssembly.instantiate(module, ...args).then(instance => { > + assert_Instance(instance, exports) ditto ::: testing/web-platform/tests/wasm/jsapi/instance/constructor.any.js @@ -50,5 @@ > - for (const argument of invalidArguments) { > - assert_throws(new TypeError(), () => new WebAssembly.Instance(module, argument), > - `new Instance(module, ${format_value(argument)})`); > - } > -}, "Non-object imports"); I think we're losing test coverage here, since this isn't replaced. Maybe just discard this removal? ::: testing/web-platform/tests/wasm/jsapi/modules.js @@ +1,1 @@ > +const moduleCreators = [ nit: naming in this file is a bit disingenuous, because the `args` we provide are to the Instance ctor/WebAssembly.Instantiate functions, and the extra checks we're doing happen on the Instance. The frontier is thin between modules and instances, though, but what do you think of renaming moduleCreators => instanceCreators and this file accordingly?
Attachment #9009044 -
Flags: review?(bbouvier) → review+
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #1) > Comment on attachment 9009044 [details] [diff] [review] > Patch v1 > > Review of attachment 9009044 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/web-platform/tests/wasm/jsapi/instance/constructor.any.js > @@ -50,5 @@ > > - for (const argument of invalidArguments) { > > - assert_throws(new TypeError(), () => new WebAssembly.Instance(module, argument), > > - `new Instance(module, ${format_value(argument)})`); > > - } > > -}, "Non-object imports"); > > I think we're losing test coverage here, since this isn't replaced. Maybe > just discard this removal? I forgot to mention that; I wrote those tests twice, once here and once in https://searchfox.org/mozilla-central/rev/bdc89dfd7869e418d788b28eb60ab8d94e708a15/testing/web-platform/tests/wasm/jsapi/bad-imports.js > ::: testing/web-platform/tests/wasm/jsapi/modules.js > @@ +1,1 @@ > > +const moduleCreators = [ > > nit: naming in this file is a bit disingenuous, because the `args` we > provide are to the Instance ctor/WebAssembly.Instantiate functions, and the > extra checks we're doing happen on the Instance. The frontier is thin > between modules and instances, though, but what do you think of renaming > moduleCreators => instanceCreators and this file accordingly? Yeah, I wan't completely happy about that. instanceCreators seems worse, though, because the functions return the inputs necessary to create an Instance, rather than creating them themselves... Any other ideas? :)
Comment 3•6 years ago
|
||
I don't excessively care here... instanceTestCreator? (or Factory for an extra Java feel to it) Anyway, anything works fine, I don't expect us to have to look at this file too much.
Pushed by Ms2ger@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b969eeed85fa Share tests between new WebAssembly.Instance() and WebAssembly.instantiate(); r=bbouvier
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b969eeed85fa
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13162 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/13162 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/PPcvCE69R0C5h6pWAjBCuw)
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•