Opened 2 years ago
Closed 2 years ago
#31003 closed enhancement (fixed)
Add minimal pytest configuration
Reported by: | gh-tobiasdiez | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.3 |
Component: | build | Keywords: | |
Cc: | mkoeppe | Merged in: | |
Authors: | Tobias Diez | Reviewers: | Matthias Koeppe |
Report Upstream: | N/A | Work issues: | |
Branch: | 462f071 (Commits, GitHub, GitLab) | Commit: | 462f071cbb7823cf1ac59a016fd71d4ff7ab4522 |
Dependencies: | Stopgaps: |
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
Status: | new → needs_review |
---|
comment:2 Changed 2 years ago by
Commit: | d099730421471828214a2959d23b85cd1798ee9f → d5c971ebc1b098cf0946c7b7da54832e4cc40484 |
---|
comment:4 Changed 2 years ago by
Commit: | d5c971ebc1b098cf0946c7b7da54832e4cc40484 → f901922cf7aec7f41430656141dd7e4cfdbc219e |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
f901922 | Add documentation for pytest
|
comment:6 follow-up: 9 Changed 2 years ago by
Does this really need the line from __future__ import annotations
?
comment:8 follow-up: 12 Changed 2 years ago by
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 follow-up: 14 Changed 2 years ago by
Replying to mkoeppe:
Does this really need the line
from __future__ import annotations
?
Yes, for dict[str, Any]
.
comment:10 follow-up: 13 Changed 2 years ago by
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
Commit: | f901922cf7aec7f41430656141dd7e4cfdbc219e → bc66a1060ee08ab0b04682bd38dca79350d50e07 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
bc66a10 | Add no-op disclaimer
|
comment:12 Changed 2 years ago by
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 Changed 2 years ago by
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 Changed 2 years ago by
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
Status: | needs_review → needs_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
Commit: | bc66a1060ee08ab0b04682bd38dca79350d50e07 → 72ae90f95484ed19786b962f0b12f421ea71ceba |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
72ae90f | Clarify that sage has to be installed
|
comment:17 Changed 2 years ago by
Status: | needs_work → needs_review |
---|
You need to run pytest from a virtual environment where Sage has been installed, since otherwise the imports to cython modules are not resolved (e.g. using the editable install). I've clarified this point in the documentation.
comment:19 Changed 2 years ago by
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
Status: | needs_review → needs_work |
---|
comment:21 follow-up: 23 Changed 2 years ago by
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
?
comment:22 Changed 2 years ago by
Commit: | 72ae90f95484ed19786b962f0b12f421ea71ceba → 99c4ffd4d1cde07a243ae9c3d9d8caa4d1340f51 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
99c4ffd | Merge branch 'develop' of git://github.com/sagemath/sage into public/build/pytest_config
|
comment:23 follow-up: 24 Changed 2 years ago by
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 importfrom sage.misc.lazy_attribute import lazy_attribute
?
Yes.
comment:24 Changed 2 years ago by
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:26 follow-up: 28 Changed 2 years ago by
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
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 Changed 2 years ago by
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
Commit: | 99c4ffd4d1cde07a243ae9c3d9d8caa4d1340f51 → bf014ba98eddf0ee6289581de40d50dce894a422 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
bf014ba | Enforce that test files need to end on _test.py
|
comment:30 Changed 2 years ago by
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
Developer's guide (src/doc/en/developer/) would need updates in
- coding_basics.rst
- reviewer_checklist.rst
comment:32 follow-up: 33 Changed 2 years ago by
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 Changed 2 years ago by
comment:34 Changed 2 years ago by
Status: | needs_work → needs_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
Commit: | bf014ba98eddf0ee6289581de40d50dce894a422 → 38e210e6fe21f5f49e066142533f3f7794c6dfb6 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
38e210e | Merge branch 'develop' of git://github.com/sagemath/sage into public/build/pytest_config
|
comment:37 Changed 2 years ago by
Status: | needs_review → needs_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 2 years ago by
Status: | needs_work → needs_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 2 years ago by
Commit: | 38e210e6fe21f5f49e066142533f3f7794c6dfb6 → 62b1c6177607b85a8d2cc5136bc23d660f0d9931 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
62b1c61 | Igonre deprecation_test as well
|
comment:40 Changed 2 years ago by
Reviewers: | → Matthias Koeppe |
---|---|
Status: | needs_review → positive_review |
OK this looks like a nice clean no-op now.
comment:43 Changed 2 years ago by
Commit: | 62b1c6177607b85a8d2cc5136bc23d660f0d9931 → 462f071cbb7823cf1ac59a016fd71d4ff7ab4522 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
462f071 | Fix documentation
|
comment:44 Changed 2 years ago by
Status: | needs_work → positive_review |
---|
Sorry overlooked this. Since the fix was trivial, I'll put it back to "positive review".
comment:45 Changed 2 years ago by
Branch: | public/build/pytest_config → 462f071cbb7823cf1ac59a016fd71d4ff7ab4522 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Branch pushed to git repo; I updated commit sha1. New commits:
Mark as strict testing, since we pass