Replace custom Sage unit test discovery by pytest

tobiasdiez commented 3 years ago

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:

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

tobiasdiez commented 3 years ago

Description changed:

--- 
+++ 
@@ -15,6 +15,7 @@
 - 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 IDE (auto completion, test discovery and easier invocation)
+- Less and clearer code (no hidden assumptions)

 Disadvantages of this approach:
 - Current developer have to learn the new conventions (files need to be named `test_*.py` and tests methods need to start with `test_`) and pytest

mkoeppe commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Description changed:

--- 
+++ 
@@ -21,6 +21,15 @@
 - Current developer have to learn the new conventions (files need to be named `test_*.py` and tests methods need to start with `test_`) and pytest
 - Tests no longer live in the same file as the code (I would say this is an advantage, but some people may prefer to have them really close together).

+By default, pytest implements the following standard test discovery:
+
+- Search for `test_*.py` or `*_test.py` files (I would stick to the `test` prefix).
+- From those files, collect test items:
+   - Functions or methods with prefix `test`
+   - If the functions are inside of classes, then the class name has to start with `test` as well.

 What do you think? (Please feel free to add more people to cc if you think they may be interested.)

+TOOD:
+- Complete migration of tests to pytest
+- Integrate `pytest` in `sage -t` and tox (and patchbot?)

tobiasdiez commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 3b243d0 to e198d1c

tobiasdiez commented 3 years ago

Dependencies: #30748

tobiasdiez commented 3 years ago

comment:23

Thanks for the input. I've now reworked the code completely. The generic category tests are now dynamically mixed in the test class. The new approach is explained in the ticket description in more detail. The upshot is that it is very easy to add new generic test methods (simply add them to the category test class, e.g. test_sets_cat) and to add new examples (simply create a new test class implementing category_instances).

tobiasdiez commented 3 years ago

Description changed:

--- 
+++ 
@@ -1,13 +1,4 @@
 Currently, the sage code contains `_test_xyz` methods that are invoked via doctests and discovered via a custom `TestSuit` class. In this ticket, we are proposing to replace this custom designed test discovery by using the established pytest framework https://docs.pytest.org/ (this is inline with #28936 - using mainstraim Python test infrastructure).
-
-The changes are as follows:
-- Move `_test_xyz` from `module` to new file `test_module`
-- Rewrite test using pytest (which is straightforward and more or less only amounts to removing all the custom code involving `self_tester`)
-- Remove invoking the `TestSuit` in the doctests
-- Instead, add the corresponding test elements as parameterized fixtures for pytest
-
-For the moment, I did this only for one test method in `sets_cat` to illustrate the process.
-What is still remaining is to call `pytest` as part of the build process so that the newly added tests are found and checked.

 Advantages of this approach:
 - Reuse standard test infrastructure instead of a custom test discovery interface (this is a big plus as we do not have to maintain it ourself, especially given the large list of todo's in the TestSuite code)
@@ -21,6 +12,37 @@
 - Current developer have to learn the new conventions (files need to be named `test_*.py` and tests methods need to start with `test_`) and pytest
 - Tests no longer live in the same file as the code (I would say this is 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 `test_module` (e.g. `test_sets_cat`)
+- Rewrite test 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 `test_finite_semigroups`)
+- 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 two test methods in `sets_cat` to illustrate the process.
+
+The new implementation is as follows:
+- The test class for the category (e.g. `test_sets_cat`) defines generic tests that should pass for every object in that category.
+- For every object in the category there is a test class (e.g. `test_finite_semigroups`) that provides instances to test via the method `category_instances`
+- Running pytest finds the test class `test_finite_semigroups`, then determines the categories of the object 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.
+- Thus, the output is as follows:
+
+```
+sage/categories/test_finite_semigroups.py::TestFiniteSemigroup::test_element_eq_reflexive[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED [ 25%]
+sage/categories/test_finite_semigroups.py::TestFiniteSemigroup::test_element_eq_reflexive[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED [ 50%]
+sage/categories/test_finite_semigroups.py::TestFiniteSemigroup::test_cardinality_return_type[An example of a finite semigroup: the left regular band generated by ('a', 'b')] PASSED [ 75%]
+sage/categories/test_finite_semigroups.py::TestFiniteSemigroup::test_cardinality_return_type[An example of a finite semigroup: the left regular band generated by ('a', 'b', 'c')] PASSED [100%]
+```
+
+
+TOOD (as follow-up tickets):
+- Complete migration of tests to pytest
+- Integrate `pytest` in `sage -t` and tox
+- Call `pytest` as part of the build process so that the newly added tests are found and checked
+- After the migration is done, remove invoking the `TestSuit` in the doctests and remove the `TestSuit` class.
+
+---
+
 By default, pytest implements the following standard test discovery:

 - Search for `test_*.py` or `*_test.py` files (I would stick to the `test` prefix).
@@ -28,8 +50,3 @@
    - Functions or methods with prefix `test`
    - If the functions are inside of classes, then the class name has to start with `test` as well.

-What do you think? (Please feel free to add more people to cc if you think they may be interested.)
-
-TOOD:
-- Complete migration of tests to pytest
-- Integrate `pytest` in `sage -t` and tox (and patchbot?)

mkoeppe commented 3 years ago

comment:24

It's really hard to see from this ticket how this is intended to go.

The previous version that I reviewed was removing existing doctests and reimplementing a small subset of them using pytest.

Now the new version is only adding some new tests using pytest that seem to duplicate some existing tests run by the doctest framework.

tobiasdiez commented 3 years ago

comment:25

The current version is meant as a starting point for discussion whether this approach is ok. So it is a fully working prototype that indeed reimplements a subset of the current category framework tests, and is flexible enough to be extended to cover everything. Getting full parity with the current category tests needs a bit of work by extending test_sets_cat with the remaining test functions.

I'm not sure what I should do now. The only open point is that pytest needs to be run during the normal test run (on user systems as well as on the ci pipeline). But that's not really the purpose of this ticket. I think, it's clear that one cannot reimplement the complete category tests in one ticket, so this is why I opted for a minimal version. What is your proposal to proceed from here?

tobiasdiez commented 3 years ago

comment:26

Feedback/review still needed.

mkoeppe commented 3 years ago

comment:27

Replying to @tobiasdiez:

Getting full parity with the current category tests needs a bit of work by extending test_sets_cat with the remaining test functions.

But even for the test function that you already added, it is only testing a few objects.

So to get parity even for this one test function, category_instances would have to be extended vastly -- if I understand the design correctly.

It is unclear who should do this work and why.

tobiasdiez commented 3 years ago

Changed dependencies from #30748 to #30748, #30901

tobiasdiez commented 3 years ago

comment:29

I've now vastly extended the tests, and replaced some of the TestSuite calls by pytest. A handful of test methods are still missing to get full feature parity (will do this later), but I hope the idea and design get clearer now (at the cost that there are bigger changes).

The following change is characteristic for the aim of migrating from Sage's custom TestSuite framework to pytest:

-        sage: TestSuite(C).run()
+        sage: import pytest
+        sage: pytest.main(["-r", "test_finite_semigroups.py"])

(calling pytest from the doctests is only a temporary work-around until it's properly integrated into the build chain)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

`94e20c7`	`Revert some of the changes`
`6dd6e5c`	`Fix compilation`
`eceefb3`	`Remove string wrap`
`d345bff`	`Fix test`
`c47c4bf`	`Correct indent`
`3fcaf5f`	`Merge branch 'develop' of git://github.com/sagemath/sage into public/build/multiarchsimple`
`090e6f1`	`Simplify code`
`fa4556a`	`Remove _get_sage_local`
`7b8be0d`	`Merge branch 'public/build/multiarchsimple' of git://trac.sagemath.org/sage into public/refactoring/pytest`
`1488582`	`Towards full feature parity`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from e198d1c to 1488582

mkoeppe commented 3 years ago

comment:31

Replying to @mkoeppe:

But even for the test function that you already added, it is only testing a few objects.

So to get parity even for this one test function, category_instances would have to be extended vastly -- if I understand the design correctly.

These changes do not seem to address the above comment? You added more test functions, instead of showing how one would achieve parity for what one test function covers. Or maybe I'm misunderstanding.

tobiasdiez commented 3 years ago

comment:32

I guess there are two ways for migration:

Replace one test in the category framework by a pytest, and then make sure that this test is invoked for every example. Do this again for every test.
Duplicate all tests in the category framework using pytest, and then replace one TestSuite(...).run statement by its pytest equivalent. Do this again for every test invocation.

Since there are only a small number of tests (order of 10) but way more examples (order of 1000), my idea was follow the second approach since this can be done easier in steps.

Of course, the complete migration is quite a huge project. This ticket was meant to as a starting point.

The advantages I see of pytest over a custom testsuite implementation are in the ticket description.

mkoeppe commented 3 years ago

comment:33

If I understand the proposed design correctly, for each category you would manually maintain a list of category instances to test. To me this seems like a huge step backward: The categories of an object are computed dynamically - and thus all test methods that come with the categories are run.

mkoeppe commented 3 years ago

comment:34

(Feel free to add this to "disadvantages of the approach" in the ticket description.)

mkoeppe commented 3 years ago

comment:35

From "advantages" in the ticket description:

Clear separation of test methods vs production code. For example, the test_*.py files can easily be excluded from distribution.

We definitely would not want to exclude them from the distribution! Also user-defined classes will want to run the test methods that are provided by the categories as part of their tests.

mkoeppe commented 3 years ago

comment:36

Could you elaborate on this in the ticket description:

Good support by IDE (auto completion, test discovery and easier invocation)

tobiasdiez commented 3 years ago

comment:37

Replying to @mkoeppe:

If I understand the proposed design correctly, for each category you would manually maintain a list of category instances to test.

Yes, but that's not different from the current code. There you also create the examples and then run the testsuite (which makes sure to run the correct tests for this category). This is the same in the proposed design, except that you now define the examples in the test file. I would even argue that the proposed design is more flexible and makes it very transparent which examples are used to test a given category.

tobiasdiez commented 3 years ago

comment:38

Replying to @mkoeppe:

Could you elaborate on this in the ticket description:

Good support by IDE (auto completion, test discovery and easier invocation)

Done.

tobiasdiez commented 3 years ago

Description changed:

--- 
+++ 
@@ -5,7 +5,7 @@
 - With this there also comes 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 IDE (auto completion, test discovery and easier invocation)
+- Good support by IDE (auto completion, test discovery and easier invocation). For example, VS code automatically discovers all pytest tests, provides a convenient interface to run them (all, subselection, 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:

tobiasdiez commented 3 years ago

comment:39

Replying to @mkoeppe:

From "advantages" in the ticket description:

Clear separation of test methods vs production code. For example, the test_*.py files can easily be excluded from distribution.

We definitely would not want to exclude them from the distribution! Also user-defined classes will want to run the test methods that are provided by the categories as part of their tests.

Probably depends on the context. People using sagelib only as a (calculation) library may not need these tests. One could for example easily split them out into a new sage.testing package (or sage.categories.testing) once everything is modularized nicely.

mkoeppe commented 3 years ago

comment:40

Replying to @tobiasdiez:

Replying to @mkoeppe:

If I understand the proposed design correctly, for each category you would manually maintain a list of category instances to test.

Yes, but that's not different from the current code. There you also create the examples and then run the testsuite (which makes sure to run the correct tests for this category). This is the same in the proposed design, except that you now define the examples in the test file.

I think you may be missing that the category of an object is not statically known but is computed by code.

tobiasdiez commented 3 years ago

comment:41

No, I'm not. The test_some_category.py says only "please check the category tests for the following examples" and pytest automatically determines based on the category of these examples which tests to run. In fact, the latter is done here

+def pytest_pycollect_makeitem(collector: PyCollector, name: str, obj: type):
+    if inspect.isclass(obj) and "category_instances" in dir(obj):
+        # Enrich test class by functions from the corresponding category test class
+        for category, category_test_class in categories_with_tests.items():
+            if category() in obj.category_instances()[0].category().all_super_categories():
+                methods= [method for method in dir(category_test_class)
+                        if not method.startswith('__') and callable(getattr(category_test_class, method))]
+            
+                for method in methods:
+                    setattr(obj, method, getattr(category_test_class, method))
+
+        return pytest.Class.from_parent(collector, name=name, obj=obj)
+    else:
+        return None

mkoeppe commented 3 years ago

comment:42

OK, but how is this compatible with your design to list the test objects in category_instances?

tobiasdiez commented 3 years ago

comment:43

The design is as follows. Say you have a category LieGroups combining the categories Manifolds and Groups. Then you:

Create manifolds_test and groups_test files, specifying the category tests that should hold for all objects of this category.
Create a liegroups_test file, specifying in category_instances the examples of Lie groups you want to test, say SO(3) and SU(2). Moreover, add test methods that are particular for the cateogory of Lie groups.

When run, the code then looks at these examples, determines that they are objects of the categories Manifolds, Groups and Lie Groups and collects all tests from manifolds_test, groups_test and liegroups_test.

This can already be seen in action in the code, where the finite semigroups inhert tests from the category of sets and of finite sets.

mkoeppe commented 3 years ago

comment:44

So in the end it does not matter at all in which method category_instances an object is put?

mkoeppe commented 3 years ago

comment:45

Replying to @tobiasdiez:

This can already be seen in action in the code, where the finite semigroups inhert tests from the category of sets and of finite sets.

But having test_associativity supplied by TestFiniteSemigroup surely can't be the final design?!

tobiasdiez commented 3 years ago

comment:46

Replying to @mkoeppe:

So in the end it does not matter at all in which method category_instances an object is put?

No, it doesn't really matter. One could also have one big test file listing all these examples. But I would advocate to have one test file for each category, because then one can easily also write additional tests (not related to the category framework). It's also consistent with the usual conventions where one usually has one test file for each module file.

sagemath / sage

Replace custom Sage unit test discovery by pytest #30738

33546 Replace custom Sage doctest discovery by pytest (using pytest_collect_file)