-
Notifications
You must be signed in to change notification settings - Fork 37
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
Jail doctestplus outputs in tmpdir #98
Conversation
doctest_plugin = request.config.pluginmanager.getplugin("doctestplus") | ||
if isinstance(request.node, doctest_plugin._doctest_textfile_item_cls): | ||
# Don't apply this fixture to io.rst. It reads files and doesn't write | ||
if "io.rst" not in request.node.name: |
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.
io.rst
sounds awfully specific to astropy
core library. How do we generalize this?
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.
Only based on the narrative, I don't get why would it hurt to still apply when it reads files but not writes?
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.
Yeah, this is specific to the astropy core library. The problem with that doctest is that it looks for an input file in the working directory. If one makes the working dir a tmpdir
, then the file is no longer in the working directory, so one needs a general way of getting the file. Using importlib.resources
might be the solution.
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.
Hmm looks like importlib.resources is only available since Python 3.7.
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.
It is available as an external package for python 3.6 and earlier. importlib_resources
One can do something like.
Okay, so I guess direct copy-paste didn't work. Any ideas? |
Yes, it does need a changelog. As for a test, can we not just try to write a dummy file in one of the tests? Though I'm not fussy, if it works locally for astropy, I would be happy to merge and open a reminder issue instead, rather than not merging but waiting for a good test solution. |
I can think of two ways to test:
|
I feel like we cannot proceed here until we know how #95 plays out. |
Co-authored-by: James Davies <[email protected]>
"""Run doctests in isolated tmpdir so outputs do not end up in repo""" | ||
# Trigger ONLY for doctestplus | ||
doctest_plugin = request.config.pluginmanager.getplugin("pytest_doctestplus") | ||
if isinstance(request.node, doctest_plugin.DocTestTextfilePlus): |
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.
Not sure how to access the class name when this fixture lives in the plugin itself and not over at core lib.
AttributeError: module 'pytest_doctestplus.plugin' has no attribute 'DocTestTextfilePlus'
I think we need to find a different solution on this side. |
This was originally implemented by @jdavies-st as astropy/astropy#9439 but was decided that this should move here instead.
Fix #68
xref astropy/astropy#10037
TODO