CI: Added C and Python Code Coverage#30911
Conversation
|
It might be nice to add a |
|
Do you use the |
|
I was thinking same @mattip , would something like scientific-python/spin#315 work?
No, we use Ninja's builtin gcov: https://github.com/scientific-python/spin/blob/1713846e86d1c84037976e0f2392abead645c75d/spin/cmds/meson.py#L660-L665 |
|
It works 🎉, reports from CI run: It might be worth waiting for scientific-python/spin#316 however, but I'm marking it ready for review as just the command will change. |
|
Cool. The artifacts can only be viewed via the CI run link, and then opening the "Upload Python coverage report" section, then clicking the link. Likewise the C coverage report. The link you put into the comment above does not work (maybe it is only for you?). Then I get a text table showing each file, here are the first few rows:
|
This is a good point, let me see if we can get it readable directly from the PR page somehow through a published link.
There are multiple formats today and even directory upload works, so maybe we can somehow get HTML to work. Let me try it out
There are other packages like diff-cover that can run as a parallel job. There should be some elegant way of doing it that's not too slow. I think this would be of most use for maintainers to check before merging a PR, will look into this as well. Thanks for the comments! |
|
Thanks. This is all a bit experimental, so i expect some of the answers will be “nope, can’t do that” |
I've written and published a GitHub Action designed for exactly this use case just a couple of days ago (it works with |
|
Diff cover seems to work (for Python): https://github.com/numpy/numpy/actions/runs/22886825193/artifacts/5842973779. Added a tmp commit to test: 060f220. In case of missing lines, we get more details like this: # Diff Coverage
## Diff: origin/main...HEAD, staged and unstaged changes
- numpy/lib/_version.py (0.0%): Missing lines 95-96
## Summary
- **Total**: 2 lines
- **Missing**: 2 lines
- **Coverage**: 0%
## numpy/lib/_version.py
Lines 91-100
91 vercmp = 1
92 else:
93 vercmp = -1
94
! 95 if vercmp == 1:
! 96 print("hello")
97
98 return vercmp
99
100 def _compare_pre_release(self, other):
---
|
|
I am not a big fan of including an action that needs write privileges. That is why suggest that we get a nice console based story before thinking about adding it to CI |
|
@mattip what does console-based mean in this context? |
|
Getting a local run of Maybe helpful to take a step back. What exactly would be a user story for the coverage information in practice? Has there been discussion recently asking for coverage? I can see some usages: do tests cover all the code paths in my recent commits? Do I have dead code? But neither of those require the use of CI. How can we get a good tool for the use cases people might care about? |
|
Ahh got it. I always wanted to know if I have ~100% of my newly added code for each PR in C or Python. An additional usecase as you mentioned might be to find dead code, but that can be done easily by: In case of the first requirement, it would be good to show coverage in the PR for maintainers to ask the OPs specific tests to add for someone new |
@mattip the write privilege should only be required for statuses, not for code/issues/etc. FWIW https://github.com/scientific-python/circleci-artifacts-redirector-action also needs the same write status privileges. It's a bit of a GitHub Actions limitation (or you could call it a design choice), but the action won't work until the workflow file is committed to the |
Just because we have one problematic action doesn't mean we like it.
Hmm. Maybe some other reviewers could add more opinions, but when we had it, I rarely looked at the coverage report. |
Let me start a thread in the mailing list. |
4 participants
Added C and Python Code Coverage
Notes
Related
spinin CI #30886os.path.commonprefixdeprecation #30898coverage,gcovandgenerate-lcov-htmlflags inspin test#24992Mailing list discussions link: https://mail.python.org/archives/list/numpy-discussion@python.org/thread/3I5CDDYE6F36M22EPB25B4VDGPWHGWV2/
ToDo