Stop skipping tests

[flying-sheep]

Please remove that:

scipy_doctest/scipy_doctest/plugin.py

Lines 52 to 55 in e1a9521

	if config.getoption("--doctest-modules"):
	path_str = str(collection_path)
	if "tests" in path_str or "test_" in path_str:
	return True

I run pytest --doctest-modules --pyargs mypackage ./tests in every single project I maintain to run both unit- and doctests together. Why shouldn’t I?

[ev-br]

Well, I don't believe there's a single answer to which way this goes. Historically, scipy and numpy take the view that doctests are separate (one recent discussion is numpy/numpy#26604 (comment); this is definitely not the first one on the subject).
Again historically, the separation was used to stress the difference between unit tests of the functionality and doctests to provide examples in the documentation.

Historical baggage aside, there is a definite need for a switch.
ISTM we can add a variable to DTConfig so that you can set it in conftest.py of every single project you maintain, and have a single pytest incantation to run both unit tests and doctests.
DTConfig.skip_test_paths = {True,False} --- Any suggestions for a betrter name?

[flying-sheep]

Yeah, --doctest-modules means “also collect and run doctests”.

So maybe --doctest-only would make sense?

[ev-br]

Yeah, --doctest-modules means “also collect and run doctests”.

Well, not in scipy-doctest, it does not mean that :-). That's an arbitrary decision which could cut both ways, and we can spend any amount of time arguing which one is obviously correct.

All that said, if this is a major barrier for adoption, scipy-doctest can contemplate a breaking change. Would need a version bump and thinking through how to migrate existing users.

For numpy and scipy, the main usage is behind the spin command, so bumping the requirement and migrating the internal mapping of spin check-docs from pytest --doctest-modules to pytest --doctest-only is not prohibitively disruptive.

For scikit-learn, I'm not sure what their usage is. Let me ping @lesteve : how disruptive it would be if scipy-doctest==2 changes the meaning of --doctest-modules to "run both unit tests and doctests" and adds --doctest-only for the current "only run doctests"?

[flying-sheep]

That's an arbitrary decision which could cut both ways, and we can spend any amount of time arguing which one is obviously correct.

I don’t want to argue, I’m just saying that pytest already has the behavior I described and you change the meaning of an existing flag. Something I’d have never guessed and needed to read the code for to understand.

That was very surprising to me and I assumed it was a compatibility bug between doctest plugins. I didn’t even consider that it could be a conscious choice until I read the code.

All that said, if this is a major barrier for adoption, scipy-doctest can contemplate a breaking change.

I can only speak for myself: for me, keeping in mind the primary purposes¹ of all pytest plugins is complex enough. Having one of them change built-in functionality without that being its primary purpose isn’t something I want to have to keep in mind.

I’d only use scipy_doctest if it has a way to turn this off (default or not), and I’d feel better about using it if the default was not to change this behavior.

Would need a version bump and thinking through how to migrate existing users.

I totally get that “plugin I’m considering to adopt changes behavior in a surprising way” is not nearly as bad as “plugin I’m already using suddenly changes behavior”, so I totally get it if you don’t ever do it.

But as said: I’d recommend that you at least add a switch to turn this off and document front and center that it does that and how to turn it off.

Footnotes

1. e.g. like pytest-randomly’s whole purpose is to change test order ↩

[lesteve]

In scikit-learn we would need to adapt similarly as in numpy if scipy-doctest==2 decides to change the behaviour.

To be honest, while I originally found it a tiny bit surprising that --doctest-modules only run doctests (different from vanilla pytest), I also find it reasonable that by default doctests are separated from unit tests. In my mind, in scikit-learn we want to make sure that the code examples run and give somewhat meaningful results but doctests should not be considered as thorough tests as unit tests.

[flying-sheep]

I also find it reasonable that by default doctests are separated from unit tests

It would totally be – if that was the default behavior that pytest had, or if --doctest-modules was a new flag introduced by this extension.

[ev-br]

Okay, it's pretty clear that the functionality is needed; I still think that pytest --doctest-modules behavior is suboptimal, but undoing it in a plugin is even less optimal. So, let's plan it.

The desired endgame:

• $ python -m scipy_doctest is consistent with $ python -m doctest : only runs doctests
• $ pytest --doctest-modules is consistent with the pytest behavior: runs both doctests and unit tests
• $ pytest --doctest-only is the current --doctest-modules: only runs doctests

Alternatives are just too ugly/cumbersome: make --doctest-modules depend on a DTConfig entry (yikes), or --doctest-modules --doctest-only (why two switches).

So a rollout plan:

• In 1.8: implement --doctest-only to be identical to the current --doctest-modules
• Migrate existing usages (scipy, numpy, pywavelets, scikit-learn, who else?) to use --doctest-only
• In 2.0: breaking change the meaning of --doctest-modules

[ev-br]

The plan of #198 (comment) is sadly not workable, for two reasons, one large and one small:

The large one: --doctest-only option is used by pytest-doctestplus; if we add it, calling pytest fails if pytest-doctestplus is installed; accordin to pytest-dev/pytest#13435, the failure seems unrecoverable in the current pytest.

The small one: the assumption for --doctest-modules is rather deep in the scipy-doctest code; changing this is too annoying, so I'm being lazy here.

Therefore, gh-199 adds a two-option solution: --doctest-modules --doctest-only-doctests={true,false}.

The amended plan:

In 1.8

• pytest --doctest-modules is backwards compatible (runs only doctests)
• pytest --doctest-modules --doctest-only-doctests=true is equivalent
• pytest --doctest-modules --doctest-only-doctests=false is new, runs both doctests and unit tests

In 2.0

• pytest --doctest-modules runs both (i.e. we're back to parity with vanilla pytest-- this is a breaking change)
• pytest --doctest-modules --doctest-only-doctests=false is equivalent
• pytest --doctest-modules --doctest-only-doctests=true is required to only run doctests

[flying-sheep]

That sounds like a great plan!

[ev-br]

scipy-doctest==1.8.0 is now a thing, https://pypi.org/project/scipy-doctest/

Next step is to add a new CLI flag to

• scipy -- MAINT: smoke-docs: add a --doctest-only-doctests flag to spin/dev.py scipy#23078
• numpy -- MAINT: Bump scipy-doctest to 1.8 numpy/numpy#29085
• pywavelets -- MAINT: require scipy_doctest>=1.8.0 PyWavelets/pywt#805
• scikit-learn -- MAINT: make scipy-doctest usage forward-compatible scikit-learn/scikit-learn#31609

and other known users.