-
-
Notifications
You must be signed in to change notification settings - Fork 405
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
Test that everything can be imported during ci #36591
Conversation
This produces 47K lines of output, and various unexplained error messages.
|
At the end you find a summary, if you scroll up a little bit more you find the full exception stack trace. I've now silenced the other output as much as possible (it still shows how much tests are collected per file). |
That's the summary now:
(more details in the logs) |
I've fixed this bizarre looking error in #36698 |
I'd suggest to just exclude these ( |
Also |
Exclude please |
883e05f
to
e349b00
Compare
There are doctests in these modules, so I think completely ignoring them is to coarse. |
Done
Done |
The log has 2000 lines of the form
https://github.com/sagemath/sage/actions/runs/7285981360/job/19853897084?pr=36591#step:16:26 Could you add a message to the log please that explains what it is? |
best to exclude setup.py
|
There are lots of error messages related to optional packages, for example https://github.com/sagemath/sage/actions/runs/7285981360/job/19853897084?pr=36591#step:16:3134
Would it be easy to skip the files that are marked as depending on an optional feature? |
Co-authored-by: Matthias Köppe <mkoeppe@math.ucdavis.edu>
Done.
Will look into this as follow-up. (It is really not the point of this PR to fix/exclude any of these failing imports) |
Documentation preview for this PR (built with commit cc3792f; changes) is ready! 🎉 |
@@ -174,6 +174,9 @@ jobs: | |||
|
|||
- name: Check that all modules can be imported | |||
run: | | |||
# The following command checks that all modules can be imported. | |||
# The output also includes a long list of modules together with the number of tests in each module. | |||
# This can be ignored. |
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.
Is the output intended? Do you use this information? Or should everyone ignore it?
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.
It's safe to ignore (but pytest doesn't support a way to hide it)
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.
Fine. I assume that you mean that you also do not use this information. We'll iterate the cosmetics in follow-up PRs.
<!-- ^^^^^ 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 sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- 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 sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> Currently you cannot import many modules directly. We use `pytest --collect-only` to test during ci that the imports are working. As a first step, we run pytest through sage, which makes sure that `sage.all` is loaded. Once the reported errors are fixed (e.g. by filtering out modules that fail because of FeatureNotPresentError), we can check that importing without `sage.all` works. ### 📝 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 - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36591 Reported by: Tobias Diez Reviewer(s): Matthias Köppe, Tobias Diez
<!-- ^^^^^ 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 sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- 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 sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> Currently you cannot import many modules directly. We use `pytest --collect-only` to test during ci that the imports are working. As a first step, we run pytest through sage, which makes sure that `sage.all` is loaded. Once the reported errors are fixed (e.g. by filtering out modules that fail because of FeatureNotPresentError), we can check that importing without `sage.all` works. ### 📝 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 - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36591 Reported by: Tobias Diez Reviewer(s): Matthias Köppe, Tobias Diez
<!-- ^^^^^ 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 sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> As reported in https://groups.google.com/g/sage- devel/c/mLtXcaypU3Q/m/jHoVjQ9oAAAJ @kwankyu This failure was introduced as a side effect of sagemath#36591: The pytest discovery of modules disagrees with `sage_setup`'s notion of package directories vs. data directories. We repair it by making `sage.tests.books` an ordinary package directory (correct because the whole thing is shipped by **sagemath-repl**). <!-- 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 sagemath#12345". --> Fixes sagemath#37216 <!-- 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 - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37185 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría
Currently you cannot import many modules directly. We use
pytest --collect-only
to test during ci that the imports are working. As a first step, we run pytest through sage, which makes sure thatsage.all
is loaded. Once the reported errors are fixed (e.g. by filtering out modules that fail because of FeatureNotPresentError), we can check that importing withoutsage.all
works.📝 Checklist
⌛ Dependencies