-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
TST, CI: turn on codecov patch diffs #12253
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
f91d5c2
to
eae231b
Compare
* now that we have an open channel of communication with the codecov team and GitHub app has been activated (scipygh-12240), try turning on pull request patch diffs again, similar to NumPy PR 16409
eae231b
to
850487b
Compare
Co-authored-by: Thomas Hu <thomasrockhu@users.noreply.github.com>
One note here, we've been seeing some issues with users using |
Ah, very cool, before applying your fix I did:
and I can indeed reproduce locally:
|
* use the codecov `yaml` validation server in our CI to check for issues in our codecov config; based on discussion in scipy/scipy#12253 * I think the exit code is still `0` when invalid, but at least the output would provide some traction if coverage seems to be misbehaving * we could also try to parse the output and set exit code based on validity status; maybe overkill for now though?
The |
Those files were changed recently in other PRs though, that's probably a clue. I think I also saw that @peterbell10 may change how our Travis build caching works--not sure if that's related or not. |
It's showing the diff since 3393164. I'm guessing this is because coverage results aren't uploaded for |
Hmm, so I'd have to convince the team to re-enable running the full test suite for one job in a CI matrix somewhere for merge commits basically. |
@tylerjereddy would carryforward flags be helpful here (aka use flags so you don't have to upload all coverage reports on every commit)? |
It looks like we do still use GitHub Actions to run CI after a push to If we were to turn on codecov reports for the MacOS jobs on GitHub Actions, would that provide the necessary "base" for producing useful patch diffs? Are patch diffs really only useful for the same job "type?" So, if we only push codecov reports for MacOS CI, would the patch diff contribution for Windows 32 bit on Azure CI play nicely? |
I don't know. I say let's just try it and see if it yields something useful, and disable it if it doesn't. Another option would be to add one more entry to the GitHub actions that does the most complete testing possible (TESTMODE=full) with the fastest build (probably Linux?). That is assuming that the GitHub actions is usually not (and wouldn't become) our bottleneck, which I think is probably true. |
w.r.t github actions, there can be more than one workflow YAML file in |
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.
LGTM. I say we merge and try it. It's easy enough to add patch: false
or target: 1
in the future if this turns out not to be useful. But I've used a similar config in other repos and so far it has been helpful.
Alright, I'll put it in then. If patch coverage diffs do start working properly it can indeed be useful for the reviewer of PRs. |
with the codecov team and GitHub app has been activated
(Codecov migration to marketplace app #12240), try turning on pull request patch diffs again,
similar to TST, CI: turn on codecov patch diffs numpy/numpy#16409