test: Made unit test passing with node-chakracore

[kunalspathak]

There were 3 unit test that were failing with node-chakracore because of error message difference between v8 and chakracore. Added logic to generate error on fly and use that to compare the error message.

For should parse deep object test case, chakracore throws Out of stack for size 500 depth. v8 throws after 800 limit and the variation is because different size for stack probing. Restricted the depth to 250 which should be good enough to validate the depth and not throw.

Ref: nodejs/node-chakracore#189

[dougwilson]

Thanks @kunalspathak ! It looks like we cannot support Chakracore with this module if our deep parsing test does not pass, sorry!

[dougwilson]

The value 500 has already been promised to users. We can evaluate if Chakracore is a viable platform for us to use in the next major version, through, but in order to even do that, we need to test against Chakracore on Travis CI as a prerequisite, if you have any information on how we can set that up.

[dougwilson]

Sorry, didn't mean to close, but need to determine what the reason for the loss of supported depth is and how we can get 500 supported on Chakracore before I can merge this.

[dougwilson]

P.S. I flagged this PR as "needs tests" since there are no tests for this--i.e. if I revert the code after merging the CI will not flag any failures, so we have no way to keep these working down the road after this first merge.

[kunalspathak]

CI will not flag any failures,

This should happen automatically once we start having CI for node-chakracore.

how we can get 500 supported on Chakracore

I didn't spend much time on why we were getting stack overflow. The call stack came from some node_module 'qs'. I will take a look more closely tomorrow an update this thread.`

The value 500 has already been promised to users.

Just curious, could you point me to the doc that calls that out?

if you have any information on how we can set that up.

As mentioned in expressjs/express#3251 (comment) , if you could point me to your existing Travis CI scripts, I can take a look to see how to support node-chakracore.

[dougwilson]

This should happen automatically once we start having CI for node-chakracore.

Oh, would that show up in the PRs on this repo ? (i.e, the status API on GitHub)

[kunalspathak]

Oh, would that show up in the PRs on this repo ? (i.e, the status API on GitHub)

It should, and it can be configured in the setup scripts. I will dig a bit more and let you know.

[dougwilson]

Sweet! That would just be something we add to the .travis.yml file or something? Please feel free to update the .travis.yml file in the PR directly, or make another PR, whichever. Or if you have instructions I can do the setup as well.

[kunalspathak]

Regarding Out of stack space error, below is the call stack where it starts calling recursively inside qs module util.js (Chrome devtools truncates the path so you won't be able to see it clearly in below stack) and reaches the end of stack space. I suppose the code inside qs can be optimized to have the calls iterative instead of recursive, but I won't go there for now. ECMA specs doesn't specify when an engine should throw stackoverflow. Stack space management and size is clearly left on the engine implementation and I don't think it is good idea to take dependency on it to set limit. I am sure with x86 or arm this would throw stackoverflow before 500 limit.

To summarize, it is not a chakracore bug 😄

[dougwilson]

Gotcha, awesome 👍 it sounds like it's just that our dependency doesn't yet support chakracore, then? Probably would need our major dependencies like qs and iconv-lite supporting chakracore before we can realistically be able to do so.

[kunalspathak]

Probably would need our major dependencies like qs and iconv-lite supporting chakracore

I verified running unit test for qs and iconv-lite with node-chakracore and they pass. So its just nature of unit test in body-parser which calls qs because of which we are seeing OOS.

[dougwilson]

Ok. I mean, really the main blocker to accepting this is that I need instructions (or you can update the PR) on getting Chakracore running on our CI, so that we can actually test these changes.

[dougwilson]

Needs CI validation of changes.

[kunalspathak]

Sure. I will work on that in next few days.

[kunalspathak]

@dougwilson - Can you follow these instructions to enable node-chakracore testing in CI?

[kunalspathak]

@jasongin - FYI

[kunalspathak]

Travis is failing because of same problem as mentioned here. Can you please take a look and suggest what is needed?

[dougwilson]

@kunalspathak I don't see anything obvious from looking at the output, just that the npm command cannot find the mocha binary for some reason. It could be some kind of conflict from running both nvm (from Travis CI) and now nvs (added in this PR) and so maybe there are two different npms being used in there, not sure, that is just speculation.

[jasongin]

The mocha not-found issue is because the CI script needs to include an explicit call to npm install. Travis CI would do an npm install automatically when using the built-in Node.js support via nvm, but not when using nvs instead.

However, @kunalspathak was referring to a different issue, about the Node-ChakraCore install failing due to the GitHub API rate-limiting, as described at https://github.com/jasongin/nvs/blob/master/doc/CI.md#github-api-tokens

[kunalspathak]

In the recent travis run, chakracore failed because of rate limit and node/0.7 failed because it is not supported by nvs. @jasongin , any technical blocker to support old versions of node?

[dougwilson]

Is testing against node/0.7 necessary? It wasn't being test before, but I see node/0.8 is missing, so it seems like that was a typo.

[kunalspathak]

You are right, it was a typo, but still nvs will complain for it.

[jasongin]

any technical blocker to support old versions of node?

Not that I know of. I just never tried because hardly anyone still uses node < 0.10.

[kunalspathak]

So yes, the eval case of depd doesn't work with node-chakracore. However I re-ran the scripts on my local machine that .travis runs and it runs fine with node-chakracore. We can try again after your npm 5 fix because I think that is the reason it is failing for both node 8.0 and node-chakracore.

[dougwilson]

Oh, sorry, I was away and you mentioned above the issue was with depd so that made me think you were saying the issues I found. The main issue I looked back up in my notes now that I'm at a computer is the TypeError: Object doesn't support property or method 'getThis' that is triggered within depd under ChakraCore, not the eval thing that I was recalling above (the eval just shows a less helpful stack trace, while this causes an thrown error).

If those are allowed to fail, they would appear in an entirely different section in the Travis CI build list, but they aren't. My only assumption is that it's maybe because Travis CI doesn't support doing this though environment variables? Not sure, because I see you listed them, but Travis CI doesn't agree. Perhaps you have to list the entire build environment to work? I.e. you may need to add the NVS_VERSION=1.4.1 to each allowed fail line gosh, this is going to be hard to maintain properly... why can't we use the built-in Travis CI way to declare Node.js versions, again?).

[kunalspathak]

@dougwilson - I have switched back to use nvm instead of nvs and now things are working as expected. node8.0 and node-chakracore are failing because of depd but they are not failing the build. I assume this PR will land once npm 5 work is done.

[dougwilson]

That's correct, which I am working on landing now, to merge this tonight.

[dougwilson]

Looks like this is no longer a needed change with current ChakraCore (or, at least with current master and ChakraCore).

[dougwilson]

Nevermind... I didn't think the error would be SyntaxError: JSON.parse Error: Invalid character at position:1. I wonder what is going on here...

[dougwilson]

Ah, Out of stack space on the server, like you said up top, haha.

Testing to see if the review is preventing PR from running through CI.

[dougwilson]

Sorry if my pushes are causing noise; the CI wasn't running the PR because of merge conflict, so I rebase on top of current master (which has the npm 5 support), but now trying to figure out why chakra core is not running.

[kunalspathak]

I will take a look tomorrow.

[kunalspathak]

@dougwilson - Looks like the problem was that node-chakracore version specified in .travis was 8.1.2 which is lesser than the last specified node-v8 version in the matrix i.e. 8.2 and may be it thinks that there is no need to install (and test) older version. I bumped up the version of node-chakracore to 8.2.1 and it is kicking off the test and passing.

[dougwilson]

So I just rebased your PR to incorporate the JSON changes that are in master, leaving the only issue the Qs module problem where it stack overflows during it's compact stage. I'm working to see if I can just PR them a fix rather than change the test here -- especially because this very test found a giant performance regression in Node.js that they fixed before releasing :O !

Anyway, the current ChakraCore is failing on npm prune operation in Travis CI:

/home/travis/.travis/job_stages: line 57:  3758 Segmentation fault      (core dumped) npm prune

[dougwilson]

PR made to qs: ljharb/qs#224

So we basically have the following on the checklist:

(1) qs gets a fix to not blow the stack of deep objects
(2) npm prune stops segfaulting

[ljharb]

qs fix released in v6.5.1

[dougwilson]

Thanks for the quick turn-around @ljharb ! @kunalspathak I rebased your PR here on top of the current master now and ChakraCore passes on all the tests now without any modification. The npm prune command on ChakraCore is still failing due the segmentation fault, though.

[kunalspathak]

Thanks. I will take a look.

[dougwilson]

The underlying changes in this PR have been adapted, but with node-chakracore being archived, it doesn't make sense to land the last bit, which is the CI testing.