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 Windows CI job (Node 12) #3376

Merged
merged 2 commits into from
Feb 25, 2020
Merged

Add Windows CI job (Node 12) #3376

merged 2 commits into from
Feb 25, 2020

Conversation

cgewecke
Copy link
Collaborator

Description

Part of #3098.

  • Adds a basic check in CI to make sure Web3 installs and runs on Windows 10 / Node 12.
  • Moves other E2E tests up to Node 12
  • Increases the proxy registry timeouts (again) because that continues to be a problem
  • Adds an 'install' script to move some logic which decides whether to install web3's full dependency tree out of travis.yml (because getting too big)

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

  • Test / OS

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success and extended the tests if necessary.
  • I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@coveralls
Copy link

coveralls commented Feb 17, 2020

Coverage Status

Coverage remained the same at 86.047% when pulling 3498b73 on ci/bump-node-12 into f71e6a5 on 1.x.

@cgewecke cgewecke added 1.x 1.0 related issues CI Review Needed Maintainer(s) need to review labels Feb 17, 2020
@nivida nivida requested a review from holgerd77 February 20, 2020 10:30
@holgerd77 holgerd77 requested a review from ryanio February 24, 2020 10:16
Copy link
Collaborator

@ryanio ryanio left a 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 :)

@cgewecke
Copy link
Collaborator Author

cgewecke commented Feb 24, 2020

@ryanio Thanks!

I’ve had a lot of success migrating some of the EthereumJS repos over to Github Actions

👍

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.

@holgerd77
Copy link
Collaborator

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)?

@cgewecke
Copy link
Collaborator Author

@holgerd77 @ryanio Rebased, just pinging for a double-check.

@holgerd77 holgerd77 merged commit ccc229e into 1.x Feb 25, 2020
@holgerd77 holgerd77 deleted the ci/bump-node-12 branch February 25, 2020 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Review Needed Maintainer(s) need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants