8000 travis: add c code coverage measurements by MatthieuDartiailh · Pull Request #14311 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

travis: add c code coverage measurements #14311

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

Merged
merged 5 commits into from
May 31, 2019

Conversation

MatthieuDartiailh
Copy link
Contributor

PR Summary

Add c-code coverage measurement when testing under linux on Travis. Coverage uses lcov which allows to specify lines that should be ignored, contrary to gcov.

I will monitor the build and the coverage report to make sure they make sense before requesting a review.

@tacaswell tacaswell added this to the v3.2.0 milestone May 24, 2019
@MatthieuDartiailh
Copy link
Contributor Author

I am facing a couple of surprising behaviors here:

  • on Python 3.6, the system tries to install numpy from source (and fails). This is not the case on the master branch but I don't see how it is related to my changes
  • on Python 3.7, I do get the proper reports for the c/c++ code but I lost all the Python code report. Coverage.py complains it did not collect anything but I don't see the relation to my changes. I used the same setup, I used in https://github.com/nucleic/atom where I collect both.

I will investigate them but if anybody comes with a solution he is welcome.

@anntzer
Copy link
Contributor
anntzer commented May 24, 2019

@MatthieuDartiailh and myself recently met at https://githubsatellite.com/ (thanks github for inviting us), and he suggested that he could take advantage of his expertise in setting up C coverage for kiwisolver to add that feature to matplotlib too, which I was quite enthusiastic about. This may also be useful if at some point mplcairo does move to core matplotlib, as mplcairo is nearly fully C++.

@MatthieuDartiailh
Copy link
Contributor Author
MatthieuDartiailh commented May 29, 2019

I fixed the obvious building issue, but I am still confused by how python coverage is collected (or rather not). On this branch I get coverage warning at the end of running pytest but before the slowest tests and short summary (https://travis-ci.org/matplotlib/matplotlib/jobs/538973201#L2545). On master the same warning is issued by later (https://travis-ci.org/matplotlib/matplotlib/jobs/538952156#L2590).
I checked and the same parameters are used on the other ci and actually all ci complains about this but in different places (which makes me wonder how you actually get anything reported).
What bother me is that I also see the Python coverage actually decreases which I believe indicates that there is something slightly off here. If anybody that understands better how Python coverage was set up for matplotlib can comment it would be great.

@MatthieuDartiailh MatthieuDartiailh changed the title [WIP] travis: add c code coverage measurements travis: add c code coverage measurements May 29, 2019
@anntzer
Copy link
Contributor
anntzer commented May 30, 2019

I think the warning is a spurious one, basically pytest-dev/pytest-cov#129.
Perhaps you can check if removing -n2 helps for your case? (i.e. I'm not sure C++ coverage and pytest-xdist play well together)

@MatthieuDartiailh
Copy link
Contributor Author

Thanks for the explanation @anntzer. It looks like you see the same fluctuation of coverage over the python files in #14373 so I guess it is somehow unrelated to my changes. C/C++ coverage measurement can handle multiprocess (if the compiler is recent enough). So I think this is good to go from my point of view.

@anntzer
Copy link
Contributor
anntzer commented May 30, 2019

Indeed, we've always seen these tiny coverage changes and they've always been a mystery to us.

Copy link
Contributor
@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member
@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dstansby dstansby merged commit 012d055 into matplotlib:master May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0