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

sage -t: Distinguish .pxd from .pyx in doctest basenames #36238

Merged
merged 2 commits into from
Sep 16, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Sep 10, 2023

These basenames are used in the stats file, recording test failures and running times in ~/.sage/timings2.json.
Currently, element.pyx and element.pxd have the same basename, and so they clobber each other in the stats file. It depends on the order of execution which of the two wins. This affects sage -t --failed and the sort order according to running times.

Here we disambiguate them by adding a suffix for .pxd and .pxi files.

Cherry-picked from #35095, where the stats are used for creating records of known test failures of the modularized distributions.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 12, 2023

Because of the change, I think .pxd files are not doctested. .pxi files are not also doctested, but I think these files do not contain doctests anyway. See this in src/sage/doctest/control.py

...
    if if_installed and ext in ('.py', '.pyx', '.pxd'):
        module_name = get_basename(filename)
        try:
            if not importlib.util.find_spec(module_name):  # tries to import the containing package
                if log:
                    log(f"Skipping '{filename}' because module {module_name} is not present in the venv")
                return True
        except ModuleNotFoundError as e:
            if log:
                log(f"Skipping '{filename}' because module {e.name} cannot be imported")
            return True   
...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 12, 2023

This is only when --if-installed is in use; and a few lines above one can see that .pxd cannot happen here actually. I'll simplify the code to make it clearer.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 12, 2023

OK. Then lgtm.

Let's see the tests pass again.

@github-actions
Copy link

Documentation preview for this PR (built with commit d3a00b7; changes) is ready! 🎉

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests passed.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 13, 2023

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 14, 2023
…senames

    
<!-- ^^^^^
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 -->
These basenames are used in the stats file, recording test failures and
running times in `~/.sage/timings2.json`.
Currently, `element.pyx` and `element.pxd` have the same basename, and
so they clobber each other in the stats file. It depends on the order of
execution which of the two wins. This affects `sage -t --failed` and the
sort order according to running times.

Here we disambiguate them by adding a suffix for .pxd and .pxi files.

<!-- 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". -->
Cherry-picked from sagemath#35095, where the stats are used for creating records
of known test failures of the modularized distributions.
<!-- 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.
- [x] 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#36238
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 14, 2023
…senames

<!-- ^^^^^
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 -->
These basenames are used in the stats file, recording test failures and
running times in `~/.sage/timings2.json`.
Currently, `element.pyx` and `element.pxd` have the same basename, and
so they clobber each other in the stats file. It depends on the order of
execution which of the two wins. This affects `sage -t --failed` and the
sort order according to running times.

Here we disambiguate them by adding a suffix for .pxd and .pxi files.

<!-- 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". -->
Cherry-picked from sagemath#35095, where the stats are used for creating records
of known test failures of the modularized distributions.
<!-- 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.
- [x] 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#36238
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@vbraun vbraun merged commit 905d3c2 into sagemath:develop Sep 16, 2023
16 of 17 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 16, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 23, 2023
…silence modularized distributions in CI

    
<!-- ^^^^^
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 -->

The distributions `sagemath-repl` and `sagemath-categories` run parts of
the testsuite in a virtual environment created by `tox` when
SAGE_CHECK=yes. This also runs as part of the Build & Test CI.

Here we make several improvements:
- Support SAGE_CHECK=warn
- Invoke the doctester with `--baseline-stats-path={toxinidir}/known-
test-failures.json`
- Make the tests silent unless new test failures, not recorded in the
`known-test-failures.json`, are encountered

To help maintain the `known-test-failures.json`, we also add new
features to the command `sage --fixdoctests`.
- `sage --fixdoctests --update-known-test-failures` reads the stats
files generated by the doctester in the virtual environments and writes
updated `known-test-failures.json` files to the source tree
- doctester stats now also include a field `ntests` - number of doctests
of a module that were run

`sage --fixdoctests` also receives new switches `--distribution all`,
`--fixed-point`, `--verbose`, `--no-diff` and some other improvements.

<!-- 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". -->
- Part of: sagemath#29705
- Cherry-picked from: sagemath#35095
<!-- 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.
- [x] 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: ...
-->
- Depends on sagemath#36263 (merged here)
- Depends on sagemath#36238 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36264
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 1, 2023
    
<!-- ^^^^^
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 -->

NumPy 1.26 provides Python 3.12 support and Cython 3 compatibility.

1.26.0 was released 2023-09-16.

As NumPy has changed its build system from setuptools-pinned-to-an-
ancient-version to meson_python, we get rid of our ancient version of
`setuptools`. The SPKGs `setuptools` and `setuptools_wheel` now ship the
same (current) version.

- [x] Check portability run:
https://github.com/mkoeppe/sage/actions/runs/5959239800
- [ ] Check SAGE_FAT_BINARY

<!-- 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". -->
Resolves sagemath#34816
<!-- 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.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies
- Depends on sagemath#36112 (for the `meson_python` upgrades, merged here)
- Depends on sagemath#35404 (merged here as
part of above)
- Depends on sagemath#35810 (merged here as part of above)
- Depends on sagemath#36110 (merged here)
- Depends on sagemath#36263 (merged here)
- Depends on sagemath#36238 (merged here)
- Depends on sagemath#36255 (merged here)
<!-- 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#36123
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit that referenced this pull request Oct 8, 2023
    
<!-- ^^^^^
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 -->

NumPy 1.26 provides Python 3.12 support and Cython 3 compatibility.

1.26.0 was released 2023-09-16.

As NumPy has changed its build system from setuptools-pinned-to-an-
ancient-version to meson_python, we get rid of our ancient version of
`setuptools`. The SPKGs `setuptools` and `setuptools_wheel` now ship the
same (current) version.

- [x] Check portability run:
https://github.com/mkoeppe/sage/actions/runs/5959239800
- [ ] Check SAGE_FAT_BINARY

<!-- 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". -->
Resolves #34816
<!-- 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.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies
- Depends on #36112 (for the `meson_python` upgrades, merged here)
- Depends on #35404 (merged here as
part of above)
- Depends on #35810 (merged here as part of above)
- Depends on #36110 (merged here)
- Depends on #36263 (merged here)
- Depends on #36238 (merged here)
- Depends on #36255 (merged here)
<!-- 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: #36123
Reported by: Matthias Köppe
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants