-
-
Notifications
You must be signed in to change notification settings - Fork 534
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
Push coverage from GH Actions; remove TravisCI #1034
Conversation
- run: npm run test-cov | ||
- run: npm install ${{ matrix.typescript }} --force | ||
- run: set +e ; npm run test-cov ; echo $? > tests-exit-code | ||
- run: npm install -g coveralls@2 && cat ./coverage/lcov.info | coveralls |
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.
You can use if: ${{ always() }}
instead: https://help.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#always.
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.
Thanks, I was having so much trouble navigating the github actions docs.
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.
It's been pretty challenging, but I've been using it in some private monorepos lately so got a bunch of first-hand experience (which seems to be the only way to learn).
I can't recall ever having to do anything... But I found this: https://github.com/nickmerwin/node-coveralls#usage. We should probably just use this: https://github.com/coverallsapp/github-action. |
I believe you can also grab |
It's not urgent, but if we can it's probably nice - I mostly disabled this across all repos due to the number of notifications I'd otherwise receive. |
@cspotcode We should also consider creating a "nightly" GitHub Action next that tests the latest TypeScript build. We have it currently set up on Travis CI, though I've mostly not kept up with the failures. |
I'll look into a nightly test. For notifications, I think that's configured on a user-by-user basis. At least I can't find anything that lets me configure in the workflow yml. |
@blakeembrey I think I have a nightly test set up correctly. Do you know why it's saying coverage dropped so much? Also, how do you typically use the coverage results? Since it's for the emitted .js, I can't use the UI to visualize coverage. |
Nevermind my question about dropping coverage; it must have been caused by the intentially failing test. Now coverage says it's good. |
@blakeembrey This is ready for a final rubber stamp. |
In this repo, I'm not sure since a lot of the tests run using the CLI - I've never actually double checked that we're measuring it correctly (pretty sure I wasn't). In general, I view it locally if it's well under what I expect. We could move it to properly measure coverage on TypeScript though, things have improved a lot since I originally set up |
@blakeembrey Are there environment variables that need to be passed to coveralls? I can't seem to see where they would be configured on TravisCI.
Also fixes failing lint step after upgrading typescript compiler version.EDIT moved this to a separate PR #1035node 6 tests uncovered this since they do not obey package-lock.json
Do we need to preserve notifications behavior?