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

Fix windows CI & lengthen verdaccio timeouts #3421

Merged
merged 7 commits into from Mar 25, 2020
Merged

Fix windows CI & lengthen verdaccio timeouts #3421

merged 7 commits into from Mar 25, 2020

Conversation

@cgewecke
Copy link
Member

cgewecke commented Mar 18, 2020

Description

Some CI tuning for the "allowed failure" jobs that stems from Ryan's work on Github Actions.

  • kills a zombie Node process that was making the windows CI hang
  • increases verdaccio's http-agent timeouts (should make those jobs more stable)

Type of change

  • CI improvement

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.
  • I have executed npm run test:cov and my test cases do cover all lines and branches of the added code.
  • 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

This comment has been minimized.

Copy link

coveralls commented Mar 18, 2020

Coverage Status

Coverage remained the same at 86.047% when pulling b92cf84 on ci/tuning into dee72e0 on 1.x.

Copy link
Collaborator

ryanio left a comment

Looks good.

Did you want to add a few more zeroes to keepAliveMsecs?

Did you by any chance try if other values like max_timeout, max_fail, or server: keepAliveTimeout would additionally help at all? https://github.com/ryanio/web3.js/blob/c9e9c3051e88f0ad07a81d9f6c78fcb488a19b85/verdaccio.yml

@cgewecke

This comment has been minimized.

Copy link
Member Author

cgewecke commented Mar 18, 2020

@ryanio Ah thanks. Looking at the logs you ran, my impression was that the other timeouts (e.g 10m) weren't getting hit. Is anything waiting 10 minutes in those jobs?

And e2e_ganache (which usually passes on Travis) is timing out on GH actions, unless the http-agent timeouts are set wide. Maybe that setting is the decisive one?

Tbh I don't really have any idea what's going on 🙂 Making all the settings long as you suggest is probably safest.

Did you want to add a few more zeroes to keepAliveMsecs?

Yes!

What is your opinion about leaving the allowed failures in Travis and using GH Actions for the real jobs? Only reason I suggest this is that Travis' UI seems slightly better for that use-case - you can easily see if the job failed but the run as a whole passes.

@ryanio

This comment has been minimized.

Copy link
Collaborator

ryanio commented Mar 18, 2020

Is anything waiting 10 minutes in those jobs?

In my runs it would randomly time out on me when downloading deps, so I bumped everything to 10mins to play it safe, but I never saw it wait that long. It would usually just immediately fail on retries up to the max_fail limit. Seemed to fail at a different dep every time so it was confusing indeed, although I think it did succeed a few times.

What is your opinion about leaving the allowed failures in Travis and using GH Actions for the real jobs? Only reason I suggest this is that Travis' UI seems slightly better for that use-case - you can easily see if the job failed but the run as a whole passes.

Sure that seems nice, however unfortunately last I heard the ethereum org still doesn't have access to GH Actions as a feature so it might not be possible to actually switch :(

@ryanio
ryanio approved these changes Mar 18, 2020
@cgewecke

This comment has been minimized.

Copy link
Member Author

cgewecke commented Mar 18, 2020

@ryanio

Sure that seems nice, however unfortunately last I heard the ethereum org still doesn't have access to GH Actions as a feature so it might not be possible to actually switch :(

Oh ok. I've ported over a couple improvements you made to eth.accounts.wallet to make those less flaky too then.

@ryanio
ryanio approved these changes Mar 18, 2020
Copy link
Collaborator

ryanio left a comment

Great, LGTM :)

# Debugging...
ps -ef
# Terminate stray node process
kill -9 $(grep node <(ps -ef) | sed 's/.*\travis *\([0-9]*\).*/\1/')

This comment has been minimized.

Copy link
@holgerd77

holgerd77 Mar 20, 2020

Collaborator

Hmm, these kind of things seem pretty dangerous to me if run in a local environment. E.g. I tested on my machine and the non-kill part returned a totally unrelated "Atom Helper" process having something with node in the passed CLI parameters.

Is there any way to make this more robust to avoid side effects like this?

This comment has been minimized.

Copy link
@cgewecke

cgewecke Mar 20, 2020

Author Member

@holgerd77

Oh yes...do you think checking that the CI environment variable is set before running this would be enough?

Here's a list of env variables set in TravisCI

This comment has been minimized.

Copy link
@cgewecke

cgewecke Mar 25, 2020

Author Member

Added a check for CI in 2f1ebc5

cgewecke added 2 commits Mar 23, 2020
@cgewecke cgewecke mentioned this pull request Mar 25, 2020
8 of 13 tasks complete
Copy link
Collaborator

holgerd77 left a comment

LGMT now.

@cgewecke cgewecke merged commit 9394a4d into 1.x Mar 25, 2020
3 checks passed
3 checks passed
bundlesize ./packages/web3/dist/web3.min.js: 1.19MB < maxSize 1.5MB (no compression)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@cgewecke cgewecke deleted the ci/tuning branch Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.