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

Jail doctestplus outputs in tmpdir #98

Closed
wants to merge 2 commits into from

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Mar 13, 2020

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

  • Does this need a change log?
  • How do we test this? Does this really work?

@pllim pllim added the bug label Mar 13, 2020
@pllim pllim requested review from astrofrog and bsipocz March 13, 2020 20:46
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:
Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

https://github.com/spacetelescope/asdf/blob/522f25b60715612dd1369a644c756fb7572b89d1/asdf/tests/helpers.py#L45-L51

@pllim
Copy link
Contributor Author

pllim commented Mar 13, 2020

    @pytest.fixture(autouse=True)
    def _docdir(request):
        """Run doctests in isolated tmpdir so outputs do not end up in repo"""
        # Trigger ONLY for doctestplus
        doctest_plugin = request.config.pluginmanager.getplugin("doctestplus")
>       if isinstance(request.node, doctest_plugin._doctest_textfile_item_cls):
E       AttributeError: 'NoneType' object has no attribute '_doctest_textfile_item_cls'

Okay, so I guess direct copy-paste didn't work. Any ideas?

@bsipocz
Copy link
Member

bsipocz commented Mar 13, 2020

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.

@astrofrog
Copy link
Contributor

I can think of two ways to test:

  • Have two doctests that run one after the other - the first creates a file and the second asserts that the file doesn't exist

  • Have a doctest that creates a file, and use pytest_configure/pytest_unconfigure to make sure no files are created in the current working directory over the entire test suite run

@pllim
Copy link
Contributor Author

pllim commented Mar 16, 2020

I feel like we cannot proceed here until we know how #95 plays out.

"""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):
Copy link
Contributor Author

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'

@pllim
Copy link
Contributor Author

pllim commented Apr 1, 2020

I think we need to find a different solution on this side.

@pllim pllim closed this Apr 1, 2020
@pllim pllim deleted the output-tmpdir branch April 1, 2020 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent doctest dumping/leaving files in repo
4 participants