-
Notifications
You must be signed in to change notification settings - Fork 924
CI: Only run coverage jobs on master #2214
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
Conversation
eb02199
to
44150ba
Compare
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.
Won't this mean we lose information about test coverage in PRs? I personally don't regular inspect this, but some people might?
Yes that is correct. @nevi-me @HaoYang670 @jhorstmann @viirya @sunchao do you ever use the per PR code coverage report ? I am proposing to only run coverage jobs on master to improve our CI |
An alternative could be to run coverage as part of tests, via I locally use |
On second thoughts, I believe we can't use certain actions, I wanted to add llvm-cov is generally faster than tarpaulin though, so I'll explore getting it to run without the action above |
Thanks @nevi-me ! Note there is already one simple action in the |
Marking as a draft so we don't merge this PR accidentally |
Okay, thanks, I'll work on this on the weekend, if I can't make llvm-cov work, we can move to using tarpaulin only on master. |
For what it is worth I don't think this ticket is super high priority -- I was just going through all the CI checks in general (on the occasion of the |
Shall we update and get this one in, there seems to be fairly broad consensus? |
👍 I will do so. We'll save some CO2 and maybe help the other PRs to run a bit faster. We can always re-enable if someone wants to see it. |
I don't really know how to test this other than on master, so YOLO here we go: |
Well, this is promising -- the job is queued to run on the master commit: https://github.com/apache/arrow-rs/runs/7679145290?check_suite_focus=true |
Benchmark runs are scheduled for baseline = d87f6a4 and contender = 5166a08. 5166a08 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Draft until #2212Which issue does this PR close?
re #2149
Rationale for this change
The codecov job is long (example takes 20 minutes, and I haven't seen its results referred to in PRs myself.
It also has several unfortunate issues (like not accounting for coverage of doc tests), which we should probably file as a separate issue if anyone is looking at its results.
Thus, let's only run it on pushes to master
What changes are included in this PR?
Are there any user-facing changes?
no