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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch Patch v1Splinter Review
      No description provided.
Attachment #9009044 - Flags: review?(bbouvier)
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+
(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? :)
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
https://hg.mozilla.org/mozilla-central/rev/b969eeed85fa
Status: NEW → RESOLVED
Closed: 6 years ago
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: