Fix error messages

[vweevers]

Addresses #344 (comment) and #344 (comment).

[vweevers]

Side note: test/parallel/test-stream-pipeline.js is failing with OOM errors on Windows with node 6.

Edit: #353

[vweevers]

Test failure is unrelated (it's the travis+npm timeout issue).

[mcollina]

Amazing work! Would you mind adding some sort of unit test under test/ours? So we can verify those messages are the same? I fear they can get out of date real quick, and the Node.js unit tests do not verify them.

cc @BridgeAR

[vweevers]

Would you mind adding some sort of unit test under test/ours?

OK. Should I use tap, tape or assert? The test suite is a mixture of these.

[BridgeAR]

LGTM. Good work and thanks a lot for this!

[BridgeAR]

@mcollina thinking about the changes: as long as they initially are aligned it should be fine. Testing these could only be done properly in Node core itself and at least adding tests for ERR_INVALID_ARG_TYPE would be redundant in core as that error is tested very often, including the error message.

[mcollina]

LGTM

[vweevers]

Testing these could only be done properly in Node core itself and at least adding tests for ERR_INVALID_ARG_TYPE would be redundant in core

I already wrote tests, before I read this. So let me know if I should revert the commit. IMO it can't hurt to have tests here, because the code is not 100% equal to core, we have ponyfills for String.prototype.includes etc.

[BridgeAR]

@vweevers all stream related Node core tests run against this module as well and that is how the tests should be tested. Having these tests here mean both sides could still diverge without noticing it. They probably do not hurt either tough.

To me it's your call if you keep them or remove them, even tough I would remove them.

[vweevers]

Having these tests here mean both sides could still diverge without noticing it.

I see what you mean.

@mcollina your call :)

[mcollina]

Still LGTM