-
Notifications
You must be signed in to change notification settings - Fork 5k
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 Windows CI job (Node 12) #3376
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cgewecke, @holgerd77 asked if I could do a review here so I’m happy to help.
Overall the PR looks great and I like how the external bash scripts organize the conditional logic well 👍 easy to understand and parse.
I very much like the verdaccio solution for e2e testing by swapping web3.js in other libs for the CI built version! Neat idea and seems really powerful for catching bugs.
The new Windows job has to go in as an "allowed failure" because even though it succeeds, the Travis' Windows container hangs open without completing.
OK great, yeah it looks like it's running fine since it exited with status 0 at the end of the basic_usage test.
Over the past few months I’ve had a lot of success migrating some of the EthereumJS repos over to Github Actions. I found it quite a bit faster than travis or circleci and required fewer hacky workarounds, at least for our simpler requirements. I’m unsure of how strong gh actions Windows support is, but perhaps it’s worth a look in the future. The current travis config looks a bit sophisticated though so I understand if it’s not a priority for now :)
@ryanio Thanks!
👍 Migrating might mostly be a matter of transferring the Travis matrix / ci.sh setup to a github.yml - could be pretty straightforward. Note to self: "allowed failure" is ~ called continue-on-error in Github actions. |
Branch is very much behind on this PR, wouldn't dare to use the "Update branch" functionality. @cgewecke can you rebase here (will likely need re-approvals then)? |
0479ce3
to
afc6aeb
Compare
@holgerd77 @ryanio Rebased, just pinging for a double-check. |
Description
Part of #3098.
The new Windows job has to go in as an "allowed failure" because even though it succeeds, the Travis' Windows container hangs open without completing.
This is a known problem there - their Windows is in beta. It has various fixes, none of which seemed to help in Web3's case. Tried to isolate the cause of the hang by closing all handles including virtual registry and eliminating the Node tests, etc but couldn't determine the source. There are also notes in the Travis issues suggesting that they should be clearing up resources on their end...idk.
In summary: this test can be used as a pre-publication check and it's a convenient way of evaluating reported Windows installation issues.
Related to #3371, #3051, #3033, #3002 etc...
Type of change
Checklist:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:unit
with success and extended the tests if necessary.npm run build-all
and tested the resulting file/'s fromdist
folder in a browser.CHANGELOG.md
file in the root folder.