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

Fix inconsistency between traced files and reported files #2784

Merged
merged 2 commits into from
Jan 17, 2019
Merged

Fix inconsistency between traced files and reported files #2784

merged 2 commits into from
Jan 17, 2019

Conversation

wjsi
Copy link
Contributor

@wjsi wjsi commented Jan 5, 2019

When solving #2776 which reported Plugin 'Cython.Coverage.Plugin' did not provide a file reporter for '/Users/wenjun.swj/miniconda3/lib/python3.7/site-packages/gevent/_hub_local.py' when executing coverage report, I find the cause is that some tracers built in Cython.Coverage do not have corresponding reporters.
In Cython.Coverage, when Plugin._parse_lines(c_file, filename) returns (None, None), Plugin.file_tracer(filename) returns a tracer, while Plugin.file_reporter(filename) returns None, and then coverage.py reports the error. This happens when shared packages have both *.py and *.c sharing the same base name. For instance, in the wheel package of gevent, both _hub_local.c and _hub_local.py exist, which mislead Cython.Coverage to produce a tracer as it does not ignore shared libraries. However, file_reporter() ignores shared libraries, and the report error is raised.
The simple solution is to ignore shared libraries in file_tracer like it does in file_reporter, and coverage report does not raise errors, thus fixes #2776 .

@scoder
Copy link
Contributor

scoder commented Jan 5, 2019

Thanks. Do you think you could come up with a test case for this? This seems like the kind of setup that will get lost and break right the next time we change the code.
You can look at the existing coverage .srctree tests in tests/run/, they are basically multiple files stuffed into one text archive, with the test commands at the top.

@wjsi
Copy link
Contributor Author

wjsi commented Jan 6, 2019

@scoder A test case is added. It mocks the behavior of gevent._hub_local by importing a Cython-compiled accelerated version and reproduces the error under unmodified Cython.Coverage.

@wjsi wjsi changed the title Fix inconsistency between trace files and report files Fix inconsistency between traced files and reported files Jan 6, 2019
from Cython.Build import cythonize

setup(ext_modules = cythonize([
'pkg/*.py*',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it actually necessary for this test to compile the .py files, in addition to the .pyx files?

I'm asking for two reasons: it complicates the test setup (thus increasing the risk of potentially unrelated failures), and there is currently a bug in your branch that I fixed in latest master regarding the compilation of __init__.py files under Windows, so I'm getting failures in appveyor. Merging master should resolve that, but if we can avoid this complication entirely, then that's clearly preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, changed to *.pyx.

@scoder scoder merged commit 40ddd70 into cython:master Jan 17, 2019
@scoder scoder added this to the 3.0 milestone Jan 17, 2019
@scoder
Copy link
Contributor

scoder commented Jan 17, 2019

Thanks

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.

Cython.Coverage produces error when using gevent
2 participants