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

Add an ini option doctest_standalone_namespace #6978

Closed

Conversation

asmeurer
Copy link

@asmeurer asmeurer commented Mar 27, 2020

This makes doctests run in a standalone namespace. The default is False, which
does the pre-existing behavior where doctests reuse the namespace of the
module they are written in.

Fixes scientific-python/pytest-doctestplus#15.

I still need to document this flag. But before I do, is this the best name for this option? I feel like the name I used here isn't great, but I couldn't think of a better one.

This makes doctests run in a standalone namespace. The default is False, which
does the pre-existing behavior where doctests reuse the namespace of the
module they are written in.

Fixes scientific-python/pytest-doctestplus#15.
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks a lot @asmeurer, this looks great already!

We just need a few extra things:

  1. Please add a new section explaining this option, including rationale and a small example, in doc/en/doctest.rst. Also include a link to the reference (see below).
  2. Include reference documentation for the option in doc/en/reference.rst#configuration-options, including a link to the doctest section added above.
  3. A feature CHANGELOG entry. Ideally it would describe the option shortly and include a link to the docs added in 1).

Again, thanks for working on this. 👍

@@ -62,6 +62,12 @@ def pytest_addoption(parser):
parser.addini(
"doctest_encoding", "encoding used for doctest files", default="utf-8"
)
parser.addini(
"doctest_standalone_namespace",
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of any better options for the name either.
How about calling it doctest_context with a non bool type, or even better, doctest_reset_context? That would leave the option up to implement something similar to the plot directive's context. And then, this can remain a bool.

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with doctest_reset_context, although perhaps doctest_empty_context better conveys what the option actually does?

Copy link
Author

Choose a reason for hiding this comment

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

From a technical point of view, "namespace" is a better term than "context". Two function doctests don't interact with each other, which is what "reset context" seems to imply to me. So if you define a variable in one doctest it won't be available in another doctest.

Rather, they just reuse the module namespace, so if a variable is defined at the module level (not in a doctest), it gets defined automatically in a doctest. Maybe a name like doctest_no_module_namespace (although honestly my biggest issue with the current name is that it is too long).

Copy link
Member

Choose a reason for hiding this comment

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

IMHO a long option name is OK if it correctly describes the behavior, because it will be written in a pytest.ini file only once anyway. 😁

Copy link
Author

Choose a reason for hiding this comment

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

I guess my problem isn't so much the length as the fact that the option name seems convoluted. I don't think many people will look at "doctest_standalone_namespace" and understand what it means unless they already know.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point, agree.

doctest_no_module_namespace seems better in that light then, IMHO.

asmeurer added a commit to asmeurer/ndindex that referenced this pull request Apr 23, 2020
This flag isn't supported in pytest yet. It will be once
pytest-dev/pytest#6978 is merged. However, I would
recommend waiting until that PR is merged to merge this since the name of the
flag may change.

The flag will make it so that the doctests require all names to be imported in
order to pass, as opposed to the default behavior which automatically includes
all names from the surrounding module in the doctest namespace.
) -> None:
if _is_mocked(obj):
return

if self.config.getini("doctest_standalone_namespace"):
globs = {}

Choose a reason for hiding this comment

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

Based on https://github.com/pytest-dev/pytest/blob/5.3.1/src/_pytest/doctest.py#L109-L113 that seems to use DoctestModule for *.py and DoctestTextfile for *.txt/*.rst, as this code change is only in DoctestModule, does this mean that this flag would apply only to the doctest examples inside *.py files and not the files that are fully doctest e.g. *.rst? Is that intended?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think non-py files can have variables that are defined outside of the doctest. Or is there something I'm missing?

Choose a reason for hiding this comment

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

There is the fixture doctest_namespace.

I am still getting familiar with it, though I have the impression that doctest_namespace carries state across modules / folders and I was wondering whether this patch was going to interact with doctest_namespace.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how this currently interacts with that, nor how it should.

Copy link
Member

Choose a reason for hiding this comment

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

It does interact, but in this case I don't think the new option should interact at all with the doctest_namespace fixture: the intent there is to inject globals into the tests (such as the numpy import), so resetting that would defeat the purpose.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe it makes sense to allow globs to be modified in a fixture in conftest.py (side note: "globs" is an unfortunate name from doctest; it is short for "globals" and has nothing to do with shell globs). Although I do like the simplicity of an ini setting to get the globs = {} behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, taking a second look at the code, doctest_namespace won't be affected by this at all, because the doctest_namespace dict is injected during the setup phase of the test, while here globs is cleared during collection.

@asmeurer could you please add a test which ensures setting doctest_standalone_namespace to True still allow doctests to access the dict returned by doctest_namespace? We also should explicitly mention this in the description of the doctest_standalone_namespace.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Hi everyone,

Sorry for the lack of response here.

Besides the discussion about the name of the option, we also would need:

  • Changelog
  • Add @asmeurer to authors
  • Add a description and use case for the option in doctest.rst
  • Quick reference to it in reference.rst

@asmeurer
Copy link
Author

Sorry for sitting on this. I do intend to fix it. I partly haven't finished it because I don't really like the option name used here, partly because I had some issues building the docs, and partly because I got sidetracked with other things.

@@ -62,6 +62,12 @@ def pytest_addoption(parser):
parser.addini(
"doctest_encoding", "encoding used for doctest files", default="utf-8"
)
parser.addini(
"doctest_standalone_namespace",
Copy link
Member

Choose a reason for hiding this comment

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

IMHO a long option name is OK if it correctly describes the behavior, because it will be written in a pytest.ini file only once anyway. 😁

) -> None:
if _is_mocked(obj):
return

if self.config.getini("doctest_standalone_namespace"):
globs = {}
Copy link
Member

Choose a reason for hiding this comment

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

Actually, taking a second look at the code, doctest_namespace won't be affected by this at all, because the doctest_namespace dict is injected during the setup phase of the test, while here globs is cleared during collection.

@asmeurer could you please add a test which ensures setting doctest_standalone_namespace to True still allow doctests to access the dict returned by doctest_namespace? We also should explicitly mention this in the description of the doctest_standalone_namespace.

@asmeurer
Copy link
Author

I added a TODO list to the PR description so I don't forget anything.

Actually another reason I haven't worked on this is that for the project I was going to use it on, I've had to move to a custom doctest runner because I am doctesting Markdown files and I need to be able to tell the doctest runner to ignore lines starting with ```. I do this by monkeypatching DoctestRunner.run. I don't know if there is some way to do the same thing with pytest-doctest.

@asmeurer
Copy link
Author

Or maybe it should just be smart enough to do that automatically for Markdown files. I can open a new issue.

Base automatically changed from master to main March 9, 2021 20:40
@nicoddemus
Copy link
Member

@asmeurer closing this for now to reduce the number of opened PRs, but feel free to reopen in case you want to retake this. 👍

@nicoddemus nicoddemus closed this Oct 5, 2021
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.

Check examples to be standalone
4 participants