sage -t: Do not run pytest on individual Python files unless they match the pytest file pattern #31924
Comments
This comment has been minimized.
This comment has been minimized.
|
comment:3
It seems that there is no way to tell So, is there anything standing against just doing it like this? diff --git a/src/bin/sage-runtests b/src/bin/sage-runtests
index 16d3295..e852853 100755
--- a/src/bin/sage-runtests
+++ b/src/bin/sage-runtests
@@ -143,15 +143,16 @@ if __name__ == "__main__":
DC = DocTestController(options, args)
err = DC.run()
- try:
- exit_code_pytest = 0
- import pytest
- pytest_options = ["--import-mode", "importlib"]
- if options.verbose:
- pytest_options.append("-v")
- exit_code_pytest = pytest.main(pytest_options + args)
- except ModuleNotFoundError:
- print("Pytest is not installed, skip checking tests that rely on it.")
+ exit_code_pytest = 0
+ if not args:
+ try:
+ import pytest
+ pytest_options = ["--import-mode", "importlib"]
+ if options.verbose:
+ pytest_options.append("-v")
+ exit_code_pytest = pytest.main(pytest_options + args)
+ except ModuleNotFoundError:
+ print("Pytest is not installed, skip checking tests that rely on it.") |
|
comment:4
Directory arguments work fine, it's just regular file arguments that cause the trouble. |
|
comment:5
Replying to @mkoeppe:
I see! I will have a look at it next week! |
|
comment:6
I think the problem is that in |
|
comment:7
Replying to @tobiasdiez:
I cannot verify this on my environment: ~/devel/sage$ ./local/bin/pytest src/sage/databases/knotinfo_db.py
======================================================================================================================================== test session starts ========================================================================================================================================
platform linux -- Python 3.9.5, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /home/sebastian/devel/sage/src, configfile: tox.ini
collected 0 items / 1 error
============================================================================================================================================== ERRORS ===============================================================================================================================================
__________________________________________________________________________________________________________________________ ERROR collecting sage/databases/knotinfo_db.py ___________________________________________________________________________________________________________________________
ImportError while importing test module '/home/sebastian/devel/sage/src/sage/databases/knotinfo_db.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
local/lib/python3.9/importlib/__init__.py:127: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
src/sage/databases/knotinfo_db.py:31: in <module>
from sage.structure.sage_object import SageObject
src/sage/structure/__init__.py:2: in <module>
import sage.structure.element
E ModuleNotFoundError: No module named 'sage.structure.element'
====================================================================================================================================== short test summary info ======================================================================================================================================
ERROR src/sage/databases/knotinfo_db.py
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
========================================================================================================================================= 1 error in 0.29s ========================================================================================================================================== |
|
comment:8
Okay, then I misread the exception message. Thanks for the investigation. So the problem is that pytest doesn't find the compiled cython modules. I think the common advice is to use the editable install. Another solution that might work is to install and run pytest from the same virtual environment where sage is installed in. Finally, there are some hacks to add the sage path in |
|
comment:9
Replying to @tobiasdiez:
Why does it find them for directory arguments?
How can we achieve one of these options?
I don't think that it is worth to do that (citation from the comment: And do not forget to remove this before publishing to PyPI). |
|
comment:10
Replying to @soehms:
Good question. I have no idea. Maybe the directory argument is in a wrong format so that pytest actually doesn't try to load any test files?
The editable install is possible since #31377 and should just work. I'm not familiar enough with the "usual" sage build to be able to recommend a solution that works with it. In the end, you need to specify the sage install directory as part of the PYTHON_PATH so that pytest is able to find the compiled cython modules. Maybe @mkoeppe has an idea. |
|
comment:11
Replying to @soehms:
I don't think it does. What is happening, if I'm not mistaken, is simply that pytest, as configured in Hence my suggestion of a workaround -- filter out Python file name arguments before passing them to pytest when invoked via |
|
comment:12
Yes, that would work as a workaround. However, the underlying problem is still that pytest cannot find cython modules and I think this should be fixed as well (since otherwise you cannot have a pytest test file that has similar imports as knotinfo_db.py) |
|
comment:13
Replying to @tobiasdiez:
Yes, I agree, but that would be another ticket, as this is not "critical" for Sage 9.4. |
|
comment:14
Replying to @tobiasdiez:
After building Sage with |
|
comment:15
Replying to @mkoeppe:
First, I thought the same. But, the following output made me doubt:
Passing an empty argument list, causes an error, as well: The same output shows with |
|
comment:16
Replying to @soehms:
OK then, that's another case in which |
|
comment:17
I would implement the workaround suggested in comment 11. But at the moment I'm sticking at the following question: What is the best way to share the pattern |
|
comment:21
Thinking about this again, I was wondering what is the desired behavior of the
This also seems to be the default behavior of According to this point of view, the reported error is not really a problem with the pytest test discovery but more so an issue with loading these files as modules. If the import would work, pytest would try to find pytest tests in these files, which don't exist, so it would simply not collect anything. |
|
comment:22
Replying to @tobiasdiez:
To you mean to replace relative import statements by absolute ones all over the source code? Would this solve the problem? BTW: Did you note that the Traceback (most recent call last):
File "/home/sebastian/devel/sage/src/bin/sage-runtests", line 159, in <module>
exit_code_pytest = pytest.main(pytest_options + args)
TypeError: can only concatenate list (not "Namespace") to listProbably this is due to #32332. |
|
comment:23
I don't have much experience with relative imports and namespace imports. But yes, converting relative imports to absolute ones should fix the issue. |
|
comment:24
I get a doctest failure in The second failure gets fixed after #32892 but not the first one. Here's a standalone example of the issue: What I think is going on: pytest is called by where I don't know what's a proper way to fix this. Maybe filter out the list of files in |
|
comment:25
Replying to @tobiasdiez:
Sounds like we should make a script that replaces all relative import statements by an absolute one (all over the source tree). Are we sure that there are no such import statement being intentional (by whatever reason)? |
|
comment:26
Replying to @tornaria:
What is the difference to the suggestion made in comment:11 ff? Do you have a hint to the question I asked in comment:17 ? |
|
comment:112
Replying to @soehms:
There's no fundamental discussion happening here. It's not a question of "either-or", but a strategy of fixing now what can be fixed to get a high quality release 9.6, and doing the larger fixes when they are ready. In comment:78, comment:89, comment:90, etc. I have already explained how to adjust the change in this ticket when this work is done. |
|
comment:113
Replying to @mkoeppe:
I've seen that the discussion was about strategy. But obviously the strategy was not clarified completely and thus the discussion was not finished at that point. By your comment comment 109 and the new ticket #33572 the situation is improved, now. The difficulty with this ticket is that it shades out a couple of issues Tobias is currently working on. I can understand that this was annoying. For example the bug treated in #33550 will not show any more in the way as it did before if this ticket is released. Thus, we should work throughout all of these issues and find new test cases for them, now. Probably, the best way to do this is to use #33572. |
|
comment:114
Replying to @mkoeppe:
All of which are ready for review. |
|
comment:115
Replying to @soehms:
I agree #33572 would be a good way to test. But as the discussion there shows running I hope #33550, #33560, and #33561 get reviewed before this ticket here is merged, since otherwise the reviewers have to revert the changes manually before running |
|
comment:116
May I suggest adding some switches to
And maybe:
I think it would be quite useful (a) if I happen to have pytest installed and for some reason (maybe something is broken, perhaps because a bug in my system pytest, whatever) I prefer to skip pytest (b) if I want to actually test pytest (maybe I'm working on the pytest code) I may want to run pytest on different arguments without having to wait for doctest (c) want to run pytest on some file not matching the pattern. All of these are easy to acomplish by trivial patching of |
|
comment:117
Thanks for the review. |
|
comment:118
Replying to @tornaria:
Separate ticket |
|
comment:119
This is on Apple Silicon M1 Macs with Homebrew up to date, both macOS 11.6.5 (Big Sur) with Xcode 13.2.1 and macOS 12.3 (Monterey) with Xcode 13.3. SageMath 9.6.beta6 together with this ticket #31924 and nothing else: Am I doing something wrong? BTW, I am at a loss with all the tickets mentioned here. Some would not merge with this one. |
|
comment:120
That's fixed in the latest version of #33549 - I haven't merged that here. |
|
comment:122
Sorry for the lack of attention! |
|
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:
|
|
comment:124
Rebased on 9.6.beta6, away from #33549 |
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes #1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> This fixes the failure seen, for example, https://github.com/sagemath/sage/actions/runs/5678761729/job/15389749593 ?pr=35991 Note that the last failure message ``` local/var/lib/sage/venv-python3.8/lib/python3.8/site- packages/matplotlib/tests/__init__.py:6: in <module> raise IOError( E OSError: The baseline image directory does not exist. This is most likely because the test data is not installed. You may need to install matplotlib from source to get the test data. =========================== short test summary info ============================ ERROR - OSError: The baseline image directory does not exist. This is most likely because the test data is not installed. You may need to install matplotlib from source to get the test data. !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!! ============================== 1 error in 12.78s =============================== Error: Process completed with exit code 2. ``` This is not a fault of matplotlib! The fault lies on our pytest, which blindly runs all tests found in the directory tree under the root dir. (matplotlib test files are supposed to run in a development install, where the baseline image directory exists, but our install (SPKG) of matplotlib is not a development install, and hence there's no baseline image directory.) So we need to stop pytest from running blind. The failure in the incremental test workflow results from an error in `sage-runtests`. The relevant lines are ```python exit_code_pytest = 0 import pytest pytest_options = [] if args.verbose: pytest_options.append("-v") # #31924: Do not run pytest on individual Python files unless # they match the pytest file pattern. However, pass names # of directories. We use 'not os.path.isfile(f)' for this so that # we do not silently hide typos. filenames = [f for f in args.filenames if f.endswith("_test.py") or not os.path.isfile(f)] if filenames or not args.filenames: # <------------------------------------ faulty line! print(f"Running pytest on {filenames} with options {pytest_options}") exit_code_pytest = pytest.main(filenames + pytest_options) if exit_code_pytest == 5: # Exit code 5 means there were no test files, pass in this case exit_code_pytest = 0 ``` Look at the faulty line. The condition test allows pytest run without explicit filenames. This results in running pytest for all test files found under the (sage) root dir. Hence pytest attempts to run tests in matplotlib in `local/var/lib/sage/venv-python3.11/lib/python3.11/site- packages/matplotlib/tests`. The branch of this PR fixes the problem. <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes #12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - #12345: short description why this is a dependency - #34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #35999 Reported by: Kwankyu Lee Reviewer(s): Kwankyu Lee, Matthias Köppe, Tobias Diez
sage -tis broken since #31003/#31103 because*_test.pyfiles; but if one usessage -twith file arguments, this will override it and lead to many errors.Related report in https://groups.google.com/g/sage-devel/c/SE_A2Jw5Kko/m/KQpA9GbjBQAJ:
In this ticket, we fix it by passing names of regular files to
pytestonly if they match the pytest pattern (ending with_test.py).(The dependency #32975 renamed the files in the Sage sources that used this naming scheme but were not pytest files.)
Example:
CC: @tobiasdiez @soehms @JohnCremona @dimpase @saraedum @isuruf @tscrim
Component: doctest framework
Author: Matthias Koeppe
Branch/Commit:
1214e0cReviewer: Sebastian Oehms
Issue created by migration from https://trac.sagemath.org/ticket/31924
The text was updated successfully, but these errors were encountered: