Skip to content
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

ENH,WIP: Add support for including cython in coverage analysis #1004

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

aragilar
Copy link
Member

Adds support for optionally building the h5py cython code with coverage support, WIP until coverage is setup for the rest of h5py.

@aragilar aragilar added this to the 2.9 milestone Feb 20, 2018
@aragilar aragilar force-pushed the add_cython_coverage branch 2 times, most recently from f8cd9d9 to 55c153f Compare June 11, 2018 08:01
@tacaswell tacaswell modified the milestones: 2.9, 2.10 Oct 14, 2018
@takluyver
Copy link
Member

Has anyone worked on setting up coverage for the pure Python parts of h5py?

@aragilar
Copy link
Member Author

I've fixed the issue where codecov wasn't receiving the data in #1160, and #1003 added the actual collection of coverage via pytest, so the python part should work once #1160 is merged.

@takluyver
Copy link
Member

The test failure looks like a random thing - I saw a similar failure on a different job on #1132.

@aragilar
Copy link
Member Author

Yep, I had appveyor re-run the test and it seemed to pass...

@takluyver
Copy link
Member

👍

Do you want to rebase this now that #1160 is merged so we can check it's working correctly with codecov? Or merge it and try it out in master?

@aragilar
Copy link
Member Author

Rebased, let's see how it does.

Aside, should we look at using something like https://bors.tech/ or https://mergify.io/ to run the tests on the final version before commiting the merge (does the meeseeksmachine bot do that I wonder)?

@takluyver
Copy link
Member

If master has changed, usually a quick close/reopen of the pull request will rebuild the PR merged onto current master. I only suggested rebasing in this case because I want to see codecov compare it against the new 'base' commit. So I don't feel a great need to introduce more services - but I also don't have strong objections if you want to use them.

@takluyver
Copy link
Member

Looks like something's not quite working:

==> Collecting reports
    Generating coverage xml reports for Python
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.6.7/bin/coverage", line 11, in <module>
    sys.exit(main())
  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/coverage/cmdline.py", line 756, in main
    status = CoverageScript().command_line(argv)
  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/coverage/cmdline.py", line 509, in command_line
    self.coverage.load()
  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/coverage/control.py", line 675, in load
    self._init()
  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/coverage/control.py", line 226, in _init
    self.plugins = Plugins.load_plugins(self.config.plugins, self.config, self.debug)
  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/coverage/plugin_support.py", line 40, in load_plugins
    __import__(module)
ModuleNotFoundError: No module named 'Cython'
    Error running `coverage xml -i`: None

@takluyver
Copy link
Member

I'm guessing that's because Cython gets installed in the test environment set up by tox, but not in the Travis environment where the coverage data is being processed.

@aragilar
Copy link
Member Author

I've added Cython to the outer Travis/appveyor python interpreter, let's see if it now works.

@takluyver
Copy link
Member

Coverage is getting uploaded again, but it doesn't appear to be measuring the Cython files. They're not showing up in the coverage reports in the log either.

@codecov
Copy link

codecov bot commented Jan 10, 2019

Codecov Report

Merging #1004 into master will decrease coverage by 0.54%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
- Coverage   84.28%   83.73%   -0.55%     
==========================================
  Files          17       18       +1     
  Lines        2049     2146      +97     
==========================================
+ Hits         1727     1797      +70     
- Misses        322      349      +27
Impacted Files Coverage Δ
h5py/h5py_warnings.py 89.47% <0%> (-10.53%) ⬇️
h5py/_hl/dataset.py 84.19% <0%> (-3.5%) ⬇️
h5py/_hl/selections.py 80.98% <0%> (-2.24%) ⬇️
h5py/_hl/filters.py 88.95% <0%> (-1.75%) ⬇️
h5py/_hl/attrs.py 86.17% <0%> (-1.23%) ⬇️
h5py/_hl/vds.py 96.07% <0%> (-0.29%) ⬇️
h5py/ipy_completer.py 0% <0%> (ø) ⬆️
h5py/_hl/__init__.py 100% <0%> (ø) ⬆️
h5py/highlevel.py 100% <0%> (ø)
h5py/_hl/group.py 96% <0%> (+0.03%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7261bc4...1e98bd5. Read the comment docs.

@aragilar
Copy link
Member Author

I forgot to actually specific that the tests should have Cython coverage (the envvar wasn't set), but testing this locally I get Fatal Python error: PyThreadState_Get: no current thread, so I'll need to dig into what's happening there before this can be merged.

@aragilar
Copy link
Member Author

Update: Somehow, coverage is including the coverage of the Cython files, but not the metadata stating that the Cython.Coverage plugin needs to be used to read the files (coverage is treating them as python files, previously it couldn't find the files, which is what the latest commit fixes).
As to why that metadata isn't being written, I'm not sure.

@takluyver
Copy link
Member

Bumping from the 2.10 milestone because this isn't a priority and we're not currently sure how to make it work.

@takluyver takluyver removed this from the 2.10 milestone Apr 25, 2019
@takluyver takluyver mentioned this pull request Aug 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants