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

Add minimal pytest configuration #31003

Closed
tobiasdiez opened this issue Dec 4, 2020 · 53 comments
Closed

Add minimal pytest configuration #31003

tobiasdiez opened this issue Dec 4, 2020 · 53 comments

Comments

@tobiasdiez
Copy link
Contributor

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.

CC: @mkoeppe

Component: build

Author: Tobias Diez

Branch/Commit: 462f071

Reviewer: Matthias Koeppe

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

@tobiasdiez tobiasdiez added this to the sage-9.3 milestone Dec 4, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2020

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

d5c971eMark as strict testing, since we pass

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2020

Changed commit from d099730 to d5c971e

@mkoeppe
Copy link
Member

mkoeppe commented Dec 4, 2020

comment:3

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2020

Changed commit from d5c971e to f901922

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2020

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

f901922Add documentation for pytest

@tobiasdiez
Copy link
Contributor Author

comment:5

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

@mkoeppe
Copy link
Member

mkoeppe commented Dec 5, 2020

comment:6

Does this really need the line from future import annotations?

@mkoeppe
Copy link
Member

mkoeppe commented Dec 5, 2020

comment:7

Also, should this not be hooked into tox?

@mkoeppe
Copy link
Member

mkoeppe commented Dec 5, 2020

comment:8

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).")

@tobiasdiez
Copy link
Contributor Author

comment:9

Replying to @mkoeppe:

Does this really need the line from future import annotations?

Yes, for dict[str, Any].

@tobiasdiez
Copy link
Contributor Author

comment:10

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 ;-)).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2020

Changed commit from f901922 to bc66a10

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2020

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

bc66a10Add no-op disclaimer

@tobiasdiez
Copy link
Contributor Author

comment:12

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!

@mkoeppe
Copy link
Member

mkoeppe commented Dec 5, 2020

comment:13

Replying to @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.)

@mkoeppe
Copy link
Member

mkoeppe commented Dec 5, 2020

comment:14

Replying to @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.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 6, 2020

comment:15

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2020

Changed commit from bc66a10 to 72ae90f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2020

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

72ae90fClarify that sage has to be installed

@tobiasdiez
Copy link
Contributor Author

comment:17

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.

@tobiasdiez
Copy link
Contributor Author

comment:18

Does it work for you now?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2020

Changed commit from bf014ba to 38e210e

@tobiasdiez
Copy link
Contributor Author

comment:36

Still needs review.

@mkoeppe
Copy link
Member

mkoeppe commented Jan 23, 2021

comment:37

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.

@tobiasdiez
Copy link
Contributor Author

comment:38

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2021

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

62b1c61Igonre deprecation_test as well

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2021

Changed commit from 38e210e to 62b1c61

@mkoeppe
Copy link
Member

mkoeppe commented Jan 23, 2021

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Member

mkoeppe commented Jan 23, 2021

comment:40

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

@tobiasdiez
Copy link
Contributor Author

comment:41

Thanks!

@vbraun
Copy link
Member

vbraun commented Jan 24, 2021

comment:42

Documentation doesn't build

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 28, 2021

Changed commit from 62b1c61 to 462f071

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 28, 2021

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

462f071Fix documentation

@tobiasdiez
Copy link
Contributor Author

comment:44

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

@vbraun
Copy link
Member

vbraun commented Mar 7, 2021

Changed branch from public/build/pytest_config to 462f071

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

3 participants