Skip to content

Skip doctest collection in subpackages based on requirements#112

Merged
pllim merged 8 commits intoscientific-python:masterfrom
astrofrog:doctest-skip-subpackage
May 20, 2020
Merged

Skip doctest collection in subpackages based on requirements#112
pllim merged 8 commits intoscientific-python:masterfrom
astrofrog:doctest-skip-subpackage

Conversation

@astrofrog
Copy link
Contributor

@astrofrog astrofrog commented May 15, 2020

This implements an idea related to those in #110, namely that we can skip collecting doctests in specific sub-packages by specifying paths and requirements in the setup.cfg file, e.g.:

    doctest_subpackage_requires =
        astropy/wcs/*:scipy>2.0;numpy>1.14
        astropy/cosmology/*:scipy>1.0

This ended up being cleaner than using e.g. __doctest_subpackage_requires__ in e.g. __init__.py files since these would need to be parsed with e.g. ast and not imported and so on.

Note that for skipping setup_package.py files there is already a solution which is to specify:

doctest_norecursedirs =
    */setup_package.py

in setup.cfg files and it works nicely.

Note that this then adds back the default behavior of failing on import errors since there is now a way to work around those.

EDIT: Fix #110

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks. Conceptually, this is awesome!

However, I think we should open an experimental PR over at astropy to test this out in practice before merging, if the other reviewers are okay with this PR.

c.join('testcode.py').write(pyfile)

reprec = testdir.inline_run(test, "--doctest-plus")
reprec.assertoutcome(passed=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to check that the one passed is actually "a"?

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

README.rst Outdated
>>> asdf.open('file.asdf')

Finally, it is possible to skip collecting doctests in entire subpackages by
using the ``doctest_subpackage_requires`` option in ``pytest.ini`` or
Copy link
Contributor

Choose a reason for hiding this comment

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

What about tox.ini? I think that is one of the 3 files that pytest will look for; see https://docs.pytest.org/en/latest/customize.html#finding-the-rootdir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact elsewhere in the README we only mention setup.cfg so I've updated it to match other parts of the README.

@astrofrog
Copy link
Contributor Author

However, I think we should open an experimental PR over at astropy to test this out in practice before merging, if the other reviewers are okay with this PR.

Will do!

@astrofrog
Copy link
Contributor Author

@pllim - what do you find more readable:

 doctest_subpackage_requires =
        astropy/wcs/*:scipy>2.0;numpy>1.14

or

 doctest_subpackage_requires =
        astropy/wcs/*=scipy>2.0;numpy>1.14

(that is, : or = as a separator?)

@pllim
Copy link
Contributor

pllim commented May 16, 2020

I think the = separator is more consistent with the rest of setup.cfg?

@astrofrog
Copy link
Contributor Author

Ok I'll change it once I know if astropy/astropy#10358 works

@astrofrog
Copy link
Contributor Author

Not sure why _check_distribution emitted a warning but I don't think it's needed, so removing that.

Copy link
Contributor

@saimn saimn left a comment

Choose a reason for hiding this comment

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

Looks great 👍


parser.addini("doctest_subpackage_requires",
"A list of paths to skip if requirements are not satisfied. Each item in the list "
"should have the syntax path:req1;req2.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be path= ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I am excited! Some minor comments.

break

for option in config.getini("doctest_subpackage_requires"):
subpackage_pattern, required = option.split('=', 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will adding spaces around = affect the logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep it works! But I added calls to strip() in my latest commit to make really sure.

astrofrog and others added 2 commits May 20, 2020 14:10
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

LGTM. Definitely a great improvement. Thanks!

@pllim pllim merged commit 492f520 into scientific-python:master May 20, 2020
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.

add doctest_ignore_import_errors_modules

3 participants