Invalid JavaScript tests for multi-value functions

[thibaudmichaud]

At the WebAssembly/JavaScript boundary, multi-values are converted to JS Arrays according to the spec text. This is not taken into account in the generated JS tests, where assert_return still expects exactly two scalar arguments. See test/core/_output/if.js:167 for example:

assert_return(() => call($1, "multi", [0]), 9, -1)

The JS Array [9, -1] is the expected result of the WebAssembly function, but assert_return will discard its third argument and compare the array with 9.

[binji]

Thanks for this bug report! I see the same issue. I was able to fix this with a relatively simple change to the assert_return function:

-function assert_return(action, expected) {
+function assert_return(action, ...expected) {
   let actual = action();
-  if (!Object.is(actual, expected)) {
-    throw new Error("Wasm return value " + expected + " expected, got " + actual);
-  };
+  if (actual === undefined) {
+    actual = [];
+  } else if (!Array.isArray(actual)) {
+    actual = [actual];
+  }
+  if (actual.length !== expected.length) {
+    throw new Error(expected.length + " value(s) expected, got " + actual.length);
+  }
+  for (let i = 0; i < actual.length; ++i) {
+    if (!Object.is(actual[i], expected[i])) {
+      throw new Error("Wasm return value " + expected[i] + " expected, got " + actual[i]);
+    };
+  }
 }

One concern is that the multi-value repo does not include the upstream changes to assert_return that combines the NaN checking too (see WebAssembly/spec#1104). At the same time, there is a pending merge from the multi-value repo back to the upstream spec (see WebAssembly/spec#1145).

It's tempting to merge that PR as-is, and fix on head. But I suppose the right way to do it is to merge from spec to multi-value, fix the bug here, then update the PR with this fix.

[Ms2ger]

Fixed in #45.