New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Made unit test passing with node-chakracore #3251

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kunalspathak
Contributor

kunalspathak commented Mar 21, 2017

There were 4 unit test that were failing with node-chakracore because of error message difference between v8 and chakracore. Added engine specific check to validate the expected error message.

Ref: nodejs/node-chakracore#189

@dougwilson

Based on what the tests are actually trying to assert (that the error was passed to the callback), I don't think it's necessary at all to be validating the specific error message back. I'm sure there would be a way to just make a single conditional work on v8 and chakracore without checking the engine.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Mar 21, 2017

Member

In addition to my comment above, it would be awesome (this is tangental; so really a different PR) if we could add chakracore to Travis CI so we can keep it green for chakra.

Member

dougwilson commented Mar 21, 2017

In addition to my comment above, it would be awesome (this is tangental; so really a different PR) if we could add chakracore to Travis CI so we can keep it green for chakra.

@dougwilson dougwilson self-assigned this Mar 21, 2017

@@ -100,7 +101,10 @@ describe('app', function(){
app.render('user.tmpl', function (err, str) {
// nextTick to prevent cyclic
process.nextTick(function(){
err.message.should.match(/Cannot read property '[^']+' of undefined/);
err.message.should.match(jsEngine.engineSpecificMessage({

This comment has been minimized.

@dougwilson

dougwilson Mar 21, 2017

Member

This could probably just be assert.ok(err)

@dougwilson

dougwilson Mar 21, 2017

Member

This could probably just be assert.ok(err)

@@ -110,7 +111,10 @@ describe('res', function(){
request(app)
.get('/')
.expect(/Cannot read property '[^']+' of undefined/, done);
.expect(jsEngine.engineSpecificMessage({

This comment has been minimized.

@dougwilson

dougwilson Mar 21, 2017

Member

I would just replace the res.end(err.message) above with res.send('caught error: ' + err.message') and then the expect could just be .expect(/^caught error: /, done) since that would tell us we got the error in next(err)

@dougwilson

dougwilson Mar 21, 2017

Member

I would just replace the res.end(err.message) above with res.send('caught error: ' + err.message') and then the expect could just be .expect(/^caught error: /, done) since that would tell us we got the error in next(err)

@@ -335,7 +339,10 @@ describe('res', function(){
request(app)
.get('/')
.expect(/Cannot read property '[^']+' of undefined/, done);
.expect(jsEngine.engineSpecificMessage({

This comment has been minimized.

@dougwilson

dougwilson Mar 21, 2017

Member

This could just do the same thing the above did.

@dougwilson

dougwilson Mar 21, 2017

Member

This could just do the same thing the above did.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Mar 21, 2017

Contributor

Thanks @dougwilson for the quick feedback. I was hesitant in adding engineSpecificMessage() but I didn't want to change the meaning of unit test and not much aware on how important those validations are. If you feel that checking for error message is not that important, I can go ahead with fixing as you suggested.

Contributor

kunalspathak commented Mar 21, 2017

Thanks @dougwilson for the quick feedback. I was hesitant in adding engineSpecificMessage() but I didn't want to change the meaning of unit test and not much aware on how important those validations are. If you feel that checking for error message is not that important, I can go ahead with fixing as you suggested.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Mar 21, 2017

Contributor

if we could add chakracore to Travis CI so we can keep it green for chakra.

Is this the right repo to open issue for CI for node-chakracore?

Contributor

kunalspathak commented Mar 21, 2017

if we could add chakracore to Travis CI so we can keep it green for chakra.

Is this the right repo to open issue for CI for node-chakracore?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Mar 22, 2017

Member

If you feel that checking for error message is not that important, I can go ahead with fixing as you suggested.

In these three cases, the tests do not actually care what the message is, as can be seen in the test descriptions.

Is this the right repo to open issue for CI for node-chakracore?

I'm not sure what you mean. Can you elaborate?

Member

dougwilson commented Mar 22, 2017

If you feel that checking for error message is not that important, I can go ahead with fixing as you suggested.

In these three cases, the tests do not actually care what the message is, as can be seen in the test descriptions.

Is this the right repo to open issue for CI for node-chakracore?

I'm not sure what you mean. Can you elaborate?

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Mar 22, 2017

Contributor

I'm not sure what you mean. Can you elaborate?

I mean, is there a place where we can track this work item? How is your existing Travis CI for node-v8 set up? I can take a look into scripts, if you can point me to it. But if it is just as simple as downloading node binaries from nodejs.org then you can get node-chakracore binaries from here.

Contributor

kunalspathak commented Mar 22, 2017

I'm not sure what you mean. Can you elaborate?

I mean, is there a place where we can track this work item? How is your existing Travis CI for node-v8 set up? I can take a look into scripts, if you can point me to it. But if it is just as simple as downloading node binaries from nodejs.org then you can get node-chakracore binaries from here.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Mar 22, 2017

Member

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. Ideally if chakracore is a supported platform (which I see no reason not to get it there), but Express.js is made up of a lot of dependencies, a lot of which have entire test suites not represented here, so prior to marking Express.js as supporting chakracore, ideally at least all the Express.js project dependencies below it have their test suites running in chakracore as well so we can keep everything functioning in an ongoing manner.

I mean, is there a place where we can track this work item?

Just make a PR with the setup or provide we the instructions for how any project can test against chakracore in Travis CI and I can help get it setup.

How is your existing Travis CI for node-v8 set up? I can take a look into scripts, if you can point me to it.

We just use the standard Node.js setup: https://docs.travis-ci.com/user/languages/javascript-with-nodejs/

You can find our .travis.yml file in the root of the project: https://github.com/expressjs/express/blob/master/.travis.yml

Ideally we need to test on Windows as well (Travis CI is the Linux test platform). We use AppVeyor for that (and file is here: https://github.com/expressjs/express/blob/master/appveyor.yml)

But if it is just as simple as downloading node binaries from nodejs.org then you can get node-chakracore binaries from here.

I'm not familiar with how to do that in Travis CI / AppVeyor. Are there any instructions for how to do this? How do other Node.js projects test against chakracore?

Member

dougwilson commented Mar 22, 2017

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. Ideally if chakracore is a supported platform (which I see no reason not to get it there), but Express.js is made up of a lot of dependencies, a lot of which have entire test suites not represented here, so prior to marking Express.js as supporting chakracore, ideally at least all the Express.js project dependencies below it have their test suites running in chakracore as well so we can keep everything functioning in an ongoing manner.

I mean, is there a place where we can track this work item?

Just make a PR with the setup or provide we the instructions for how any project can test against chakracore in Travis CI and I can help get it setup.

How is your existing Travis CI for node-v8 set up? I can take a look into scripts, if you can point me to it.

We just use the standard Node.js setup: https://docs.travis-ci.com/user/languages/javascript-with-nodejs/

You can find our .travis.yml file in the root of the project: https://github.com/expressjs/express/blob/master/.travis.yml

Ideally we need to test on Windows as well (Travis CI is the Linux test platform). We use AppVeyor for that (and file is here: https://github.com/expressjs/express/blob/master/appveyor.yml)

But if it is just as simple as downloading node binaries from nodejs.org then you can get node-chakracore binaries from here.

I'm not familiar with how to do that in Travis CI / AppVeyor. Are there any instructions for how to do this? How do other Node.js projects test against chakracore?

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jun 15, 2017

Contributor

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

Contributor

kunalspathak commented Jun 15, 2017

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

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jun 23, 2017

Contributor

Ping, @dougwilson , did you get chance to look into nvs?

Contributor

kunalspathak commented Jun 23, 2017

Ping, @dougwilson , did you get chance to look into nvs?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jun 23, 2017

Member

No, I have not had a chance, though I assume you should be able to update the PR to do that as well, right? Travis CI just uses whatever the contents of .travis.yml is on the given branch.

Member

dougwilson commented Jun 23, 2017

No, I have not had a chance, though I assume you should be able to update the PR to do that as well, right? Travis CI just uses whatever the contents of .travis.yml is on the given branch.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jun 27, 2017

Contributor

@jasongin - FYI.

Contributor

kunalspathak commented Jun 27, 2017

@jasongin - FYI.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jun 27, 2017

Contributor

@dougwilson - I added travis and appveyor support for node-chakracore. The last job (chakracore/latest) in travis is failing because you might want to add github API token to this repo. See this for more details. I am not sure why others are failing and I am not too familiar with travis working. If I understand correctly, all the dependencies should be installed in install step and script section should work, but for some reason it doesn't find some modules. If you get chance, can you please take a look and suggest what is needed?

Contributor

kunalspathak commented Jun 27, 2017

@dougwilson - I added travis and appveyor support for node-chakracore. The last job (chakracore/latest) in travis is failing because you might want to add github API token to this repo. See this for more details. I am not sure why others are failing and I am not too familiar with travis working. If I understand correctly, all the dependencies should be installed in install step and script section should work, but for some reason it doesn't find some modules. If you get chance, can you please take a look and suggest what is needed?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jun 28, 2017

Member

Why download the files from GitHub at all? Looking at https://github.com/Microsoft/ChakraCore/releases it looks like the binaries are available on Azure, which I presume requires no GitHub token.

Member

dougwilson commented Jun 28, 2017

Why download the files from GitHub at all? Looking at https://github.com/Microsoft/ChakraCore/releases it looks like the binaries are available on Azure, which I presume requires no GitHub token.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jun 28, 2017

Contributor

Why download the files from GitHub at all?

@jasongin, any idea?

Contributor

kunalspathak commented Jun 28, 2017

Why download the files from GitHub at all?

@jasongin, any idea?

@jasongin

This comment has been minimized.

Show comment
Hide comment
@jasongin

jasongin Jun 28, 2017

NVS is not downloading ChakraCore releases, it is downloading Node-ChakraCore releases. The latter are not hosted on Azure. Additionally, NVS needs to query the GitHub releases API to find what releases are available and metadata about each release.

jasongin commented Jun 28, 2017

NVS is not downloading ChakraCore releases, it is downloading Node-ChakraCore releases. The latter are not hosted on Azure. Additionally, NVS needs to query the GitHub releases API to find what releases are available and metadata about each release.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jun 28, 2017

Contributor

@dougwilson - I was able to make travis working with only exception for node-chakracore for which you will have to add github API token to the repo.

Contributor

kunalspathak commented Jun 28, 2017

@dougwilson - I was able to make travis working with only exception for node-chakracore for which you will have to add github API token to the repo.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jun 28, 2017

Member

Setting up a GitHub token is not going to work for this project, unfortunately. It would need protection so it needs to be encrypted, but that still has this problem:

From https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml encrypted variables are not available in builds in pull requests, and since pull requests are used even for the project collaborators, it means that it's effectively useless as all PRs will never be able to install ChakraCore.

Member

dougwilson commented Jun 28, 2017

Setting up a GitHub token is not going to work for this project, unfortunately. It would need protection so it needs to be encrypted, but that still has this problem:

From https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml encrypted variables are not available in builds in pull requests, and since pull requests are used even for the project collaborators, it means that it's effectively useless as all PRs will never be able to install ChakraCore.

kunalspathak added a commit to kunalspathak/nvs that referenced this pull request Jul 8, 2017

change default remote for chakracore-release
Now that nodejs.org started hosting chakracore-release, change the default remote
for `chakracore` to https://nodejs.org/download/chakracore-release/.

This will help using nvs for expressjs/express#3251 , expressjs/body-parser#233
and sass/node-sass#1777

kunalspathak added a commit to kunalspathak/nvs that referenced this pull request Jul 8, 2017

change default remote for chakracore-release
Now that nodejs.org started hosting chakracore-release, change the default remote
for `chakracore` to https://nodejs.org/download/chakracore-release/.

This will help using nvs for expressjs/express#3251 , expressjs/body-parser#233
and sass/node-sass#1777

kunalspathak added some commits Jul 11, 2017

Upgrade nvs to 1.4
This supports downloading chakracore release binaries from nodejs.org instead of github
@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jul 11, 2017

Contributor

@dougwilson - should be good to go?

Contributor

kunalspathak commented Jul 11, 2017

@dougwilson - should be good to go?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jul 11, 2017

Member

Nice, great to see it all working now \m/ . Is there a way to pin the chakracore to a given minor version or move it to allowed failure? Just requiring a PR to pass with whatever the bleeding edge is is not good; it caused a lot of heart ache with Node.js, which is why the minors are pinned and any non-pinned thing is allowed to fail.

Member

dougwilson commented Jul 11, 2017

Nice, great to see it all working now \m/ . Is there a way to pin the chakracore to a given minor version or move it to allowed failure? Just requiring a PR to pass with whatever the bleeding edge is is not good; it caused a lot of heart ache with Node.js, which is why the minors are pinned and any non-pinned thing is allowed to fail.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jul 11, 2017

Contributor

The node-chakracore is a stable release that contains released and stabilized version of chakracore. However I understand the concern and we can pin the existing version of node-chakracore. How do you do it?

Contributor

kunalspathak commented Jul 11, 2017

The node-chakracore is a stable release that contains released and stabilized version of chakracore. However I understand the concern and we can pin the existing version of node-chakracore. How do you do it?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jul 11, 2017

Member

We pin them to the minor version.

Member

dougwilson commented Jul 11, 2017

We pin them to the minor version.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jul 11, 2017

Contributor

Sure, but my question was how do you specify what version is pinned? Is it done through .travis.yml and appveyor.yml?

Contributor

kunalspathak commented Jul 11, 2017

Sure, but my question was how do you specify what version is pinned? Is it done through .travis.yml and appveyor.yml?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jul 11, 2017

Member

Sorry, I didn't realize the question at first. Yes, you can see all the minor versions listed in both of those files for Node.js

Member

dougwilson commented Jul 11, 2017

Sorry, I didn't realize the question at first. Yes, you can see all the minor versions listed in both of those files for Node.js

@dougwilson dougwilson removed the needs tests label Jul 11, 2017

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jul 11, 2017

Member

Also, I don't see the Node.js nightly running on Travis CI anymore after this change, if you can look into why that build got dropped from the matrix.

Member

dougwilson commented Jul 11, 2017

Also, I don't see the Node.js nightly running on Travis CI anymore after this change, if you can look into why that build got dropped from the matrix.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jul 11, 2017

Contributor

Also, I don't see the Node.js nightly running on Travis CI anymore after this change

Hhm, i am not sure matrix/include/allow-failures work. I will experiment this a little bit to make sure nightly is picked up.

Contributor

kunalspathak commented Jul 11, 2017

Also, I don't see the Node.js nightly running on Travis CI anymore after this change

Hhm, i am not sure matrix/include/allow-failures work. I will experiment this a little bit to make sure nightly is picked up.

some bug fixes
* Allow nightly to run and allow_failure in travis
* Pin the version of node-chakracore
@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jul 11, 2017

Contributor

@dougwilson - Pinned the chakracore version and made nightly run in travis.

Contributor

kunalspathak commented Jul 11, 2017

@dougwilson - Pinned the chakracore version and made nightly run in travis.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jul 13, 2017

Contributor

Let me know if anything else is needed to merge this PR.

Contributor

kunalspathak commented Jul 13, 2017

Let me know if anything else is needed to merge this PR.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jul 13, 2017

Member

Looks pretty good to me. I did just recently find out from another module that the new npm 5 will incorrectly create a lockfile during the npm prune operation at the top of the cache restore, and will cause issues when any new dependency is added so it won't install. Since this adds npm 5 to be a required passing test, let's get the changes necessary to property restore the modules from the cache when npm 5 is being used merged prior to this (I'll do that). May have to resolve some merge conflicts after landing that change though. Alternatively we can land this with npm 5 versions in the allowed failures list first then the npm 5 support.

Which way do you want to proceed?

Member

dougwilson commented Jul 13, 2017

Looks pretty good to me. I did just recently find out from another module that the new npm 5 will incorrectly create a lockfile during the npm prune operation at the top of the cache restore, and will cause issues when any new dependency is added so it won't install. Since this adds npm 5 to be a required passing test, let's get the changes necessary to property restore the modules from the cache when npm 5 is being used merged prior to this (I'll do that). May have to resolve some merge conflicts after landing that change though. Alternatively we can land this with npm 5 versions in the allowed failures list first then the npm 5 support.

Which way do you want to proceed?

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jul 13, 2017

Contributor

I think if we want to add support for npm 5 in near future, let us add clean support instead of allowing it to fail initially and then cleaning it up. Will you be updating the same PR? I think different PR would make more sense in this case. Thoughts?

Contributor

kunalspathak commented Jul 13, 2017

I think if we want to add support for npm 5 in near future, let us add clean support instead of allowing it to fail initially and then cleaning it up. Will you be updating the same PR? I think different PR would make more sense in this case. Thoughts?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jul 15, 2017

Member

Yep, separate from this PR makes sense to me as well. I'll go ahead and make that today.

Member

dougwilson commented Jul 15, 2017

Yep, separate from this PR makes sense to me as well. I'll go ahead and make that today.

@kunalspathak

This comment has been minimized.

Show comment
Hide comment
@kunalspathak

kunalspathak Jul 17, 2017

Contributor

@dougwilson - Let me know if you need anything else to merge this PR.

Contributor

kunalspathak commented Jul 17, 2017

@dougwilson - Let me know if you need anything else to merge this PR.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jul 17, 2017

Member

I never got to implementing the npm 5 support yet. So really it's just either (1) we are waiting for me to land that first or (2) change the pr to make the npm 5 runs allowed to fail. Since we agreed on option 1, just need to get that done before this can land.

Member

dougwilson commented Jul 17, 2017

I never got to implementing the npm 5 support yet. So really it's just either (1) we are waiting for me to land that first or (2) change the pr to make the npm 5 runs allowed to fail. Since we agreed on option 1, just need to get that done before this can land.

@kunalspathak kunalspathak referenced this pull request Aug 9, 2017

Open

Chakracore support #311

@dougwilson dougwilson referenced this pull request Sep 25, 2017

Merged

Release 4.16 #3423

20 of 20 tasks complete

dougwilson added a commit that referenced this pull request Sep 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment