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

Use pytest-doctestplus to skip some inline doctests #1790

Merged
merged 4 commits into from
Mar 8, 2022
Merged

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 7, 2022

Description of proposed changes

We're adding more and more inline code examples in function docstrings as tracked in issue #1686. We don't want these new doctests slow down our CI, so we skip these doctests using the # doctest: +SKIP directive. Here is an example doctest in grdlandmask:

    >>> import pygmt  # doctest: +SKIP
    >>> # Create a landmask grid with an x-range of 125 to 130,
    >>> # and a y-range of 30 to 35
    >>> landmask = pygmt.grdlandmask(
    ...     spacing=1, region=[125, 130, 30, 35]
    ... )  # doctest: +SKIP

As you can see, there are many downsides for this kind of doctests:

  1. # doctest: +SKIP only works for one single line, so we have to add # doctest: +SKIP many times
  2. # doctest: +SKIP has 18 characters. So even a short code like landmask = pygmt.grdlandmask(spacing=1, region=[125, 130, 30, 35]) must be split into multiple lines
  3. there is no simple way to run these tests

In this PR, the pytest-doctestplus plugin is used to solve the problems above. For each module, we just need to define an internal variable __doctest_skip__, which contains a list of functions/classes whose tests should be skipped.
See the changes in pygmt/src/grdlandmask.py as an example and also the official documentation of pytest-doctestplus.

As you can see, it solves the problems 1 and 2 above.

Before After
image image

For problem 3, there is still no easy way, but I think it's possible to comment the __doctest_skip__ line, run the tests, and then revert the changes.

@seisman
Copy link
Member Author

seisman commented Mar 7, 2022

Ping @meghanrjones @weiji14 @willschlitzer @michaelgrund for comments on the new doctest method.

@seisman seisman added the maintenance Boring but important stuff for the core devs label Mar 7, 2022
@seisman seisman added this to the 0.6.0 milestone Mar 7, 2022
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Great find! Should have found this earlier 😆 I have one suggestion in regard to Problem 3 below.

pyproject.toml Outdated
@@ -3,7 +3,7 @@ omit = ["*/tests/*", "*pygmt/__init__.py"]

[tool.pytest.ini_options]
minversion = "6.0"
addopts = "--verbose --durations=0 --durations-min=0.2 --doctest-modules --mpl --mpl-results-path=results"
addopts = "--verbose --durations=0 --durations-min=0.2 --doctest-plus --mpl --mpl-results-path=results"
Copy link
Member

@weiji14 weiji14 Mar 7, 2022

Choose a reason for hiding this comment

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

For problem 3, there is still no easy way, but I think it's possible to comment the doctest_skip line, run the tests, and then revert the changes.

Actually, from reading the doctest-plus docs (https://github.com/astropy/pytest-doctestplus/tree/v0.12.0#setup-and-configuration), it says:

Using pytest's built-in --doctest-modules option will override the behavior of this plugin, even if doctest_plus = enabled in setup.cfg, and will cause the default doctest plugin to be used. However, if for some reason both --doctest-modules and --doctest-plus are given, the pytest-doctestplus plugin will be used, regardless of the contents of setup.cfg.

So maybe we just need to edit the Makefile, and have have a make doctest command that uses --doctest-modules instead of doctest-plus to run all of these doctests? Could also setup a CI to have it run weekly if needed.

Copy link
Member Author

@seisman seisman Mar 7, 2022

Choose a reason for hiding this comment

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

Good suggestion. I removed the --doctest-plus option from pyproject.toml, and added it to make test command. I also the make fulltest target with the --doctest-modules option (I think the make doctest command is misleading).

Now, running make test collects 541 tests, and running make fulltest collects 542 tests. The extra one in make fulltest comes is the grdlandmask doctest.

@weiji14 weiji14 added this to In progress in Release v0.6.x via automation Mar 7, 2022
@seisman seisman marked this pull request as ready for review March 7, 2022 14:25
Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

This is great!

I think the contributors guide should provide information about adding inline examples/doctests including how to skip tests using __doctest_skip__ and run the full tests using make fulltest or pytest pygmt/src/<module>.py --doctest-modules, but this could be handled in a separate PR.

Release v0.6.x automation moved this from In progress to Reviewer approved Mar 7, 2022
@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label Mar 7, 2022
@seisman
Copy link
Member Author

seisman commented Mar 8, 2022

pytest pygmt/src/.py --doctest-modules

I added the --doctest-modules option back to pytest's default options so that people don't have to give the --doctest-modules option when running tests in a single module.

Now here are three different ways to run tests:

  • make test: use the default pytest options (i.e., --doctest-modules --doctest-plus), so that all tests and some doctests will run
  • make fulltest: use --doctest-modules, all tests and all doctests will run
  • pytest pygmt/src/<module>.py: any doctests in the module will run

@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 8, 2022
@seisman seisman merged commit d24c0a6 into main Mar 8, 2022
Release v0.6.x automation moved this from Reviewer approved to Done Mar 8, 2022
@seisman seisman deleted the skip-doctest branch March 8, 2022 03:01
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…ols#1790)

* Use pytest-doctestplus to skip some inline doctests
* Install pytest-doctestplus in workflows
* Add the fulltest target to run all tests, including all doctests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants