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

Travis CI: Add parallel test runs on macOS and Windows #1800

Closed

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Jun 25, 2019

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Travis CI: Add parallel test runs on macOS and Windows

Windows and Python 3 run in allow_failures mode until we are ready to support them.

.travis.yml Outdated
before_install: choco install python2
allow_failures:
- os: windows
- python: 3.7
Copy link
Member

Choose a reason for hiding this comment

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

are we expecting failures on windows now that #1793 is landed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python 3 yes but Windows no. See comments below.

@rvagg
Copy link
Member

rvagg commented Jun 26, 2019

both of these commits should probably be prefixed with test:

Please stress test Python 3.

Note that the Windows run is still in __allow_failures__ mode but that should be fixed in a separate PR.
@rvagg
Copy link
Member

rvagg commented Jun 26, 2019

Oh, and here's another thing, avoid merge commits, they mess up the sequential flow of commits on a branch and make the history hard to traverse. Only use git merge when you know that you're merging a branch that you know is up to date. If you end up being asked to write a commit message for git merge then you've done it wrong.

So what I do in cases like this goes something like this:

git checkout master
git pull origin master
git checkout my-local-pull-request-branch
git rebase master
git push origin my-local-pull-request-branch --force

and you can shorten that a bit and avoid the master checkout while staying on your local pull request's branch:

git fetch origin
git rebase origin/master
git push origin my-local-pull-request-branch --force

Fixing this branch might be a bit awkward now. What I'd probably do to land it is cherry-pick the first two pre-merge commits onto a fresh branch, then cherry-pick the post-merge one, fixing any conflicts manually.

If you're happy with this I could probably do all that and land it if you'd like.
There's failures on Windows but I don't suppose it's the fault of this PR: https://travis-ci.com/nodejs/node-gyp/jobs/211035846

@cclauss
Copy link
Contributor Author

cclauss commented Jun 26, 2019

Understood. My git skills are not all that they should be so I often rely on GitHub web UI which I will stop doing on Nodejs projects. I use the cheatsheet for CPython that has a similar approach to yours: https://devguide.python.org/gitbootcamp/#syncing-with-upstream

Please go ahead and do the manipulations to land this one when you are OK with it. The failure on Windows can be addressed in future PRs.

rvagg pushed a commit that referenced this pull request Jun 26, 2019
Note that the Windows run is in __allow_failures__ mode but that should be
fixed in a separate PR.

PR-URL: #1800
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Jun 26, 2019

squashed into one commit and landed into b93bac9

@rvagg rvagg closed this Jun 26, 2019
@rvagg
Copy link
Member

rvagg commented Jun 26, 2019

sorry, 7a9a038

rvagg pushed a commit that referenced this pull request Jun 26, 2019
Note that the Windows run is in __allow_failures__ mode but that should be
fixed in a separate PR.

PR-URL: #1800
Reviewed-By: Rod Vagg <rod@vagg.org>
@cclauss cclauss deleted the Travis-CI-add-macOS-and-Windows branch June 26, 2019 08:52
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.

2 participants