src/sage/libs/gap: move a few tests to pytest #40677
Conversation
orlitzky
force-pushed
the
pytest-for-libgap
branch
from
b83ab51 to
75a914b
Compare
src/sage/libs/gap/gap_test.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def tmpfile(): |
There was a problem hiding this comment.
is there nothing existing like this in pytest/sage? This feels oddly general.
There was a problem hiding this comment.
Pytest has a built-in tmpdir that does almost the same thing, but with a directory instead of a file. But considering pytest-dev/pytest#13669, this is safer for now.
There was a problem hiding this comment.
is there any other place fixtures such as this can be placed (conftest?) We will have to eventually move it there once the same fixture is needed in another module's test anyway.
There was a problem hiding this comment.
conftest.py works, done.
src/sage/libs/gap/gap_test.py
Outdated
|
|
||
| def test_gc_loop_1(): | ||
| r""" | ||
| First stress test for garbage collection in libgap. |
There was a problem hiding this comment.
I feel "[[first]] stress test" is highly redundant, mostly because there shouldn't be any need to modify the docstring if we want to reorder the tests or add something inbetween.
There was a problem hiding this comment.
I deleted "First", "Second", and "Third" from the docstrings. There's still a number in the function name, but I don't know what else to call them.
src/sage/libs/gap/gap_test.py
Outdated
| result = True | ||
| for i in range(300000): | ||
| n = libgap.Order(H1) | ||
| result |= (n == two) |
There was a problem hiding this comment.
Surely you mean &=. Still, it feels like bad practice to use bitwise operator for boolean. What about just assert n == two each time here?
There was a problem hiding this comment.
I did that first but then pytest -v prints OK three hundred thousand times.
There was a problem hiding this comment.
(The &= is fixed, thanks.)
| lis.Add(a ** 2) | ||
| lis.Add(b ** 2) | ||
| lis.Add(b * a) | ||
| assert True |
There was a problem hiding this comment.
isn't this assert useless?
There was a problem hiding this comment.
The original test did essentially the same thing. I can only assume that it's an "it doesn't segfault" test.
There are two files, src/sage/libs/gap/test{,_long}.py that contain
only tests for sage.libs.gap. These are doctests for no other reason
than because we had no other way to run them in the past. This commit
moves them to a new file, gap_test.py, that will be run by pytest
rather than the doctest runner.
In the process I've tried to explain what I think the tests are doing.
orlitzky
force-pushed
the
pytest-for-libgap
branch
from
75a914b to
d8dad41
Compare
|
sanity check, is pytest currently run on CI? If yes, approve if tests pass. |
Should be, since #40474 |
A temporary file that is deleted after the test has completed will be useful elsewhere.
These have been moved to pytest (gap/gap_test.py).
|
Documentation preview for this PR (built with commit 1b56421; changes) is ready! 🎉 |
|
CI fails are the usual suspects. Marking positive review, thanks. |
|
Nice, thanks a lot. I'm happy to see that there is finally a bit more interest in pytest 🦊 |
There are two files, `src/sage/libs/gap/test{,_long}.py` that contain
only tests for `sage.libs.gap`. These are doctests for no other reason
than because we had no other way to run them in the past. This commit
moves them to a new file, `gap_test.py`, that will be run by pytest
rather than the doctest runner.
In the process I've tried to explain what I think the tests are doing.
URL: sagemath#40677
Reported by: Michael Orlitzky
Reviewer(s): Michael Orlitzky, user202729
4 participants
There are two files,
src/sage/libs/gap/test{,_long}.pythat contain only tests forsage.libs.gap. These are doctests for no other reason than because we had no other way to run them in the past. This commit moves them to a new file,gap_test.py, that will be run by pytest rather than the doctest runner.In the process I've tried to explain what I think the tests are doing.