Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[js-api] Add tests for WebAssembly.JSTag #319

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Jul 1, 2024

This adds tests for WebAssembly.JSTag. The tests are adapted from https://github.com/v8/v8/blob/main/test/mjsunit/wasm/exnref-api.js.

Confirmed that they run successfully with web test infrastructure in Chrome with --js-flags=--experimental-wasm-exnref command line argument. The only thing failing there was the shadowrealm test, which I think we should add a separate expectation files like the existing ***.tentative.any.shadowrealm-expected.txt in
https://github.com/chromium/chromium/tree/main/third_party/blink/web_tests/external/wpt/wasm/jsapi/exception. But given that we don't host these files in this EH repo, I'll just upload the js files.

This adds tests for `WebAssembly.JSTag`. The tests are adapted from
https://github.com/v8/v8/blob/main/test/mjsunit/wasm/exnref-api.js.

Confirmed that they run successfully with web test infrastructure in
Chrome with `--js-flags=--experimental-wasm-exnref` command line
argument. The only thing failing there was the shadowrealm test, which I
think we should add a separate expectation files like the existing
`***.tentative.any.shadowrealm-expected.txt` in
https://github.com/chromium/chromium/tree/main/third_party/blink/web_tests/external/wpt/wasm/jsapi/exception.
But given that we don't host these files in this EH repo, I'll just
upload the js files.
@aheejin aheejin requested review from Ms2ger and dschuff July 1, 2024 23:21

test(() => {
assert_throws_js(TypeError, () => new WebAssembly.Exception(WebAssembly.JSTag, [{}]))
}, "Creating a WebAssembly.Exception with JSTag explicitly is not allowed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I'm showing my lack of knowledge about the details of the test infra, but does this assertion check the exact text of the error message? The spec mandates what kind of exception should be thrown (TypeError), but not the text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this string is checked against anything else. So I just used it to describe what the tests were about. Maybe I'm abusing it? 🤷🏻

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it's fine as long as the test won't fail because of different error messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is just the name of the subtest.

}
const buffer = builder.toBuffer();
const {instance} = await WebAssembly.instantiate(buffer, {
module: { throw_ref, JSTag: WebAssembly.JSTag }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be throw_ref: throw_ref,?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works, but it looks in case the imported name and the external name are the same, just writing it once seems to have the same effect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is the shorthand syntax in JS object initializers.

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


test(() => {
assert_throws_js(TypeError, () => new WebAssembly.Exception(WebAssembly.JSTag, [{}]))
}, "Creating a WebAssembly.Exception with JSTag explicitly is not allowed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is just the name of the subtest.

}
const buffer = builder.toBuffer();
const {instance} = await WebAssembly.instantiate(buffer, {
module: { throw_ref, JSTag: WebAssembly.JSTag }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is the shorthand syntax in JS object initializers.

});

const obj = {};
const wasmTag = new WebAssembly.Tag({parameters:['externref']});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears unused in this subtest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@aheejin aheejin merged commit 4c93161 into WebAssembly:main Jul 4, 2024
1 check passed
@aheejin aheejin deleted the jstag_test branch July 4, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants