-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add coverage reporting #2293
Add coverage reporting #2293
Conversation
Current coverage is 83.15% (diff: 100%)
|
Thanks for implementing this! I quickly looked around whether the results look reasonable, and here's one unexpected thing I found: https://codecov.io/gh/python/mypy/src/6e0f98c9570b3ec3102acfa7709f474277d1615a/mypy/test/testcheck.py#L201 @gvanrossum Can you check if the above function should be called? |
Also, this file is all red: https://codecov.io/gh/python/mypy/src/6e0f98c9570b3ec3102acfa7709f474277d1615a/mypy/test/testcmdline.py |
This is only coverage for the pytest tests, right? That would explain things. |
Taking a closer look, that doesn't seem to be intended. Still, it looks to me like coverage isn't getting generated for the myunit tests. That isn't necessarily a problem -- myunit is going to be killed sooner or later anyway. The one change that I would like to see made, though, is to put coverage generation behind a flag. Running the unit tests locally is a pretty frequent part of the development workflow, and having coverage enabled makes them over twice as slow for me (so I'd prefer if coverage was only enabled by default in CI). Thanks for writing this! |
@ddfisher: I've made coverage reporting optional. I suspect that it was significantly slower for you because you may not have had the C-tracer compiled (it didn't have a significant time impact for me). But, no harm making it configurable. I also (I think) got the coverage reports from the various tests to combine correctly (so, for instance, |
Hmm, seems like it's something else for me:
I'm on OS X -- could that be related? It doesn't seem like this is particularly affecting CI times, though, so this LGTM. Thanks! |
I love coverage, but I'm not yet willing to pay for it by having every CI
run be twice as slow. Can we just have it behind a flag please?
Also I suppose it depends on having coverage.py installed -- I'd like that
to be optional as well.
|
Hrm... So, it is behind a flag, but it's enabled for CI. Comparing https://travis-ci.org/python/mypy/jobs/168760048 (a build on master from before coverage landed) and https://travis-ci.org/python/mypy/builds/170515428 (a build on master from after coverage landed), there is a surprisingly large (at least to me) difference in times between both the elapsed and total times taken. I'll make a PR to disable coverage in CI for now, until I can figure out why it's slow. As far as the dependency on |
PR to remove coverage from CI: #2328 |
Regarding the surprising speed difference, wasn't there an optional
accelarator module for coverage? Maybe we should add that to
test-requirements.txt?
|
It should be installed by default when you pip install on travis. I actually verified locally and am seeing about a 25% overhead from coverage being turned on, which was surprising to @nedbat when I talked to him about it. We were just chatting about setting up a coverage loadtest, so that we can validate some of the performance findings. |
No description provided.