1488469 - Add tests for WebAssembly static methods

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9006288 [details] [diff] [review]
Patch v1

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

Thanks!

::: testing/web-platform/tests/wasm/jsapi/constructor/compile.any.js
@@ +65,5 @@
> +}, "Invalid code");
> +
> +promise_test(() => {
> +  return WebAssembly.compile(emptyModuleBinary).then(assert_Module);
> +}, "Result type");

Should we move this test prior to the branding one? It seems to subsume the branding test with undefined.

::: testing/web-platform/tests/wasm/jsapi/constructor/instantiate.any.js
@@ +121,5 @@
> +
> +promise_test(() => {
> +  const [buffer, expected] = createModule();
> +  return WebAssembly.instantiate(buffer).then(result => assert_WebAssemblyInstantiatedSource(result, expected));
> +}, "BufferSource argument");

(same question regarding order of tests with branding)

@@ +179,5 @@
> +  }).then(instance => {
> +    assert_Instance(instance, expected)
> +    assert_equals(instance.exports.fn(), value);
> +  });
> +}, "exports and imports: Module argument");

What do you think of adding tests that fail if:
- there's no import object?
- a subset of key is missing?
- (I guess all the tests you've done with the sync Instance ctor would apply here)

::: testing/web-platform/tests/wasm/jsapi/instance/constructor.any.js
@@ +204,5 @@
> +        kExprEnd,
> +    ])
> +    .exportFunc();
> +
> +  const buffer = builder.toBuffer()

nit: ; at the end

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Comment on attachment 9006288 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 9006288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: testing/web-platform/tests/wasm/jsapi/constructor/compile.any.js
> @@ +65,5 @@
> > +}, "Invalid code");
> > +
> > +promise_test(() => {
> > +  return WebAssembly.compile(emptyModuleBinary).then(assert_Module);
> > +}, "Result type");
> 
> Should we move this test prior to the branding one? It seems to subsume the
> branding test with undefined.
> 
> ::: testing/web-platform/tests/wasm/jsapi/constructor/instantiate.any.js
> @@ +121,5 @@
> > +
> > +promise_test(() => {
> > +  const [buffer, expected] = createModule();
> > +  return WebAssembly.instantiate(buffer).then(result => assert_WebAssemblyInstantiatedSource(result, expected));
> > +}, "BufferSource argument");
> 
> (same question regarding order of tests with branding)

Most of the tests I've written here start with things that I would not expect anyone to do intentionally, and move towards more plausible real-world scenarios as you get further along. IMO, the test for (the absence of) branding checks belongs more with the first part than the second, even though it doesn't throw. (Of course I'm still happy to change it if you feel strongly.)

> @@ +179,5 @@
> > +  }).then(instance => {
> > +    assert_Instance(instance, expected)
> > +    assert_equals(instance.exports.fn(), value);
> > +  });
> > +}, "exports and imports: Module argument");
> 
> What do you think of adding tests that fail if:
> - there's no import object?
> - a subset of key is missing?
> - (I guess all the tests you've done with the sync Instance ctor would apply
> here)

I made a note on my todo list to make sure they have equivalent coverage; I need to think a bit about the best way to share code between them.

> ::: testing/web-platform/tests/wasm/jsapi/instance/constructor.any.js
> @@ +204,5 @@
> > +        kExprEnd,
> > +    ])
> > +    .exportFunc();
> > +
> > +  const buffer = builder.toBuffer()
> 
> nit: ; at the end

Fixed here and in two other places. Thanks for noticing.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #2)
> Most of the tests I've written here start with things that I would not
> expect anyone to do intentionally, and move towards more plausible
> real-world scenarios as you get further along. IMO, the test for (the
> absence of) branding checks belongs more with the first part than the
> second, even though it doesn't throw. (Of course I'm still happy to change
> it if you feel strongly.)

OK, makes sense with your ordering then (I don't care too strongly either).

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c622661cfe2
Add tests for WebAssembly static methods; r=bbouvier

Comment 5

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12871 for changes under testing/web-platform/tests

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12871
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/425256349?utm_source=github_status&utm_medium=notification)

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1c622661cfe2

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged