-
-
Notifications
You must be signed in to change notification settings - Fork 395
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
Remove global sage.all import in pytest #35598
Conversation
That would a pleasant surprise if this already works. But where do we see what tests were actually run by pytest? I have the suspicion that it's not running tests, because of the namespace package issue; in https://github.com/sagemath/sage/actions/runs/4863120862/jobs/8670370254?pr=35598, it says
|
It was actually an issue with the pytest collection (the skip call for pyx files made it skip all files). This is fixed now. However, there are a few failing tests. Do you have an idea why the import of pyscipopt and cvxpy fail? |
They are not standard packages, so not installed |
Since these tests never ran, I probably didn't add the magic to conditionalize the tests. Could you help with this? |
Okay, thanks. Was easy enough to fix. So this is ready for review again (the failing doctest seems unrelated). |
Documentation preview for this PR is ready! π |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looking great. I've pushed a small change re the use of a variable from sage.env
, which makes it better for downstream.
Thanks for the review! |
π Description
Thanks to the recent advantages around
sage.all
imports (thanks @mkoeppe), we can remove the global sage.all import for pytests. This means that all pytests run now without this global import, and also the collection of tests is way quicker.Of course, for doctests invoked by pytest the
all
import is still needed, so it's moved into the corresponding fixture.π Checklist
β Dependencies