-
Notifications
You must be signed in to change notification settings - Fork 1
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
Test Using Github Actions #1
Conversation
de0d54c
to
e2ee395
Compare
8171c28
to
ed3baef
Compare
Hi @ryanio! This is super cool :) Windows passes which is fantastic. Also looks faster. Historically, only the mosaic_e2e has had intermittent timeout problems - they seem yarn specific. In Travis, you could usually get installation to complete by restarting that specific job, but sometimes it really struggled. Might be network congestion? Have also treated mosaic_e2e and ganache_e2e as "allowed failures" (Travis supports this). They're meant to be conveniences to let you see if changes might be problematic downstream rather than strict requirements. For example, in web3#3190 - the ganache job always errors because those changes flushed out a bug in one of their unit tests... but that shouldn't block the PR. Conversely in web3#3378, the mosaic test failure shows that the changes break an environment which injects an One approach might be to move those two runs into their own "job" and set continue-on-error: true, allowing them to pass even when there are problems. And perhaps we could modify the Github PR template to have a reminder in the checklist that those two jobs can/should be manually checked. Another possibility is to leave them in Travis as allowed failures and have two CI providers. That might be the simplest thing, idk. |
f57a286
to
e9d4657
Compare
@cgewecke gotcha, thanks for the helpful additional info! Looks like an easy solution will be to use: - run: bash ./scripts/ci.sh
continue-on-error: ${{ matrix.testCmd == 'e2e_ganache' || matrix.testCmd == 'e2e_mosiac' }} And with that... everything is passing! 😯🎉 One side question, is there a prettier config for web3.js? For example in EthereumJS repos we have |
Yes! That's awesome. Interestingly, the ganache job (npm) is timing out now and I don't think I've seen that before...it was always yarn before. How do you re-run a job in Github actions? I don't see retry button in UI on my side, is it there for you?
No there isn't - the project is effectively unlinted - the only rule is Not sure how difficult it is to disable it in the text editor but yes the diffs here get really mangled if it's done on automatically on save. We've talked about linting the project in a single sweep or set of PRs that don't change any logic, essentially every file would have changes. |
I haven't been able to get the ganache job to pass fully yet. It always gets caught after
with I tried a handful of things like increasing retries and timeouts and using yarn registry instead of npmjs but nothing worked yet. I'll keep thinking or tinkering around with what it could be.
Yeah it is because we're on my fork, it is normally in the top right header like this: If you want you can try to open this branch as a PR on your own fork and see if you can control the actions (that would be pretty cool).
Ok no problem, yeah it's a tough transition (but worth it imo!). I'll remove all the unnecessary formatting so the diff is clean with just the important changes. Thanks!! |
85b75c0
to
c290cb8
Compare
c290cb8
to
c9e9c30
Compare
scripts/e2e.npm.publish.sh
Outdated
BRANCH=$TRAVIS_BRANCH | ||
|
||
if [[ $GITHUB_REF = 'refs/pull/'* ]]; then | ||
BRANCH=${GITHUB_HEAD_REF##*/} |
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.
Stumbled upon a small problem with this pattern matcher during #2...
If the branch name has a slash in it - e.g issue/3112
, BRANCH
will be set to 3112
and not get found.
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.
Ah good catch, ok I’ll have to dig into some more bash syntax to learn how to extract it properly (* after 1st or 2nd slash?)
Let me know if you have a suggestion.
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.
Peeked at the env vars in CI and it seemed like GITHUB_HEAD_REF
is the extracted branch name. Maybe you just get this for free?
I tried
- PR into local branch
- PR from fork
.. and both were routed through thisrefs/pull/
clause. Is the other clause for push builds?
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.
Interesting, I was under the impression that head ref was only populated for PRs from forks, but if what you said is true then that’s great and makes it easy :)
It might not be there for push builds though as you mentioned, so it would be worth trying and see what the value is.
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.
head ref was only populated for PRs from forks
Yes, that's what it says in the GH Actions docs...mmm.
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.
Attempted fix in 7297b53
…e branch name includes a folder e.g. `issue/3112`)
3b476c2
to
7297b53
Compare
Pull Request Test Coverage Report for Build 0a4f8537ba1c558e6713129d0046f0df234340d3-PR-1
💛 - Coveralls |
This is a test PR to try out converting from Travis to Github Actions (web3#3393).
This PR: