-
Notifications
You must be signed in to change notification settings - Fork 305
Add and test Node 8 support #518
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
Conversation
|
@RubenVerborgh Nice job tracking down these issues! I wonder if we should wait until Nock works with node 8 before merging this... (I don't suppose Travis allows you to execute different scripts for different node versions?) |
|
I personally wouldn't wait for Nock; I find it very strange that they haven't tackled their issues already. It seems more important to me that we do test for Node 8 (whereas Nock currently doesn't) to avoid breaking things. It would be possible to have a switch statement per Node version. However, given that we know the proxy tests work on v6, and given that these tests only depend on the proxy and nothing else, I don't see a problem. If somebody wants to work on the proxy in the future, they will likely look at the proxy tests anyway. |
|
Nock has been updated, so I can update this PR. |
Version 9.0.14 brings Node 8 compatibility.
|
@dmitrizagidulin Rebased to the latest |
|
@RubenVerborgh ok, it's ready to go on your end? |
|
Totally! |
The server tests now run with Node 8, and this is tested on Travis.
The main problem was that setting
NODE_TLS_REJECT_UNAUTHORIZEDat runtime throughprocess.envdidn't work anymore. I have replaced this by therejectUnauthorizedconfiguration option where possible, which fixed the ACL TLS test (and can be useful for local testing). Unfortunately, the supertest library does not allow to set that option, so I set is a an environment variable before calling the Mocha tests.The remaining problem is that Nock is currently incompatible with Node 8, so for now, I have just skipped those 4 tests that use Nock.