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 to run doctests #33546

Closed
mkoeppe opened this issue Mar 21, 2022 · 52 comments
Closed

Use pytest to run doctests #33546

mkoeppe opened this issue Mar 21, 2022 · 52 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 21, 2022

As suggested in #33232 comment:1, ./sage --pytest now uses pytest to discover and run doctests.

To test, run ./sage --pytest src/sage/manifolds/. This results in

48 failed, 1416 passed, 7 warnings

With many of the failed tests being due to some issues with assumptions and/or symbolic variables and/or deprecation warnings.
To see examples of these failures, run pytest on

src/sage/manifolds/differentiable/examples/sphere.py
src/sage/manifolds/utilities.py
src/sage/manifolds/chart.py

We also add tests that verify that sage --pytest (and sage -t) correctly complain about failing doctests.

Follow-ups:

Depends on #33572

CC: @tobiasdiez @tornaria

Component: doctest framework

Author: Tobias Diez

Branch: cc19e92

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/33546

@mkoeppe mkoeppe added this to the sage-9.7 milestone Mar 21, 2022
@mkoeppe mkoeppe changed the title Replace Sage doctest discovery by using pytest_collect_file Replace custom Sage doctest discovery by pytest (using pytest_collect_file) Mar 21, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 22, 2022

Dependencies: #33549

@tobiasdiez

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

Changed dependencies from #33549 to none

@tobiasdiez
Copy link
Contributor

Branch: public/tests/pytest_doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

f67476fAdd some test and prelimary code

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

Commit: f67476f

@tobiasdiez
Copy link
Contributor

Dependencies: #33572

@tobiasdiez
Copy link
Contributor

comment:7

In principle this seems to work now. To test, run ./sage --pytest src/test_doctest.py. Feedback welcome.

Any idea why the import of Integer is necessary in test_doctest.py and how to do this on a global level? How is this done for sage -t?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 29, 2022

Changed commit from f67476f to 538ead3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 29, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

9385501src/bin/sage: Implement 'sage --pytest'
abbb61fsrc/conftest.py: import sage.all to avoid cyclic import errors
9bb72easrc/conftest.py: Add # type: ignore, add reference to trac ticket
2c16704src/conftest.py: Remove outdated text
30642basrc/bin/sage: Handle 'sage -pytest' without file args
f805567Merge remote-tracking branch 'origin/u/mkoeppe/sage__pytest' into public/tests/pytest_doctests
538ead3First working prototype

@tobiasdiez

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

comment:9

There are still a few issues, but as a first version it should be good to go.

@tobiasdiez tobiasdiez changed the title Replace custom Sage doctest discovery by pytest (using pytest_collect_file) Use pytest to run doctests Apr 5, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2022

Changed commit from 538ead3 to 58e11d7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2022

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

d881a5asrc/conftest.py: Add # type: ignore, add reference to trac ticket
1242e1fsrc/conftest.py: Remove outdated text
0d9225dsrc/bin/sage: Handle 'sage -pytest' without file args
05bc58fsrc/tox.ini (pytest): Set --import-mode importlib here, not in src/bin/sage, src/bin/sage-runtests
8825079src/doc/en/developer/tools.rst: Update section on pytest
5e6bc87Merge remote-tracking branch 'origin/u/mkoeppe/sage__pytest' into public/tests/pytest_doctests
d36adf4Fix import of sage.all in tests
813a541Add vs code config to debug pytest
d3fdd95Cleanup vscode config
58e11d7Fix checker and namespace injection

@tobiasdiez

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 5, 2022

Replying to @mkoeppe:

Currently, the file src/test_doctest.py has been added. This is to make reviewing of this ticket easier. For example, ./sage --pytest src/test_doctest.py is supposed to fail so that the reviewer can experiment with how failures are reported with pytest. I will remove this test file after the initial review.

Would it be possible to add a doctest for that?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 1, 2022

comment:13

A concern from #33531:

I couldn't help but notice that pytest will use a single thread to run, which is a HUGE regression if tests start migrating from the sage doctest framework to pytest !!! Right now running pytest through all these 9 files takes 80s, while the doctest step in --long --all mode takes just 312s wall time (using 36 threads).

@tobiasdiez
Copy link
Contributor

comment:14

There are plugins that enable parallel processing of tests like https://github.com/pytest-dev/pytest-xdist and https://github.com/browsertron/pytest-parallel. Let's keep this in mind and do it as a follow-up (especially since I'm not sure how well tested sage is with concurrent runs).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 2, 2022

comment:15

Replying to @tobiasdiez:

There are still a few issues, but as a first version it should be good to go.

I haven't tested it, but won't this ticket lead to failures when sage -t invokes pytest?

@tornaria
Copy link
Contributor

tornaria commented May 2, 2022

comment:16

Replying to @tobiasdiez:

There are plugins that enable parallel processing of tests like https://github.com/pytest-dev/pytest-xdist and https://github.com/browsertron/pytest-parallel. Let's keep this in mind and do it as a follow-up (especially since I'm not sure how well tested sage is with concurrent runs).

Parallel doctesting WORKS. As mentioned above, I can use 36 threads to doctest in --long --all mode in just 312s wall time (instead of ~3 hours which would take using a single thread). Have you actually tried using those plugins? They are not available in my distro (void linux).

Non-rethorical question: what is the advantage of pytest over the current sage doctest framework?

@kiwifb
Copy link
Member

kiwifb commented May 2, 2022

comment:17

Replying to @tornaria:

Non-rethorical question: what is the advantage of pytest over the current sage doctest framework?

My own answer: should sage developers build and maintain their own unique testing framework when there are mature testing framework around? Could their time be better used if they didn't have to maintain that framework that no one else (apart possibly a few downstream package) uses or want?

@tobiasdiez
Copy link
Contributor

comment:18

Replying to @tornaria:

Replying to @tobiasdiez:

There are plugins that enable parallel processing of tests like https://github.com/pytest-dev/pytest-xdist and https://github.com/browsertron/pytest-parallel. Let's keep this in mind and do it as a follow-up (especially since I'm not sure how well tested sage is with concurrent runs).

Parallel doctesting WORKS. As mentioned above, I can use 36 threads to doctest in --long --all mode in just 312s wall time (instead of ~3 hours which would take using a single thread). Have you actually tried using those plugins? They are not available in my distro (void linux).

If I'm not mistaken that only runs the tests in multiple independent workers and thus is only parallelism (i.e. pytest-xdist). But as far as I'm aware there are no extensive tests of sage with concurrency, that is, with multiple threads sharing the same state (i.e. pytest-parallel).

Anyway, both packages are ordinary pip installable packages.

@tobiasdiez
Copy link
Contributor

comment:19

Replying to @kiwifb:

Replying to @tornaria:

Non-rethorical question: what is the advantage of pytest over the current sage doctest framework?

My own answer: should sage developers build and maintain their own unique testing framework when there are mature testing framework around? Could their time be better used if they didn't have to maintain that framework that no one else (apart possibly a few downstream package) uses or want?

Cannot agree more. Plus using standard tools increases synergy effects, for example IDEs have special support for pytest but not for sage's custom test runner. Also new developers might have heard of and used pytest before, but have to learn sage's test runner conventions (eg command line args).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 6, 2022

comment:27
[pytest]
 python_files = *_test.py
+norecursedirs = local prefix venv build pkgs .git src/sage/pkgs src/doc src/bin src/sage/src/sage_setup

src/sage/pkgs and src/sage/src/sage_setup do not exist

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 6, 2022

comment:28

Also could you update the ticket description please?

@tobiasdiez

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 8, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

428a18cRemove nonexisting paths from config

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 8, 2022

Changed commit from f863428 to 428a18c

@tobiasdiez
Copy link
Contributor

comment:31

Replying to @mkoeppe:

[pytest]
 python_files = *_test.py
+norecursedirs = local prefix venv build pkgs .git src/sage/pkgs src/doc src/bin src/sage/src/sage_setup

src/sage/pkgs and src/sage/src/sage_setup do not exist

You are right. Not sure why I've added them in the first place, but I've removed them now.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 8, 2022

comment:32

In a build without --enable-editable:

./sage -pytest src/sage/manifolds/chart.py                                                                                                                         git:t/33546/public/tests/pytest_doctests
=================================================================================================== test session starts ====================================================================================================
platform darwin -- Python 3.10.2, pytest-7.1.2, pluggy-1.0.0
rootdir: /Users/mkoeppe/s/sage/sage-rebasing/src, configfile: tox.ini
collected 0 items / 2 errors                                                                                                                                                                                               

========================================================================================================== ERRORS ==========================================================================================================
_________________________________________________________________________________________ ERROR collecting sage/manifolds/chart.py _________________________________________________________________________________________
src/conftest.py:93: in collect
    module = import_path(self.path, root=self.config.rootpath)
local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/_pytest/pathlib.py:556: in import_path
    raise ImportPathMismatchError(module_name, module_file, path)
E   _pytest.pathlib.ImportPathMismatchError: ('sage.manifolds.chart', '/Users/mkoeppe/s/sage/sage-rebasing/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/manifolds/chart.py', PosixPath('/Users/mkoeppe/s/sage/sage-rebasing/src/sage/manifolds/chart.py'))
_________________________________________________________________________________________ ERROR collecting sage/manifolds/chart.py _________________________________________________________________________________________
local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/_pytest/runner.py:338: in from_call
    result: Optional[TResult] = func()
local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/_pytest/runner.py:369: in <lambda>
    call = CallInfo.from_call(lambda: list(collector.collect()), "collect")
local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/_pytest/doctest.py:545: in collect
    module = import_path(self.path, root=self.config.rootpath)
local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/_pytest/pathlib.py:556: in import_path
    raise ImportPathMismatchError(module_name, module_file, path)
E   _pytest.pathlib.ImportPathMismatchError: ('sage.manifolds.chart', '/Users/mkoeppe/s/sage/sage-rebasing/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/manifolds/chart.py', PosixPath('/Users/mkoeppe/s/sage/sage-rebasing/src/sage/manifolds/chart.py'))
================================================================================================= short test summary info ==================================================================================================
ERROR src/sage/manifolds/chart.py - _pytest.pathlib.ImportPathMismatchError: ('sage.manifolds.chart', '/Users/mkoeppe/s/sage/sage-rebasing/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/manifolds...
ERROR src/sage/manifolds/chart.py - _pytest.pathlib.ImportPathMismatchError: ('sage.manifolds.chart', '/Users/mkoeppe/s/sage/sage-rebasing/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/manifolds...
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 2 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
==================================================================================================== 2 errors in 1.16s =====================================================================================================

@tobiasdiez
Copy link
Contributor

comment:33

I cannot reproduce this on gitpod (which uses the editable install but is currently my only means to test things).

The strange thing is that this part of the import code shouldn't be invoked at all and this exception is only raised in the pre/append import mode. https://github.com/pytest-dev/pytest/blob/f22451717d95d9938d95019d6399a509487f35dd/src/_pytest/pathlib.py#L492-L508 vs https://github.com/pytest-dev/pytest/blob/f22451717d95d9938d95019d6399a509487f35dd/src/_pytest/pathlib.py#L556. For some reason pytest is not picking up the config from the tox ini correctly in your case. Can you please try to add --import-mode importlib to the command and see if it helps? Or setting PY_IGNORE_IMPORTMISMATCH=1 should suppress this error.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 8, 2022

comment:34

You can just reconfigure without --enable-editable and rebuild to test

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

cc19e92Specify importlib as import mode to be consistent

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2022

Changed commit from 428a18c to cc19e92

@tobiasdiez
Copy link
Contributor

comment:36

Should work now.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

comment:38

I've opened tickets for the follow-ups.

@mkoeppe

This comment has been minimized.

@tobiasdiez
Copy link
Contributor

comment:39

Two times thanks!

@vbraun
Copy link
Member

vbraun commented May 20, 2022

Changed branch from public/tests/pytest_doctests to cc19e92

@dimpase
Copy link
Member

dimpase commented Dec 7, 2022

comment:41

see #34828 for further discussion on doctesting and pytest. I noticed that if run on an out of tree .py file with sage: doctrings, they are ignored (while >>> are executed!). This seems to be a bug missed on this ticket - of course one wants Sage doctests work outside of Sage tree as well, and this is supported by "classic" Sage doctesting, i.e. by sage -doctest.

@dimpase
Copy link
Member

dimpase commented Dec 7, 2022

Changed commit from cc19e92 to none

@dimpase
Copy link
Member

dimpase commented Dec 7, 2022

comment:42

Replying to Dima Pasechnik:

I noticed that if run on an out of tree .py file with sage: doctrings, they are ignored (while >>> are executed!). This seems to be a bug missed on this ticket - of course one wants Sage doctests work outside of Sage tree as well, and this is supported by "classic" Sage doctesting, i.e. by sage -doctest.

Opened a followup ticket #34829 to deal with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants