Skip to content
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

add electron to travis test matrix #848

Merged
merged 4 commits into from May 16, 2019

Conversation

@Flarna
Copy link
Member

commented Apr 20, 2019

Seems that #841 got stuck so tried it by myself to add electron to travis test matrix.

@Flarna Flarna force-pushed the Flarna:add_electron branch from 8eb3986 to 38fb2c4 Apr 20, 2019
- if [[ -z "$ELECTRON_VERSION" && $(echo "$TRAVIS_NODE_VERSION < 4" | bc -l) == "1" ]]; then npm install npm@2 && mv node_modules npm && npm/.bin/npm --version && npm/.bin/npm install; else npm --version && npm install; fi
- if [[ -z "$ELECTRON_VERSION" ]]; then node_modules/.bin/node-gyp rebuild --directory test; else node_modules/.bin/node-gyp rebuild --target=v$ELECTRON_VERSION --dist-url=https://atom.io/download/electron --directory test; fi
script:
- if [[ -z "$ELECTRON_VERSION" ]]; then node_modules/.bin/tap --gc test/js/*-test.js; fi

This comment has been minimized.

Copy link
@mkrufky

mkrufky Apr 25, 2019

Collaborator

It looks like this is just a build check, if we're not going to actually run the node.js/nan unit tests.

Personally, I'd love to see electron added to the test matrix, but it would make a lot more sense if we could actually run the tests. I don't know if there's a straight forward way to do this, or if we need to wrap them, maybe even create a separate set of tests for electron.....

This comment has been minimized.

Copy link
@Flarna

Flarna Apr 25, 2019

Author Member

Yes, just build tests.
Honestly speaking I have no idea what's actually needed to run tests with electron but I fear it's not that easy as run nodejs which is available on the CI machines.

Copy link
Member

left a comment

I'd be okay with landing this as-is (well, after a rebase - merge conflict) as a first step to testing electron. Knowing that it builds is already a step up from the status quo.

@Flarna Flarna force-pushed the Flarna:add_electron branch from 38fb2c4 to e7e315f May 6, 2019
@Flarna

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

rebased, added electron 5.0.1 and updated other electron version to the current latest version.
Seems travis is not canceling builds if a new commit is made on the same branch. is this intended?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Oh wait, maybe I misread the diff the first time around. The node-gyp rebuild command at least should run and against electron's headers. Currently that step is skipped too.

@Flarna

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@bnoordhuis if I understand the travis log correct it calls node_modules/.bin/node-gyp rebuild --directory test; else node_modules/.bin/node-gyp rebuild --target=v$ELECTRON_VERSION --dist-url=https://atom.io/download/electron --directory test which should be what we want.

@Flarna

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

and looking into the variant for 5.0.1 it seems there is some more work to do in NAN as there are several deprecation warnings. But not sure if this should be done in this PR.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Sorry, you're right. Travis folds the build output but unfolding didn't show anything yesterday. It's there now though. Okay, objection withdrawn. :-)

@kkoopa

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

Thank you. Hopefully this will help in avoiding regressions.

@kkoopa kkoopa merged commit d105809 into nodejs:master May 16, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@Flarna Flarna deleted the Flarna:add_electron branch May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.