Opened 2 years ago

Closed 21 months ago

#31003 closed enhancement (fixed)

Add minimal pytest configuration

Reported by: Tobias Diez Owned by:
Priority: major Milestone: sage-9.3
Component: build Keywords:
Cc: Matthias Köppe Merged in:
Authors: Tobias Diez Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 462f071 (Commits, GitHub, GitLab) Commit: 462f071cbb7823cf1ac59a016fd71d4ff7ab4522
Dependencies: Stopgaps:

Status badges

Description

Add a minimal pytest configuration, so that calling pytest from src passes without any errors (and without finding any tests for the moment).

This is a preparation for future usages of pytest, such as #30362 or #30738, and is in the general spirit of #28936 (since pytest is perhaps the testing framework for python). For example, one could imagine using pytest doctest_modules to run all sage doctests as well.

Change History (45)

comment:1 Changed 2 years ago by Tobias Diez

Status: newneeds_review

comment:2 Changed 2 years ago by git

Commit: d099730421471828214a2959d23b85cd1798ee9fd5c971ebc1b098cf0946c7b7da54832e4cc40484

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

d5c971eMark as strict testing, since we pass

comment:3 Changed 2 years ago by Matthias Köppe

This file would need comments at the top that explain its purpose

comment:4 Changed 2 years ago by git

Commit: d5c971ebc1b098cf0946c7b7da54832e4cc40484f901922cf7aec7f41430656141dd7e4cfdbc219e

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

f901922Add documentation for pytest

comment:5 Changed 2 years ago by Tobias Diez

I've now added documentation, also in the tools.rst.

comment:6 Changed 2 years ago by Matthias Köppe

Does this really need the line from __future__ import annotations?

comment:7 Changed 2 years ago by Matthias Köppe

Also, should this not be hooked into tox?

comment:8 Changed 2 years ago by Matthias Köppe

I think it should be said explicitly in the file comments (and the documentation) that this is a no-op and we do not offer ANY tests tested by pytest.

(similar to the wording in the ticket description: "Add a minimal pytest configuration, so that calling pytest from src passes without any errors (and without finding any tests for the moment).")

comment:9 in reply to:  6 ; Changed 2 years ago by Tobias Diez

Replying to mkoeppe:

Does this really need the line from __future__ import annotations?

Yes, for dict[str, Any].

comment:10 in reply to:  7 ; Changed 2 years ago by Tobias Diez

Replying to mkoeppe:

Also, should this not be hooked into tox?

That would be the way to go, but I wanted to keep the ticket small (for a change ;-)).

comment:11 Changed 2 years ago by git

Commit: f901922cf7aec7f41430656141dd7e4cfdbc219ebc66a1060ee08ab0b04682bd38dca79350d50e07

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

bc66a10Add no-op disclaimer

comment:12 in reply to:  8 Changed 2 years ago by Tobias Diez

Replying to mkoeppe:

I think it should be said explicitly in the file comments (and the documentation) that this is a no-op and we do not offer ANY tests tested by pytest.

Done!

comment:13 in reply to:  10 Changed 2 years ago by Matthias Köppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Also, should this not be hooked into tox?

That would be the way to go, but I wanted to keep the ticket small (for a change ;-)).

I agree. (Given that it's a no-op, there's no point to hook it into tox now.)

comment:14 in reply to:  9 Changed 2 years ago by Matthias Köppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Does this really need the line from __future__ import annotations?

Yes, for dict[str, Any].

Thanks. So it won't be compatible with Python 3.6, but I guess that's OK because we don't really use pytest for anything.

comment:15 Changed 2 years ago by Matthias Köppe

Status: needs_reviewneeds_work

I am getting the following:

$ sage -sh
$ (cd src && pytest)
=========================================================== test session starts ===========================================================
platform darwin -- Python 3.8.6, pytest-6.1.2, py-1.9.0, pluggy-0.13.1
rootdir: /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/src
collected 0 items / 1 error                                                                                                               

================================================================= ERRORS ==================================================================
_____________________________________________ ERROR collecting sage/tests/deprecation_test.py _____________________________________________
ImportError while importing test module '/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/src/sage/tests/deprecation_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/local/Cellar/python@3.8/3.8.6/Frameworks/Python.framework/Versions/3.8/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
sage/tests/deprecation_test.py:12: in <module>
    from sage.misc.superseded import deprecated_function_alias
sage/misc/superseded.py:30: in <module>
    from sage.misc.lazy_attribute import lazy_attribute
E   ModuleNotFoundError: No module named 'sage.misc.lazy_attribute'
========================================================= short test summary info =========================================================
ERROR sage/tests/deprecation_test.py

comment:16 Changed 2 years ago by git

Commit: bc66a1060ee08ab0b04682bd38dca79350d50e0772ae90f95484ed19786b962f0b12f421ea71ceba

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

72ae90fClarify that sage has to be installed

comment:17 Changed 2 years ago by Tobias Diez

Status: needs_workneeds_review

You need to run pytest from a virtual environment where Sage has been installed (e.g. using the editable install), since otherwise the imports to cython modules are not resolved. I've clarified this point in the documentation.

Last edited 2 years ago by Tobias Diez (previous) (diff)

comment:18 Changed 2 years ago by Tobias Diez

Does it work for you now?

comment:19 Changed 2 years ago by Matthias Köppe

sage -sh does set up an environment in which python3 is Sage's python (and pytest is from this environment too when it is installed).

comment:20 Changed 2 years ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:21 Changed 2 years ago by Tobias Diez

The same code/steps is working for me and correctly reports collected 0 items. I think there is a problem with your setup that cythonized sage modules cannot be found. Can you manually import from sage.misc.lazy_attribute import lazy_attribute?

Last edited 2 years ago by Tobias Diez (previous) (diff)

comment:22 Changed 2 years ago by git

Commit: 72ae90f95484ed19786b962f0b12f421ea71ceba99c4ffd4d1cde07a243ae9c3d9d8caa4d1340f51

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

99c4ffdMerge branch 'develop' of git://github.com/sagemath/sage into public/build/pytest_config

comment:23 in reply to:  21 ; Changed 2 years ago by Matthias Köppe

Replying to gh-tobiasdiez:

The same code/steps is working for me

... in your custom setup from #30371, I assume...

and correctly reports collected 0 items. I think there is a problem with your setup that cythonized sage modules cannot be found. Can you manually import from sage.misc.lazy_attribute import lazy_attribute?

Yes.

comment:24 in reply to:  23 Changed 2 years ago by Tobias Diez

Replying to mkoeppe:

Replying to gh-tobiasdiez:

The same code/steps is working for me

... in your custom setup from #30371, I assume...

No, with sage -sh as you did above...

comment:25 Changed 2 years ago by Matthias Köppe

So you have a working standard installation of sage now?

comment:26 Changed 2 years ago by Tobias Diez

Yes, after a PC upgrade and with Ubuntu 20.10 in WSL 2 and as many system packages installed it works now. I still prefer my setup from #30371 for testing things though (because of the editable install and the nice integration with vscode).

comment:27 Changed 2 years ago by Tobias Diez

I did a bit of googling, and found essentially nothing. The closest was https://stackoverflow.com/questions/49068162/pytest-cannot-find-module-on-import-but-code-runs-just-fine/49068163#49068163 but that doesn't suggest a clear solution.

comment:28 in reply to:  26 Changed 2 years ago by Matthias Köppe

Replying to gh-tobiasdiez:

Yes, after a PC upgrade and with Ubuntu 20.10 in WSL 2 and as many system packages installed it works now. I still prefer my setup from #30371 for testing things though (because of the editable install and the nice integration with vscode).

Great. So how about reducing #30371 then, finally, to something that makes the editable install work on top of the normal installation via the distribution?

comment:29 Changed 2 years ago by git

Commit: 99c4ffd4d1cde07a243ae9c3d9d8caa4d1340f51bf014ba98eddf0ee6289581de40d50dce894a422

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

bf014baEnforce that test files need to end on _test.py

comment:30 Changed 2 years ago by Matthias Köppe

As discussed in #31103, src/bin/sage-coverage, src/bin/sage-coverageall will need updating so that they do not complain about ..._test.py files. (They are also used by the patchbot.)

comment:31 Changed 2 years ago by Matthias Köppe

Developer's guide (src/doc/en/developer/) would need updates in

  • coding_basics.rst
  • reviewer_checklist.rst

comment:32 Changed 2 years ago by Tobias Diez

Can I do this as follow-up tickets? The purpose of this one here was to provide a minimal test configuration that works for sage. Using pytest in an effective way definitely needs more changes in various areas.

comment:33 in reply to:  32 Changed 2 years ago by Matthias Köppe

Replying to gh-tobiasdiez:

Can I do this as follow-up tickets?

Sure

comment:34 Changed 2 years ago by Tobias Diez

Status: needs_workneeds_review

Ok, this is now #31123.

Can you please retry if it works for you? For me sage -sh followed by pip install pytest and pytest works. You need to run pytest from a python installation in which sage is installed including the Cython modules.

comment:35 Changed 2 years ago by git

Commit: bf014ba98eddf0ee6289581de40d50dce894a42238e210e6fe21f5f49e066142533f3f7794c6dfb6

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

38e210eMerge branch 'develop' of git://github.com/sagemath/sage into public/build/pytest_config

comment:36 Changed 23 months ago by Tobias Diez

Still needs review.

comment:37 Changed 23 months ago by Matthias Köppe

Status: needs_reviewneeds_work

The issue persists. I think the problem is that, when executing pytest from the src directory, the subdirectory sage (which does not have the compiled modules) shadows the installed sage package. So I think you need to set some option for pytest so that . is not in the path.

comment:38 Changed 23 months ago by Tobias Diez

Status: needs_workneeds_review

I read a bit more about it, and in order for pytest to find cython modules you need to use an editable install (that's why it was always working for me, regardless of how I started pytest). For now, I've added deprecation_test.py to the ignored list, so the issue should be fixed.

comment:39 Changed 23 months ago by git

Commit: 38e210e6fe21f5f49e066142533f3f7794c6dfb662b1c6177607b85a8d2cc5136bc23d660f0d9931

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

62b1c61Igonre deprecation_test as well

comment:40 Changed 23 months ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

OK this looks like a nice clean no-op now.

comment:41 Changed 23 months ago by Tobias Diez

Thanks!

comment:42 Changed 23 months ago by Volker Braun

Status: positive_reviewneeds_work

Documentation doesn't build

comment:43 Changed 23 months ago by git

Commit: 62b1c6177607b85a8d2cc5136bc23d660f0d9931462f071cbb7823cf1ac59a016fd71d4ff7ab4522

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

462f071Fix documentation

comment:44 Changed 23 months ago by Tobias Diez

Status: needs_workpositive_review

Sorry overlooked this. Since the fix was trivial, I'll put it back to "positive review".

comment:45 Changed 21 months ago by Volker Braun

Branch: public/build/pytest_config462f071cbb7823cf1ac59a016fd71d4ff7ab4522
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.