test: Add basic WebAssembly test by stevekinney · Pull Request #16760 · nodejs/node · GitHub
Skip to content

test: Add basic WebAssembly test#16760

Closed
stevekinney wants to merge 1 commit into
nodejs:masterfrom
stevekinney:basic-wasm-test
Closed

test: Add basic WebAssembly test#16760
stevekinney wants to merge 1 commit into
nodejs:masterfrom
stevekinney:basic-wasm-test

Conversation

@stevekinney

@stevekinney stevekinney commented Nov 4, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Tests a basic WebAssembly module that adds two numbers.

wasm example from the WebAssembly/wabt repo licensed Apache 2.0.

Not sure if there are licensing concerns. Please let me know if we need to add anything else.

Refs: https://github.com/WebAssembly/wabt/blob/master/demo/wat2wasm/examples.js#L27-L32

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 4, 2017
@MylesBorins

MylesBorins commented Nov 4, 2017

Copy link
Copy Markdown
Contributor

addaleax
addaleax approved these changes Nov 4, 2017
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

Comment thread test/parallel/test-wasm-simple.js Outdated

@addaleax addaleax Nov 4, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this pass make lint? I think we have a rule against assert.equal (in favour of assert.strictEqual)

@MylesBorins MylesBorins Nov 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

updated to fix lint

@mscdex

mscdex commented Nov 4, 2017

Copy link
Copy Markdown
Contributor

Isn't this just testing a strictly V8 feature? Wouldn't it be best to leave this to the V8 test suite?

@addaleax

addaleax commented Nov 4, 2017

Copy link
Copy Markdown
Member

@mscdex It will also be testing NodePlatform code once async WASM compilation is enabled by default in V8, so this will act as a test for both NodePlatform and for the WASM code in V8.

cjihrig
cjihrig approved these changes Nov 4, 2017
View reviewed changes
Comment thread test/parallel/test-wasm-simple.js Outdated

@cjihrig cjihrig Nov 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fixtures.readSync('simple.wasm') and then drop the fs module from the test?

@stevekinney
test: Add basic WebAssembly test
71036dd 
Tests a basic WebAssembly module that adds two numbers.
wasm example from the WebAssembly/wabt repo licensed
Apache 2.0.

Refs: https://github.com/WebAssembly/wabt/blob/master/demo/wat2wasm/examples.js#L27-L32
@MylesBorins

MylesBorins commented Nov 4, 2017

Copy link
Copy Markdown
Contributor

one more ci: https://ci.nodejs.org/job/node-test-pull-request/11195/

MylesBorins
MylesBorins approved these changes Nov 4, 2017
View reviewed changes

@MylesBorins MylesBorins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

thefourtheye
thefourtheye approved these changes Nov 5, 2017
View reviewed changes
Comment thread test/parallel/test-wasm-simple.js

assert.ok(WebAssembly.validate(buffer), 'Buffer should be valid WebAssembly');

WebAssembly.instantiate(buffer, {}).then((results) => {

@thefourtheye thefourtheye Nov 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: common.mustCall around the then would be better.

@addaleax addaleax Nov 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh, right … we might also want common.crashOnUnhandledRejection() to make sure it really crashes when the promise gets rejected

edit: this test might be trickier than first thought 😄

jasnell
jasnell reviewed Nov 8, 2017
View reviewed changes
Comment thread test/parallel/test-wasm-simple.js
assert.strictEqual(
results.instance.exports.addTwo(10, 20),
30,
'Exported function should add two numbers.',

@jasnell jasnell Nov 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggestion: it would likely be better to either omit the message here or include the expected and actual values in the message.

@MylesBorins MylesBorins Nov 14, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this worth blocking on or should we land and fix in a follow up?

@addaleax addaleax Nov 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yea, we should just land this

@addaleax

addaleax commented Nov 15, 2017

Copy link
Copy Markdown
Member

@addaleax addaleax closed this Nov 15, 2017
addaleax pushed a commit that referenced this pull request Nov 15, 2017
@stevekinney @addaleax
test: add basic WebAssembly test
74e7a4a 
Tests a basic WebAssembly module that adds two numbers.
wasm example from the WebAssembly/wabt repo licensed
Apache 2.0.

Refs: https://github.com/WebAssembly/wabt/blob/49b7984544ddaf14d5e2f1ad9115dad7e9a2b299/demo/wat2wasm/examples.js#L27-L32
PR-URL: #16760
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
@stevekinney @MylesBorins
test: add basic WebAssembly test
113dd2b 
Tests a basic WebAssembly module that adds two numbers.
wasm example from the WebAssembly/wabt repo licensed
Apache 2.0.

Refs: https://github.com/WebAssembly/wabt/blob/49b7984544ddaf14d5e2f1ad9115dad7e9a2b299/demo/wat2wasm/examples.js#L27-L32
PR-URL: #16760
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@stevekinney @gibfahn
test: add basic WebAssembly test
fca180e 
Tests a basic WebAssembly module that adds two numbers.
wasm example from the WebAssembly/wabt repo licensed
Apache 2.0.

Refs: https://github.com/WebAssembly/wabt/blob/49b7984544ddaf14d5e2f1ad9115dad7e9a2b299/demo/wat2wasm/examples.js#L27-L32
PR-URL: #16760
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@stevekinney @gibfahn
test: add basic WebAssembly test
621390f 
Tests a basic WebAssembly module that adds two numbers.
wasm example from the WebAssembly/wabt repo licensed
Apache 2.0.

Refs: https://github.com/WebAssembly/wabt/blob/49b7984544ddaf14d5e2f1ad9115dad7e9a2b299/demo/wat2wasm/examples.js#L27-L32
PR-URL: #16760
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
+3 more reviewers
@MylesBorins MylesBorins MylesBorins approved these changes
@thefourtheye thefourtheye thefourtheye approved these changes
@hiroppy hiroppy hiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@stevekinney @MylesBorins @mscdex @addaleax @jasnell @thefourtheye @cjihrig @hiroppy @gibfahn @nodejs-github-bot