Skip to content

Conversation

@tiggerite
Copy link
Contributor

Fixes #1680 - tested on Node 10.16.0 and Node 12.6.0

@implausible
Copy link
Member

Hi there! Thanks for working on this! Can you modify the travis.yml and appveyor.yml files to specify tests on node 12?

Copy link
Member

@implausible implausible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this is pretty straight forward and easy to review. I'm excited to get this running in CI to verify your work!

@tiggerite
Copy link
Contributor Author

I've modified both the files but now it's failing, and I'm not entirely sure why, it mentions things that aren't in the generated src folder like BooleanValue..?

@tiggerite
Copy link
Contributor Author

So it was just nan itself being out of date.. I also updated node-pre-gyp for good measure. (There's an updated node-gyp as well but 4.0.0 is quite recent compared to the node-pre-gyp that was being used)

@rcjsuen
Copy link
Member

rcjsuen commented Jul 12, 2019

What a time to be alive...
Thank you for looking into this, @tiggerite!

@tiggerite
Copy link
Contributor Author

All should be good now, no more depreciations that I can see or find. Hopefully this will help get 0.25.0 out of alpha stage - it's already really stable as far as I can tell..

@tiggerite
Copy link
Contributor Author

@implausible is this all good now? It passes all the CI checks (cleanly too.. 😀)

Copy link
Member

@implausible implausible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this! I will get this into the next alpha. Also, we are looking to close out the alpha for 0.25.0 before month is out.

@implausible implausible merged commit 485d675 into nodegit:master Jul 15, 2019
@implausible
Copy link
Member

Oof, this seems to have introduced instability into the test suite. These tests are failing on master and on several unrelated PRs:

https://travis-ci.org/nodegit/nodegit/jobs/559068365

@tiggerite
Copy link
Contributor Author

That’s very strange. It seems to affect three Rebase and two Remote tests. I can’t see why the changes would be causing those to be flaky.

@tiggerite
Copy link
Contributor Author

tiggerite commented Jul 20, 2019

Bit of a long shot but could it be because the enhanced tests still run on Node 8? Maybe try them on 10, the new Nan can’t be optimised (at the very least) for 8?

@implausible
Copy link
Member

Definitely worth considering. I've also re-read this entire PR and also can't determine what did it. I am going to double check that these tests hadn't been failing randomly before this PR as well. I don't recall seeing these tests fail in the past.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Need to implement support for node 12

3 participants