Reenable pytest again as part of `sage -t` by tobiasdiez · Pull Request #40446 · sagemath/sage

tobiasdiez · 2025-07-19T10:32:18Z

Pytest used to run on as part of sage -t, but this is no longer the case. Here, we reactivate it by removing some safe-guards that are no longer necessary as pytest is a standard package now.

Moreover, a "runtime error" due to a changing dict in an iteration is fixed in ae60cda.

📝 Checklist

The title is concise and informative.
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 and checked the documentation preview.

⌛ Dependencies

tobiasdiez · 2025-07-19T13:12:16Z

No idea about the failing tests in test-long....

orlitzky · 2025-07-19T13:45:24Z

I guess the CI is invoking pytest via the ./sage script and a doctest wrapper? I was just typing pytest like a crazy person.

tobiasdiez · 2025-07-19T16:09:27Z

I guess the CI is invoking pytest via the ./sage script and a doctest wrapper? I was just typing pytest like a crazy person.

Yes, pytest is called as part of sage -t (that was supposed to work already). I don't get the test failures in dev_tools.py - they seem to be unrelated.

user202729 · 2025-07-20T14:02:03Z

Maybe you should retrigger the CI (e.g. by pushing an empty commit) just in case.

tobiasdiez · 2025-07-24T19:15:30Z

Maybe you should retrigger the CI (e.g. by pushing an empty commit) just in case.

15f17bb is almost such an empty commit but didn't fix it. I've now opened #40474 for a minimally invasive solution.

sagemathgh-40474: Add pytest command to run tests in CI workflow       Alternative to sagemath#40446. This time running pytest directly in the CI, instead of as part of `sage -t`. Removed one old test file that fails with meson since the sage cli is different then. ### 📝 Checklist  - [ ] The title is concise and informative. - [ ] 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 and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#40474 Reported by: Tobias Diez Reviewer(s):

github-actions · 2025-09-28T06:17:40Z

Documentation preview for this PR (built with commit adcba7d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

tobiasdiez · 2025-09-28T09:56:05Z

This is now finally working. @user202729 @orlitzky please have a look.

(CI now actually runs the pytests twice: once as we call pytest directly, and once as part of sage -t. This shouldn't be a big issue as the tests complete in very short time. And it makes sure that the pytests are still working even when run from the sage env, so this should give additional security in the setup.)

user202729 · 2025-09-28T10:58:04Z

Hm… I'm not entirely sure what this does, it would be useful to explain what exactly does pytest run on. (methods start with test_ in the file? doctests? methods start with test_ in the whole repository?)

Though it's probably better to answer this in a docstring or rst file edit instead of a comment.

tobiasdiez · 2025-09-30T11:11:59Z

It's the same as running pytest directly. So, essentially _test.py files and then method with test_ therein.

This PR also doesn't introduce anything new. The functionality to run pytest as part of sage -t is there for some time, it just got broken/disabled by moving conftest.py to the root directory.

user202729 · 2025-09-30T14:47:54Z

I'm not entirely sure that's the right thing to do. Previously sage -t a.py runs doctest in a.py, now it runs doctest in a.py plus *_test.py file in the whole sage directory? At least if it runs a_test.py only I'd call it reasonable.

Plus does it work only from sage root, or any directory?

tobiasdiez · 2025-09-30T15:19:06Z

I'm not entirely sure that's the right thing to do. Previously sage -t a.py runs doctest in a.py, now it runs doctest in a.py plus *_test.py file in the whole sage directory? At least if it runs a_test.py only I'd call it reasonable.

Yes only _test files are passed on to pytest if you specify a file. To cite from the code:

        # Do not run pytest on individual Python files unless
        # they match the pytest file pattern.  However, pass names
        # of directories.

orlitzky · 2025-10-05T12:11:10Z

The dev_tools.py error must have something to do with pytest, I also had to fix it in #40971 to get the CI to pass.

orlitzky · 2025-10-05T12:18:24Z

src/sage/doctest/__main__.py

+        # 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


This is cute but we should probably use os.path.isdir() or not os.path.exists().

Do you mean adcba7d?

I don't remember what the original was doing but it looks fine now.

Okay, thanks. So this is ready to be merged or do you have any other suggestions/concerns?

Yeah, let's do it, thanks.

Thanks for the review!

orlitzky · 2025-10-05T12:44:40Z

Aside from that one small refactoring for clarity, this LGTM, but I'm not sure if we should keep the separate pytest CI job. I've been moving slow doctests to pytest, and as they accumulate, it's going to become less and less reasonable to re-run several minutes' worth of tests.

tobiasdiez · 2026-01-16T16:48:14Z

I'm not sure if we should keep the separate pytest CI job. I've been moving slow doctests to pytest, and as they accumulate, it's going to become less and less reasonable to re-run several minutes' worth of tests.

Both is fine with me. Maybe we revisit this later once more tests are converted to pytest? I also hope that in the not so far future we can simply use pytest to also run the doctests (#36981).

sagemathgh-40446: Reenable pytest again as part of `sage -t`       Pytest used to run on as part of `sage -t`, but this is no longer the case. Here, we reactivate it by removing some safe-guards that are no longer necessary as pytest is a standard package now. Moreover, a "runtime error" due to a changing dict in an iteration is fixed in sagemath@ae60cdadd52 234800ce20f13071b99842010d224. ### 📝 Checklist  - [ ] The title is concise and informative. - [ ] 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 and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#40446 Reported by: Tobias Diez Reviewer(s): Michael Orlitzky, Tobias Diez

Run pytest again on CI

9aa08c1

github-actions bot added the s: needs review label Jul 19, 2025

tobiasdiez added 2 commits July 19, 2025 13:16

Fix notebook cmd test

599cf4d

Localize imports again

15f17bb

tobiasdiez mentioned this pull request Jul 19, 2025

Run function field TestSuites via pytest #40443

Merged

tobiasdiez mentioned this pull request Jul 24, 2025

Add pytest command to run tests in CI workflow #40474

Merged

5 tasks

Merge branch 'develop' into pytest-ci

8fd28f7

tobiasdiez added 2 commits September 28, 2025 11:22

Fix RuntimeError: dictionary changed size during iteration

ae60cda

Merge remote-tracking branch 'upstream/develop' into pytest-ci

ab4902b

tobiasdiez changed the title ~~Run pytest again on CI~~ Reenable pytest again as part of sage -t Sep 28, 2025

orlitzky reviewed Oct 5, 2025

View reviewed changes

tobiasdiez and others added 5 commits October 28, 2025 11:38

Merge remote-tracking branch 'upstream/develop' into pytest-ci

1e280af

Merge branch 'develop' into pytest-ci

115da88

Merge branch 'develop' into pytest-ci

1094f0d

Merge branch 'develop' into pytest-ci

aa9cb11

use path.exists over path.isfile in doctest collector

adcba7d

orlitzky added s: positive review and removed s: needs review labels Jan 17, 2026

vbraun merged commit 36a5469 into sagemath:develop Jan 25, 2026
27 of 28 checks passed

github-actions bot removed the s: positive review label Jan 25, 2026

Uh oh!

Conversation

tobiasdiez commented Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Checklist

⌛ Dependencies

Uh oh!

tobiasdiez commented Jul 19, 2025

Uh oh!

orlitzky commented Jul 19, 2025

Uh oh!

tobiasdiez commented Jul 19, 2025

Uh oh!

user202729 commented Jul 20, 2025

Uh oh!

tobiasdiez commented Jul 24, 2025

Uh oh!

github-actions bot commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez commented Sep 28, 2025

Uh oh!

user202729 commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

user202729 commented Sep 30, 2025

Uh oh!

tobiasdiez commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlitzky commented Oct 5, 2025

Uh oh!

orlitzky Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

orlitzky Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

orlitzky Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Jan 17, 2026

Choose a reason for hiding this comment

Uh oh!

orlitzky commented Oct 5, 2025

Uh oh!

tobiasdiez commented Jan 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tobiasdiez commented Jul 19, 2025 •

edited

Loading

github-actions bot commented Sep 28, 2025 •

edited

Loading

user202729 commented Sep 28, 2025 •

edited

Loading

tobiasdiez commented Sep 30, 2025 •

edited

Loading

tobiasdiez commented Sep 30, 2025 •

edited

Loading