unit test issues

[ColinEberhardt]

I've been digging into the code for this project, with a view to contributing. The first thing I did was clone and run the tests, unfortunately ~60 are failing for me - looks like all the tests that assert errors are thrown.

I've not quite got to the bottom of it, but have found a few issues:

1. From looking at the API docs, the second argument in assert.throws should be a regex, not a string, as currently used in the tests. If you provide a string as the second argument, it will assert that an error is thrown, but not validate the message.
2. If you extend Error, additional logic is required in order to propagate the message property as detailed in this StackOverflow question

It still doesn't explain why the tests fail for me but pass on Travis, however, I thought it was worth raising these.

One another note, do you have any objections to replacing your Makefile with npm run scripts? It is a more standard tooling for JS projects, and also allows you to do things like pass args to scripts, e.g.

npm test -- --inspect --debug-brk 

This runs the tests with the debugger, allowing you to add breakpoints etc ...

[xtuc]

Thanks for the report, I'm happy to hear to you want to contribute!

1. I remember having a doubt about how to use assert.throws correctly, you are right.

Could you please provide your execution log? Are the error not matching or are the error not thrown?

2. yes, we have custom errors which extends the built-in Error: https://github.com/xtuc/js-webassembly-interpreter/blob/9d61c84c1f8daf9a55a758ae05f9cd820825342b/src/errors.js#L3-L5

I basically have the same environment than Travis, could you provide some information about your environment?

do you have any objections to replacing your Makefile with npm run scripts?

Unfortunately I love Makefiles and don't like npm scripts 😄 (Babel is also using a Makefile).
Note that you can pass arguments, like the following example:

make MOCHA_OPTS="--inspect --debug-brk" test

It will end up with ./node_modules/.bin/mocha --reporter=tap --inspect --debug-brk --recursive

[ColinEberhardt]

I remember having a doubt about how to use assert.throws correctly, you are right.

Actually, my mistake, I spotted that you are using chai, which has a slightly different assert signature, where the string matches the message property of the thrown error.

(Having said that, the chai API docs are quite misleading and don't actually cover this case properly).

Anyhow, I've done a bit more digging. The reason it is failing for me is due to the babel transform of the RuntimeError class. I've created a simple repro here:

https://github.com/ColinEberhardt/js-webassembly-interpreter-issue-repro

When this class is transformed, it adds a call to _wrapNativeSuper in the output:

https://github.com/ColinEberhardt/js-webassembly-interpreter-issue-repro/blob/master/lib/error.js#L38

The wrapper that this adds prevents the message property from being 'inherited'. If I remove the wrapper, RuntimeError works as expected, and my simple test passes.

I'd be interested to know whether your transpiled output includes _wrapNativeSuper? I'm not sure whether this is a bug in babel, or expected behaviour. Although it doesn't explain why we see different results.

Unfortunately I love Makefiles and don't like npm scripts

Ha ha, fair enough :-) My only concern with makefiles is they do make it a bit harder for Windows users. Although that's not a big deal. Thanks for the tip on arg passing.

[ColinEberhardt]

This looks like a similar issue to the one I am experiencing:

babel/babel#4390

And the addition of this plugin, as referenced in the issue, fixes it for me:

https://www.npmjs.com/package/babel-plugin-transform-builtin-extend

[xtuc]

I don't have enough time right now but, I will fix the assertion for you later. The chai API is definitely mislead, I can try to clarify/replace it. Did you manage to pass the tests on your local machine?

The reason it is failing for me is due to the babel transform of the RuntimeError class

Ah yes, that's actually a know issue that we have fix recently. babel/babel#7020

[ColinEberhardt]

Aha ... that was the final clue I needed!

I initially used yarn rather than npm to install dependencies, which ignored the repositories package-lock.json file. removing node_modules and re-installing via npm fixed the issue, the unit tests now pass :-)

That was a bit of a long journey ;-)

[xtuc]

Good news.

Btw we are already using the version of Babel that supports the native extend.

[ColinEberhardt]

At least something good came of this - I've updated the Chai docs to be clearer about the assert.throws method chaijs/chai#1113

[xtuc]

Nice! Thanks for the clarification.