-
Notifications
You must be signed in to change notification settings - Fork 110
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
Enable coverage for subprocesses #6546
Conversation
d663979
to
43ed0e8
Compare
The changes follow the documentation in coverage.readthedocs.io/subprocess.html and pymotw.com/2/site/#sitecustomize. They are based on a config variable switch to enable subprocess coverage reporting, and the introduction of a sitecustomize.py script, that performs a coverage process startup for non-datalad processes. I encountered unexpected behavior in that a system-wide installation of DataLad interfered with coverage reports - I suspect this is because I altered the PYTHONPATH variable to make the sitecustomize.py script discoverable. This unexpected behavior manifested in coverage debug messages stating that DataLad and other packages installed in site-packages would be part of stdlib, which coverage by default would not trace. I worked around this by changing datalad's installation to a user-based installation. Maybe this could also be resolved by placing sitecustomize.py into a path that already is in the Pythonpath (Lib/), but I did not go down this route. Fixes datalad#6367
ahh, I just noticed that this only enables this for Travis. Maybe we can also do it for appveyor. |
The changes on travis pass over all checks, and bump coverage especially in |
Code Climate has analyzed commit bc34a9d and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Things seems to work for appveyor, too. |
The test failures are all known, except for one, that is a stalling CI run that looks unrelated to this change. |
Didn't see stalling recently so wouldn't be sure that it is unrelated (didn't look in detail). Does it work if restarted? |
FWIW restarted Travis |
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.
This looks good to me and well worth when exploring DataLad's blind spots.
It did stall/slow on Ubuntu (appveyor) too, though (but only one run) -- so I restarted that one.
Otherwise, good to go, I think. Thx!
The appveyor failures are the known stuff. |
The changes here enable subprocess coverage and fix #6367. They follow different docs:
COVERAGE_PROCESS_START
environment variable, and lets it point to the.coveragerc
file.sitecustomize.py
file. As this file needs to be in the Pythonpath, I'm following https://pymotw.com/2/site/#sitecustomize to modify thePYTHONPATH
accordinglysite-packages
as it reported them to be "in the stdlib" (see details below). We should discuss if this is a problem - was there any reason that the installation is done system-wide? Is there any problem doing it only per user (pip install . --user
)?I have tested whether this works and what an impact it would make by running only
datalad.distributed
tests, which include theora_remote.py
that was covered to only 33.9%:With the changes in this PR, coverage of
ora_remote.py
seems to be bumped by +46% to 80% coverage: