Replace custom Sage unit test discovery by pytest

[tobiasdiez]

Currently, Sage code contains _test_xyz methods that are invoked via doctests and discovered via a custom TestSuite class. This ticket is to replace this custom test discovery by the use of the established pytest framework https://docs.pytest.org/ (this is in line with #28936 - using mainstream Python test infrastructure).

Advantages of this approach:

• Reuse standard test infrastructure instead of a custom test discovery interface (this is a big maintenance relief, especially given the large list of todos in the TestSuite code).
• Better error messages in case a test fails.
• Easier onboarding of new Python developers.
• Clear separation of test methods vs production code. For example, the *_test.py files can easily be excluded from distribution.
• Good support by IDEs (auto completion, test discovery and easier invocation). For example, VSCode automatically discovers all pytest tests, provides a convenient interface to run them (all, a selection, only failed ones) and allows to directly start a debug session for a failing test. See https://code.visualstudio.com/docs/python/testing
• Less and clearer code (no hidden assumptions).

Disadvantages of this approach:

• Current developers have to learn pytest and its conventions (files must be named *_test.py and test methods must start with test_).
• Tests no longer live in the same file as the code (I would call this an advantage, but some people may prefer to have them really close together).

In order to migrate, the following changes are necessary:

• Move _test_xyz from a category module to new file <module>_test.py (e.g. finite_semigroups_test.py).
• Rewrite tests using pytest (which is straightforward and more or less only amounts to removing all the custom code involving self_tester).
• Create a new test class for the parent class under test (e.g TestFiniteSemigroup).
• Create a static method category_instances() in this test class returning a list of instances to test.

For the moment, I did this only for a few test methods in sets_cat using finite semigroups as example to illustrate the process.

The new implementation is as follows:

• The test class for the category (e.g. in sets_cat_test.py) defines generic tests that should pass for every object in that category. Moreover, the test class may provides instances (objects) to test via the method category_instances.
• Running pytest finds the test file finite_semigroups_test.py, then determines the categories of the objects (returned from category_instances) and adds all generic test methods for that category to the test class (this happens in conftest.py).
• Each test method can have the parameter category_instance which is bound to the return value of the category_instances method.

Running cd src && pytest --verbose (from a virtual env with sage and pytest installed), the output is as follows:

sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_contains[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                         [  2%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_contains[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                    [  4%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_contains[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                               [  6%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_iter_list[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                        [  8%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_iter_list[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                   [ 10%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_iter_list[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                              [ 13%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_iter_cardinality[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                 [ 15%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_iter_cardinality[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                            [ 17%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_enumerated_set_iter_cardinality[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                       [ 19%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_associativity[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                                   [ 21%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_associativity[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                              [ 23%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_associativity[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                                         [ 26%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_an_element[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                                      [ 28%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_an_element[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                                 [ 30%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_an_element[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                                            [ 32%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_an_element_idempotent[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                           [ 34%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_an_element_idempotent[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                      [ 36%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_an_element_idempotent[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                                 [ 39%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_cardinality_return_type[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                         [ 41%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_cardinality_return_type[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                    [ 43%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_cardinality_return_type[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                               [ 45%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_construction[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                                    [ 47%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_construction[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                               [ 50%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_construction[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                                          [ 52%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_element_eq_reflexive[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                            [ 54%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_element_eq_reflexive[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                       [ 56%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_element_eq_reflexive[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                                  [ 58%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_eq_symmetric[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                           [ 60%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_eq_symmetric[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                      [ 63%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_eq_symmetric[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                                 [ 65%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_eq_transitive[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                          [ 67%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_eq_transitive[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                     [ 69%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_eq_transitive[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                                [ 71%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_neq[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED                                                                    [ 73%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_neq[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED                                                               [ 76%]
sage/categories/finite_semigroups_test.py::TestFiniteSemigroup::test_elements_neq[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c', 'd')] PASSED                                                          [ 78%]
...

As one can see, each general test method is invoked for the three examples.

TODO (in follow-up tickets):

• Complete migration to pytest.
• Once that is done, stop invoking TestSuite in doctests and remove the TestSuite class.

See also:

• Use pytest to run doctests #33546 Replace custom Sage doctest discovery by pytest (using pytest_collect_file)

CC: @mkoeppe @tscrim @nthiery @slel

Component: refactoring

Keywords: testsuite

Author: Tobias Diez

Branch/Commit: public/refactoring/pytest @ b047abe

Issue created by migration from https://trac.sagemath.org/ticket/30738

[mkoeppe]

comment:2

Are you aware that the crucial point of our _test methods, in particular those provided in the category framework, is that objects of subclasses run these tests?

[tobiasdiez]

comment:3

Yes, and this structure is also somewhat reflected in the new code as the tests for the subclasses inherit from the general base test class. If I understood the code correctly, then the main purpose of these subclasses (with respect to the tests) is to provide the examples (e.g. using the example() convention). This can be archived in a more transparent and cleaner way using parameterized test fixtures of pytest.

[mkoeppe]

comment:4

Replying to @tobiasdiez:

If I understood the code correctly, then the main purpose of these subclasses (with respect to the tests) is to provide the examples (e.g. using the example() convention).

No, the category classes provide mixins for dynamically generated classes for all Parent and Element objects. The example() are just additional documentation.

[tobiasdiez]

comment:5

Sure, but that doesn't seem to be important for the tests itself. Take for example the tests in sets_cat. Almost all of them simply call tester.some_elements() and then run certain assertions against the returned elements. Thus, there is no semantical difference to having one general test method accepting an element as argument, and then testing the elements returned by some_elements() using this method. This is exactly what is proposed in this ticket.

One question though: is the purpose of these tests to check that one general implementation is correct for all these examples (e.g sets_cat provides a general equality implementation which is checked), or that the subclasses provide an implementation that adheres to general principles (e.g. finitely_generated_semigroups provides an implementation of equality that is checked). Just so that I know where to best place these tests (test_sets_cat vs test_finitely_generated_semigroups).

[mkoeppe]

comment:6

Replying to @tobiasdiez:

One question though: is the purpose of these tests to check that one general implementation is correct for all these examples (e.g sets_cat provides a general equality implementation which is checked), or that the subclasses provide an implementation that adheres to general principles (e.g. finitely_generated_semigroups provides an implementation of equality that is checked).

The latter.

[tobiasdiez]

comment:7

Thanks, I thought so too. Then the current file structure should be right.
Should I go ahead and convert the other test methods to pytest as well? (As part of this ticket, or open a new ticket for every method?)

[mkoeppe]

comment:8

I do not know enough about pytest. Can you expand the ticket description to explain how the test discovery works after the ticket, and how it ensures that still the same things get tested?

[tobiasdiez]

comment:11

I've extended the ticket description. Does this clarify your questions?

The test test_element_eq_reflexive (and other similar tests to be added) are invoked for each element of test_elements. Thus, by adding all the instances currently covered by the unit tests we make sure that the test coverage stays the same.

[mkoeppe]

comment:12

Sorry, I still don't get it. The branch as it is on the ticket - do you claim that it still tests the same objects as before?

[tobiasdiez]

comment:13

If you run pytest on this branch, it invokes test_elements_eq_reflexive with the arguments ['a', 'b', 'c', 'ab', 'ac', 'ba', 'bc', 'ca', 'cb', 'abc'].
This is the same as the doctest for finite_semigroups:

Now, let us look at the structure of the semigroup::

        sage: S = FiniteSemigroups().example(alphabet = ('a','b','c'))
        sage: S.cayley_graph(side="left", simple=True).plot()
        Graphics object consisting of 60 graphics primitives
        sage: S.j_transversal_of_idempotents() # random (arbitrary choice)
        ['acb', 'ac', 'ab', 'bc', 'a', 'c', 'b']

    We conclude by running systematic tests on this semigroup::

        sage: TestSuite(S).run(verbose = True)
        running ._test_an_element() . . . pass
        running ._test_associativity() . . . pass
        running ._test_cardinality() . . . pass
        running ._test_category() . . . pass
        running ._test_construction() . . . pass
        running ._test_elements() . . .
          Running the test suite of self.an_element()
          running ._test_category() . . . pass
          running ._test_eq() . . . pass
          running ._test_new() . . . pass
          running ._test_not_implemented_methods() . . . pass
          running ._test_pickling() . . . pass
          pass
        running ._test_elements_eq_reflexive() . . . pass
        running ._test_elements_eq_symmetric() . . . pass
        running ._test_elements_eq_transitive() . . . pass
        running ._test_elements_neq() . . . pass
        running ._test_enumerated_set_contains() . . . pass
        running ._test_enumerated_set_iter_cardinality() . . . pass
        running ._test_enumerated_set_iter_list() . . . pass
        running ._test_eq() . . . pass
        running ._test_new() . . . pass
        running ._test_not_implemented_methods() . . . pass
        running ._test_pickling() . . . pass
        running ._test_some_elements() . . . pass

So the idea would be to replace this doctest by pytest. There are a few more doctests of this form that can be replaced in a similar vain by adding the corresponding elements to the test_elements list.

[mkoeppe]

comment:14

Replying to @tobiasdiez:

There are a few more doctests of this form that can be replaced in a similar vain by adding the corresponding elements to the test_elements list.

A few? Currently _test_elements_eq_reflexive is invoked by TestSuite(object).run() whenever object happens to be in the category of sets. This is a huge, dynamically determined list.

[tobiasdiez]

comment:15

Sure, there are quite a lot of instances where a TestSuite is run from the doctest. Each one would need to be replaced by a test file similar to test_finite_semigroups.py. Pytest is not a magician and the information which examples to test still needs to be provided.

So the replacement dictionary roughly speaking looks like:

• TestSuite -> pytest
• TestSuite(S).run(verbose = True) -> @pytest.fixture(params=S)

The idea is not to change the way the tests cases are provided (although that may certainly be improved in the future) but to replace sage's only implementation of test discovery and invocation by what is probably the most establish Python testing framework.

[mkoeppe]

comment:16

So in your design, there would be a hierarchy of Tests classes parallel to the hierarchy of, say, parent classes in sage?

[tobiasdiez]

comment:17

Yes! Every parent class would have a corresponding test class which specifies which general axioms this parent class wants to adhere to. This is done by deriving from one or more generic test classes (the one in the branch should probably be renamed to GenericSetTests).

[mkoeppe]

comment:18

Okay, but the parent classes are dynamic classes formed by mixing in axioms.
So the test classes would also be determined dynamically?

[mkoeppe]

comment:19

Replying to @tobiasdiez:

here are quite a lot of instances where a TestSuite is run from the doctest. Each one would need to be replaced by a test file similar to test_finite_semigroups.py. Pytest is not a magician and the information which examples to test still needs to be provided.

Thanks for this clarification. I'm setting this ticket to "needs_work" because the branch in this form is not complete -- it effectively removes existing tests for _test_elements_eq_reflexive. Without a much more complete branch that reaches test parity, this idea is difficult to evaluate.

Overall I think there is a conflict here between the dynamic nature of our category system and what seems to amount to a static determination of what objects to test for what properties (in the proposed changes).

[tobiasdiez]

comment:20

I'm not that familiar yet with the category framework. When you say "dynamic classes formed by mixing in axioms" you mean something like Parent.__init__(self, category = Semigroups().Finite().FinitelyGenerated())? But, in general, yes the idea is to mixin the tests. I don't see any reason why this couldn't be done dynamically.

I agree, this branch has not yet feature parity with the master branch. Before I invest more time into it, I just wanted to confirm that this idea would be worth looking at in general.

[mkoeppe]

comment:21

Replying to @tobiasdiez:

When you say "dynamic classes formed by mixing in axioms" you mean something like Parent.__init__(self, category = Semigroups().Finite().FinitelyGenerated())?

Yes, things like this.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

1df0a95	Merge branch 'develop' of git://github.com/sagemath/sage into public/refactoring/pytest
b12608b	Readd test
79abf0d	Refactor
99ae00c	Replace startup exception by warning
11882e5	Use context manager
6a52fbf	Remove lazy import finish startup
f96025e	Merge branch 'develop' of git://github.com/sagemath/sage into public/build/startupWarning
b952bb5	Fix doctests
5de08cd	Merge branch 'public/build/startupWarning' of git://trac.sagemath.org/sage into public/refactoring/pytest
e198d1c	Make tests mixins dynamic

[slel]

Changed keywords from none to testsuite

[tobiasdiez]

comment:74

Why did you change the milestone? I don't see anything that would prevent this being merged soon (except for the missing review).

[mkoeppe]

comment:75

There's been no activity, and it's not clear if there are any developers who would be interested in rewriting the entirety of our test suite.

[tobiasdiez]

comment:76

While being a relatively huge project, I think, a completely migration can be done in a relatively straightforward way with an overseeable time investment. I would definitely up for doing this alone if necessary. So I'm appreciate if this ticket could be reviewed soon. Thanks!

[mkoeppe]

comment:77

You have received review comments: comment:45, comment:48, etc.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

36cfdfc	Merge remote-tracking branch 'origin/develop' into public/refactoring/pytest
fab5793	Move test_associativity

[sagetrac-git]

Changed commit from 161e2f4 to fab5793

[tobiasdiez]

comment:80

Replying to @mkoeppe:

You have received review comments: comment:45, comment:48, etc.

I've fixed comment:45. To comment:48 I've already reacted in comment:49.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

b047abe

Merge branch 'develop' into public/refactoring/pytest

[sagetrac-git]

Changed commit from fab5793 to b047abe

[mkoeppe]

Branch has merge conflicts