test: Add basic WebAssembly test #16760

stevekinney · 2017-11-04T22:12:47Z

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

MylesBorins · 2017-11-04T22:16:20Z

ci: https://ci.nodejs.org/job/node-test-pull-request/11194/

/cc @nodejs/tsc what is the best practice for the licensing on this one? the WASM is generated from WAT code that came from an APACHE 2.0 project... we could potentially just write some new code for this but the base function is fairly straight forward.

addaleax

Thank you!

addaleax · 2017-11-04T22:18:12Z

test/parallel/test-wasm-simple.js

+assert.ok(WebAssembly.validate(buffer), 'Buffer should be valid WebAssembly');
+
+WebAssembly.instantiate(buffer, {}).then((results) => {
+  assert.equal(results.instance.exports.addTwo(10, 20), 30, 'Exported function should add two numbers.');


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

updated to fix lint

mscdex · 2017-11-04T22:24:31Z

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

addaleax · 2017-11-04T22:27:33Z

@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 · 2017-11-04T22:31:11Z

test/parallel/test-wasm-simple.js

+const fixtures = require('../common/fixtures');
+const fs = require('fs');
+
+const buffer = fs.readFileSync(fixtures.path('simple.wasm'));


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

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 · 2017-11-04T23:05:15Z

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

MylesBorins

LGTM

thefourtheye · 2017-11-05T05:26:05Z

test/parallel/test-wasm-simple.js

+
+assert.ok(WebAssembly.validate(buffer), 'Buffer should be valid WebAssembly');
+
+WebAssembly.instantiate(buffer, {}).then((results) => {


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

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 · 2017-11-08T09:45:16Z

test/parallel/test-wasm-simple.js

+  assert.strictEqual(
+    results.instance.exports.addTwo(10, 20),
+    30,
+    'Exported function should add two numbers.',


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

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

yea, we should just land this

addaleax · 2017-11-15T18:52:24Z

Landed in 74e7a4a, thanks for the PR & congratulations to being a Node contributor now! 🎉

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>

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

stevekinney force-pushed the basic-wasm-test branch from cd894c3 to f8bc445 Compare November 4, 2017 22:13

MylesBorins requested review from MylesBorins and addaleax November 4, 2017 22:14

addaleax approved these changes Nov 4, 2017

View reviewed changes

stevekinney force-pushed the basic-wasm-test branch from f8bc445 to 9538932 Compare November 4, 2017 22:24

cjihrig approved these changes Nov 4, 2017

View reviewed changes

stevekinney force-pushed the basic-wasm-test branch from 9538932 to 70ab5a2 Compare November 4, 2017 23:02

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

stevekinney force-pushed the basic-wasm-test branch from 70ab5a2 to 71036dd Compare November 4, 2017 23:04

MylesBorins approved these changes Nov 4, 2017

View reviewed changes

thefourtheye approved these changes Nov 5, 2017

View reviewed changes

hiroppy approved these changes Nov 8, 2017

View reviewed changes

jasnell reviewed Nov 8, 2017

View reviewed changes

addaleax closed this Nov 15, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

MylesBorins added dont-land-on-v4.x labels Dec 19, 2017

gibfahn added the land-on-v8.x label Dec 19, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add basic WebAssembly test #16760

test: Add basic WebAssembly test #16760

stevekinney commented Nov 4, 2017 •

edited

Loading

MylesBorins commented Nov 4, 2017

addaleax left a comment

addaleax Nov 4, 2017

MylesBorins Nov 4, 2017

mscdex commented Nov 4, 2017

addaleax commented Nov 4, 2017

cjihrig Nov 4, 2017

MylesBorins commented Nov 4, 2017

MylesBorins left a comment

thefourtheye Nov 5, 2017

addaleax Nov 5, 2017 •

edited

Loading

jasnell Nov 8, 2017

MylesBorins Nov 14, 2017

addaleax Nov 15, 2017

addaleax commented Nov 15, 2017


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

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

test: Add basic WebAssembly test #16760

test: Add basic WebAssembly test #16760

Conversation

stevekinney commented Nov 4, 2017 • edited Loading

Checklist

Affected core subsystem(s)

MylesBorins commented Nov 4, 2017

addaleax left a comment

Choose a reason for hiding this comment

addaleax Nov 4, 2017

Choose a reason for hiding this comment

MylesBorins Nov 4, 2017

Choose a reason for hiding this comment

mscdex commented Nov 4, 2017

addaleax commented Nov 4, 2017

cjihrig Nov 4, 2017

Choose a reason for hiding this comment

MylesBorins commented Nov 4, 2017

MylesBorins left a comment

Choose a reason for hiding this comment

thefourtheye Nov 5, 2017

Choose a reason for hiding this comment

addaleax Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell Nov 8, 2017

Choose a reason for hiding this comment

MylesBorins Nov 14, 2017

Choose a reason for hiding this comment

addaleax Nov 15, 2017

Choose a reason for hiding this comment

addaleax commented Nov 15, 2017

stevekinney commented Nov 4, 2017 •

edited

Loading

addaleax Nov 5, 2017 •

edited

Loading