-
-
Notifications
You must be signed in to change notification settings - Fork 360
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
Switch from Travis CI to GitHub Actions #504
Switch from Travis CI to GitHub Actions #504
Conversation
I dunno. GH Actions is promising, but that’s a fairly large amount of config for something the Travis file does in 19 lines. |
@sindresorhus You can do it in fewer files, but I myself has opted for splitting them up in my repositories, to get them as fully separate workflows in the UI and logs. It would be possible to fully replicate the Travis CI one I believe, but then the bonus of more granular feedback would get lost. Edit: The |
I gave it a second look to see whether I could slim the files down. Now they are pretty much are the simplest bare minimum that I can figure out, where one stills gets granular feedback in the UI. One could smash the three different tests together even more, but I'm not sure whether it will become more readable or worse + you likely/easily lose some of the feedback in the UI. One thing though: I do know that |
Use |
@sindresorhus Done 👍 @fisker Nice, my only feedback on that one is that it doesn't give granular feedback on whether it's the linting/integration that fails or something else – just like the Travis tests ;) Any takeaways from when you made that one? |
package.json
Outdated
@@ -15,7 +15,8 @@ | |||
}, | |||
"scripts": { | |||
"test": "xo && nyc ava", | |||
"lint": "node ./test/lint/lint.js", | |||
"test-coverage": "nyc --reporter=lcov ava", | |||
"lint": "node ./test/lint/lint.js && xo", |
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.
XO is now run twice, both on test
and lint
.
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.
@sindresorhus Yes and no. In the GitHub CI pipe test-coverage
is run rather than test
. Partly to get coverage report and partly to move the xo
part from being run on every Node.js version to being run just once as part of the linting flow.
So, test.yml
runs test-coverage
while other.yml
(suggestion on better name?) runs lint
and integration
I thought it was best to leave the test
as is, as that's likely the one that gets run manually the most.
Alright, let's do this. Can you drop the Travis file and integrate the badges? |
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.
We should use songle yml for all jobs
Done 👍
I don't see the benefit of mashing them all together in one file. Now there's at least just two files: One for those tests that don't care about which node.js version they run on and another that tests across all supported node.js versions. So all tests that are similar to one another are in the same file and those that don't really share any similarity, they are kept in separate files. |
@voxpelli use one file we can use one badge, and they are not that much |
@fisker I don't think having two badges are a bug but a feature. It's to me two very different things whether linting fails or whether tests fails. Tests verifies functionality while linting verifies code quality. Getting good indicators around which one is failing is a benefit on top of Travis CI I think. (Though I do recognize that I haven't contributed a good name for the @sindresorhus I did a rebase + squash as well as removed Node 8 to match |
.github/workflows/test.yml
Outdated
- uses: coverallsapp/github-action@master | ||
with: | ||
github-token: ${{ secrets.github_token }} | ||
parallel-finished: true |
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.
On my experience, this not right, seem need another job wait for matrix, not sure. I tried in this way before.
So I dont use parallel https://github.com/fisker/id-placeholder/blob/ae506a0069bb459f6100f2c317f377ba9f8651de/.github/workflows/continuous-integration.yml#L52
Also, this action some times fail for no reason, issue can found on their repo, so I use continue-on-error
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.
The parallel-finished
is what they document should be used when running multiple tests in parallel: https://github.com/coverallsapp/github-action#inputs
I added the continue-on-error
on error now 👍 Good feedback!
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.
I know that, also here says
Set to true in the last job, after the other parallel jobs steps have completed, this will send a webhook to Coveralls to set the build complete.
on matrix, the other parallel jobs steps have not completed.
Like I said, I'm not sure how to make it work, I tried this way before, but seems not right |
Yeah, in the example it does like what I did: https://github.com/coverallsapp/github-action/blob/master/README.md#complete-parallel-job-example 🤔 |
I don't really see the value of two badges either. You have to click the badge regardless to see the details, and neither should be failing. |
But this example is not on matrix. We can skip changing this part, just keep them. |
One more thing eslint-plugin-ava already did this https://github.com/avajs/eslint-plugin-ava/blob/master/.github/workflows/ci.yml |
.github/workflows/test.yml
Outdated
name: Node.js ${{ matrix.node_version }} on ${{ matrix.os }} | ||
runs-on: ${{ matrix.os }} | ||
strategy: | ||
matrix: |
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.
fail-fast: false is needed here
@voxpelli found a project use parallel, check the commit history of this file, guess I am right |
"rules": { | ||
"import/order": "off" | ||
} |
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.
This is really weird, see
https://github.com/voxpelli/eslint-plugin-unicorn/runs/437762645?check_suite_focus=true
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.
@voxpelli I did some adjustment. @sindresorhus You may want take a look. |
.github/workflows/ci.yml
Outdated
continue-on-error: true | ||
with: | ||
github-token: ${{ secrets.github_token }} | ||
parallel-finished: true |
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.
Why cannot this just be part of the test
job? I don't see what it does differently from the code in the test
job either.
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.
This job send a webhook to coveralls, need wait for all test job finished.
See #504 (comment) and #504 (comment)
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.
Why though? We only need coveralls to run in one of the test job, not all.
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.
I removed parallel
, and add a condition. only run on node.js 12 on linux
@sindresorhus Do you want a nice title on each task? If not, I think we are good to go |
readme.md
Outdated
@@ -1,4 +1,4 @@ | |||
# eslint-plugin-unicorn [![Build Status](https://travis-ci.org/sindresorhus/eslint-plugin-unicorn.svg?branch=master)](https://travis-ci.org/sindresorhus/eslint-plugin-unicorn) [![Coverage Status](https://coveralls.io/repos/github/sindresorhus/eslint-plugin-unicorn/badge.svg?branch=master)](https://coveralls.io/github/sindresorhus/eslint-plugin-unicorn?branch=master) | |||
# eslint-plugin-unicorn ![Build Status](https://github.com/sindresorhus/eslint-plugin-unicorn/workflows/CI/badge.svg) [![Coverage Status](https://coveralls.io/repos/github/sindresorhus/eslint-plugin-unicorn/badge.svg?branch=master)](https://coveralls.io/github/sindresorhus/eslint-plugin-unicorn?branch=master) |
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.
Shouldn't the badge be linkified?
Nah |
…-plugin-unicorn into tooling/github-workflows
Thanks @voxpelli |
Some benefits:
Example runs can be seen here:
https://github.com/voxpelli/eslint-plugin-unicorn/actions?query=branch%3Atooling%2Fgithub-workflows
Badges like these ones can be added to readme after workflows has been added: