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

Import jsapi tests from WPT. #883

Merged
merged 4 commits into from
Oct 16, 2018
Merged

Conversation

Ms2ger
Copy link
Collaborator

@Ms2ger Ms2ger commented Oct 2, 2018

No description provided.

builder => {
builder.addImport("module", "fn", kSig_v_v);
},
value);
Copy link
Member

Choose a reason for hiding this comment

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

should be imports, right?

const bytes2 = [74, 83, 65, 80, 73];

const binary = new Binary;
binary.emit_section(kUnknownSectionCode, section => {
Copy link
Member

Choose a reason for hiding this comment

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

Probably should rename to kCustomSectionCode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to bring the module builder code in sync with the other copy in this repo, but I'd rather not block this PR on that.

Copy link
Member

Choose a reason for hiding this comment

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

OK, sgtm

assert_sections(WebAssembly.Module.customSections(module, "na\uFFFDme"), [
bytes,
]);
assert_sections(WebAssembly.Module.customSections(module, "na\uDC01me"), [
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit surprising to me, since the customSections algorithm says it uses UTF-8 decode without BOM or fail, which doesn't seem to convert the 0xdc01 to a 0xfffd (though I see that elsewhere in the spec). Does the replacement happen somewhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

customSections takes a USVString, which introduces the replacement character; the "UTF-8 decode without BOM or fail" algorithm is only applied to the bytes in the module, which contain an explicit U+FFFD from the emit_string call.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that makes sense.

let kElementSectionCode = 9; // Elements section
let kCodeSectionCode = 10; // Function code
let kDataSectionCode = 11; // Data segments
let kNameSectionCode = 12; // Name section (encoded as string)
Copy link
Member

Choose a reason for hiding this comment

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

Remove (unused)

"invalid index into function table"
];

function assertTraps(trap, code) {
Copy link
Member

Choose a reason for hiding this comment

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

remove (unused?)

let kTrapFuncSigMismatch = 7;
let kTrapInvalidIndex = 8;

let kTrapMsgs = [
Copy link
Member

Choose a reason for hiding this comment

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

Not specified, right?

@littledan
Copy link
Collaborator

Before landing this patch, please verify that it works with the procedures used in at least one browser Wasm implementation to regularly update from this repository (last time I checked, all four take the tests from here, in a manual pull process).

@Ms2ger
Copy link
Collaborator Author

Ms2ger commented Oct 10, 2018

(There will be no need to import from here into SpiderMonkey, as these tests already run there.)

@littledan
Copy link
Collaborator

littledan commented Oct 10, 2018

@Ms2ger maybe you could try with another one? It test it out with SpiderMonkey's old procedure?

@binji
Copy link
Member

binji commented Oct 10, 2018

cc @titzer for trying these tests on v8

@binji
Copy link
Member

binji commented Oct 12, 2018

I started running these in v8. It seems this uses a number of new functions from the https://web-platform-tests.org/writing-tests/testharness-api.html, so we should probably add these to README.md:

assert_throws
format_value
assert_class_string
setup
promise_rejects

See the WIP changes here: https://chromium-review.googlesource.com/c/v8/v8/+/1277724

@Ms2ger
Copy link
Collaborator Author

Ms2ger commented Oct 15, 2018

Thanks! Please let me know if you have any more comments; I'll update the readme some time this week.

@binji
Copy link
Member

binji commented Oct 16, 2018

OK, I have these tests running (but not yet all passing) in v8. Since they also run in spidermonkey, I think this is good to land. We can address other concerns in future CLs.

FYI @kmiller68 @Cellule

@binji binji merged commit cf67a47 into WebAssembly:master Oct 16, 2018
@Ms2ger Ms2ger deleted the import-jsapi-tests branch October 17, 2018 08:16
Honry added a commit to Honry/spec that referenced this pull request Oct 24, 2018
binji pushed a commit that referenced this pull request Oct 24, 2018
joyeecheung pushed a commit to joyeecheung/v8 that referenced this pull request Oct 25, 2018
These changes were necessary to run with the new style of jsapi tests
introduced in WebAssembly/spec#883.

Change-Id: I4629dd48d595ed97ed0607dec9e7d9808c706a7e
Reviewed-on: https://chromium-review.googlesource.com/c/1277724
Commit-Queue: Ben Smith <[email protected]>
Reviewed-by: Andreas Haas <[email protected]>
Reviewed-by: Michael Achenbach <[email protected]>
Reviewed-by: Mathias Bynens <[email protected]>
Cr-Commit-Position: refs/heads/master@{#56745}
joyeecheung pushed a commit to joyeecheung/v8 that referenced this pull request Oct 25, 2018
This reverts commit a12203c.

Reason for revert: Breaks isolate_tests

https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux%20-%20builder/36777

Original change's description:
> [wasm] Add a new wasm-js testsuite to run js-api tests
> 
> These changes were necessary to run with the new style of jsapi tests
> introduced in WebAssembly/spec#883.
> 
> Change-Id: I4629dd48d595ed97ed0607dec9e7d9808c706a7e
> Reviewed-on: https://chromium-review.googlesource.com/c/1277724
> Commit-Queue: Ben Smith <[email protected]>
> Reviewed-by: Andreas Haas <[email protected]>
> Reviewed-by: Michael Achenbach <[email protected]>
> Reviewed-by: Mathias Bynens <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#56745}

[email protected],[email protected],[email protected],[email protected],[email protected],[email protected]

Change-Id: I2edd0ca94cb5990322571879c81671fa835f3ecd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/1286526
Reviewed-by: Bill Budge <[email protected]>
Commit-Queue: Bill Budge <[email protected]>
Cr-Commit-Position: refs/heads/master@{#56746}
joyeecheung pushed a commit to joyeecheung/v8 that referenced this pull request Oct 25, 2018
This is a reland of a12203c

Original change's description:
> [wasm] Add a new wasm-js testsuite to run js-api tests
> 
> These changes were necessary to run with the new style of jsapi tests
> introduced in WebAssembly/spec#883.
> 
> Change-Id: I4629dd48d595ed97ed0607dec9e7d9808c706a7e
> Reviewed-on: https://chromium-review.googlesource.com/c/1277724
> Commit-Queue: Ben Smith <[email protected]>
> Reviewed-by: Andreas Haas <[email protected]>
> Reviewed-by: Michael Achenbach <[email protected]>
> Reviewed-by: Mathias Bynens <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#56745}

Change-Id: I25fcd95bfc1aee1d21da390359423e5dfed112a4
Reviewed-on: https://chromium-review.googlesource.com/c/1286952
Commit-Queue: Ben Smith <[email protected]>
Reviewed-by: Ben Smith <[email protected]>
Reviewed-by: Michael Achenbach <[email protected]>
Cr-Commit-Position: refs/heads/master@{#56791}
@Ms2ger Ms2ger mentioned this pull request Dec 18, 2018
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