sage.numerical.backends: Replace use of TestSuite by pytest

[mkoeppe]

sage.numerical.backends provides an opportunity to study use of pytest as a replacement for TestSuite (proposed in #30738) in a small self-contained codebase.

GenericBackend defines an API and a small number of _test_ methods to check that the API is implemented correctly by the backend implementations. We do this at the example of _test_ncols_nonnegative, leaving the other test methods for follow-up tickets.

Important design constraint:

• The testing framework must be extensible by user-defined classes (or classes from separately installed Python packages - such as the existing sage_numerical_backends_coin).

This is accomplished by introducing a new GenericBackendTests class which defines the checks that should hold for all backend implementations. Then for each backend we introduce a class that derives from GenericBackendTests and returns in the backend test fixture the backend under test.

The pytests are now also executed in sage -t after the doctests, e.g. bin/sage-runtests --verbose . prints at the end

===================================================================================================================== test session starts ======================================================================================================================
platform linux -- Python 3.8.6, pytest-6.2.1, py-1.10.0, pluggy-0.13.1 -- /mnt/d/Programming/sage/src/.venv/bin/python3
cachedir: .pytest_cache
rootdir: /mnt/d/Programming/sage/src
collected 10 items                                                                                                                                                                                                                                             

sage/numerical/backends/cvxopt_backend_test.py::TestCVXOPTBackend::test_old_testsuite PASSED                                                                                                                                                             [ 10%]
sage/numerical/backends/cvxopt_backend_test.py::TestCVXOPTBackend::test_ncols_nonnegative PASSED                                                                                                                                                         [ 20%]
sage/numerical/backends/glpk_backend_test.py::TestGLPKBackend::test_ncols_nonnegative PASSED                                                                                                                                                             [ 30%]
sage/numerical/backends/glpk_backend_test.py::TestGLPKBackend::test_old_testsuite PASSED                                                                                                                                                                 [ 40%]
sage/numerical/backends/glpk_exact_backend_test.py::TestGLPKExactBackend::test_ncols_nonnegative PASSED                                                                                                                                                  [ 50%]
sage/numerical/backends/glpk_exact_backend_test.py::TestGLPKExactBackend::test_old_testsuite PASSED                                                                                                                                                      [ 60%]
sage/numerical/backends/interactivelp_backend_test.py::TestInteractiveLPBackend::test_ncols_nonnegative PASSED                                                                                                                                           [ 70%]
sage/numerical/backends/interactivelp_backend_test.py::TestInteractiveLPBackend::test_old_testsuite PASSED                                                                                                                                               [ 80%]
sage/numerical/backends/ppl_backend_test.py::TestPPLBackend::test_ncols_nonnegative PASSED                                                                                                                                                               [ 90%]
sage/numerical/backends/ppl_backend_test.py::TestPPLBackend::test_old_testsuite PASSED                                                                                                                                                                   [100%]

Depends on #30901
Depends on #31003
Depends on #30551

CC: @tobiasdiez @nthiery @tscrim @fchapoton @yuan-zhou

Component: doctest framework

Author: Tobias Diez, Matthias Koeppe

Branch: 0592226

Reviewer: Matthias Koeppe, Tobias Diez

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

[tobiasdiez]

comment:1

What should happen with the generic tests methods like _test_new, _test_not_implemented_methods, _test_category, _test_pickling that sadly make the "small self-contained" part hard?

[mkoeppe]

comment:2

Great point. This brings me to the question whether it would be possible to either

• invoke the existing TestSuite as one pytest method or
• dynamically generate adapter methods that invoke the existing _test methods.

[tobiasdiez]

Dependencies: #30901, #31003

[tobiasdiez]

Branch: public/refactoring/pytest-numerical-backends

[sagetrac-git]

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

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
4cdf94d	Merge branch 'public/build/multiarchsimple' of git://trac.sagemath.org/sage into public/refactoring/pytest-numerical-backends
5fd49b7	Migrate some tests to pytest
719e442	Run pytest as part of sage -t

[sagetrac-git]

Commit: 719e442

[tobiasdiez]

comment:6

Replying to @mkoeppe:

• invoke the existing TestSuite as one pytest method or

That's a great idea, making it possible to step-by-step migrate more test methods without the need to do everything at once! I've used it now.

• dynamically generate adapter methods that invoke the existing _test methods.

That might also work, but is more work since the _test methods are instance methods, and for pytest the adapter method would need to be provided with the instance (which in the current design is passed to the TestSuite). It's probably easier to simply rewrite the test method in pytest.

[mkoeppe]

comment:7

Thanks a lot!

[mkoeppe]

comment:8

Some quick remarks (I haven't had a chance to test it yet):

• I'd suggest to rename test_old_testsuite to something more specific like test_sage_unittest_testsuite.
• In the addition to src/bin/sage-runtests - this needs to work without error if pytest is not installed (currently it is only an optional package - see Make pytest a standard package #31110)

[mkoeppe]

Author: Tobias Diez

[mkoeppe]

comment:10

Also, perhaps src/bin/sage-runtests should only invoke pytest if it is testing directories/packages that contain pytest tests (any heuristic for checking this is fine).

[sagetrac-git]

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

d1f14f0

Incorporate feedback

[sagetrac-git]

Changed commit from 719e442 to d1f14f0

[tobiasdiez]

comment:12

Replying to @mkoeppe:

Some quick remarks (I haven't had a chance to test it yet):

• I'd suggest to rename test_old_testsuite to something more specific like test_sage_unittest_testsuite.
• In the addition to src/bin/sage-runtests - this needs to work without error if pytest is not installed (currently it is only an optional package - see Make pytest a standard package #31110)

Thanks! Done.

[tobiasdiez]

comment:13

Replying to @mkoeppe:

Also, perhaps src/bin/sage-runtests should only invoke pytest if it is testing directories/packages that contain pytest tests (any heuristic for checking this is fine).

Mhh, pytest has its own test discovery bult-in, so I think its fine to just pass the path to it and let it look for the files. What advantages do you see of using a custom heuristic beforehand?

[mkoeppe]

comment:14

Replying to @tobiasdiez:

What advantages do you see of using a custom heuristic beforehand?

When pytest is not installed and you run sage -t on all of Sage, it should warn that the pytest-dependent tests are not run. (Your latest commit does that.)

But it should probably not warn if a user invokes sage -t on a part of Sage that does not use pytest yet -- to be less annoying.

[mkoeppe]

comment:15

Shouldn't test_sage_unittest_testsuite be provided by something more general than GenericBackendTests? (in other words, should GenericBackendTests not derive from some other class?)

[tobiasdiez]

comment:16

Replying to @mkoeppe:

Shouldn't test_sage_unittest_testsuite be provided by something more general than GenericBackendTests? (in other words, should GenericBackendTests not derive from some other class?)

Good idea for later purposes. However, it doesn't remove the test_sage_unittest_testsuite method in GenericBackendTests since all backend tests ignore test_pickling, so this still has to be done there.

[sagetrac-git]

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

f4a0c4a

Create SageObJectTest

[sagetrac-git]

Changed commit from d1f14f0 to f4a0c4a

[mkoeppe]

comment:18

Great. Could you add a docstring for test_sage_unittest_testsuite to explain that "subclasses can override this method if they need to skip some tests".

We should also clarify whether ..._test.py should be exempt from our rule that every function needs a docstring and a doctest. If so, probably the patchbot will need some adjustments so it will not complain.

[mkoeppe]

comment:35

Replying to @tobiasdiez:

Maybe https://docs.pytest.org/en/stable/pythonpath.html helps.

Yes, it did!

[mkoeppe]

comment:37

The only remaining problem that I have is that I need to update the 3 standalone packages providing additional backends (https://github.com/mkoeppe/sage-numerical-backends-coin, ...) to also invoke pytest - and to do make sure this does not cause errors on older Sage versions.

[mkoeppe]

comment:38

To keep the work limited, perhaps it's best to keep the old _test_ methods so that the test coverage of the backend packages is not sacrificed. We will delete the _test_ methods later.

[sagetrac-git]

Changed commit from c81c8db to 0592226

[sagetrac-git]

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

0592226

src/sage/numerical/backends/generic_backend.pyx: Restore _test_ncols_nonnegative, with comment

[mkoeppe]

Reviewer: Matthias Koeppe, ...

[mkoeppe]

Changed author from Tobias Diez to Tobias Diez, Matthias Koeppe

[tobiasdiez]

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Tobias Diez

[tobiasdiez]

comment:41

Thanks a lot for having a look at this. Your changes look good to me!

[mkoeppe]

comment:42

Adding #30551 as a dependency because of from future import annotations. Or can we avoid it?

[mkoeppe]

Changed dependencies from #30901, #31003 to #30901, #31003, #30551

[vbraun]

Changed branch from public/refactoring/pytest-numerical-backends to 0592226

[saraedum]

Changed commit from 0592226 to none

[saraedum]

comment:44

This seems to break our workflow at https://github.com/flatsurf/e-antic. We are running some SageMath doctests with sage-runtests. The tests all pass and then we see:

collected 0 items

====================================================== no tests ran in 0.00s ======================================================

The problem is that this now returns with exit code 5, i.e., the CI assumes that the tests have failed.

[saraedum]

comment:45

A workaround is to drop a test_nothing.py file in the directory where the doctests are run:

def test_nothing():pass

[mkoeppe]

comment:46

Help with fixing this properly is welcome. The developer who worked on the pytest stuff has disappeared

[tobiasdiez]

comment:47

A proper fix would probably be to add an additional if check if the pytest return code is 5 here:

+    if err == 0:
+        sys.exit(exit_code_pytest)
+    else:
+        sys.exit(err)

See also the discussion in pytest-dev/pytest#2393.

I sadly don't have the time to implement it though.

[saraedum]

comment:48

The workaround in #31103 comment:45 does not work if the tested module contains some sage metaclass machinery since pytest is unvoked with --import-mode importlib, there is an error because the module is not in sys.modules and this confuses nested_class.pyx.

In that case, there is an error and not only an error code 5.

  ==================================== ERRORS ====================================
  ____________________ ERROR collecting pyflatsurf/vector.py _____________________
  ../src/pyflatsurf/vector.py:285: in <module>
      class Vectors(UniqueRepresentation, Parent):
  sage/misc/nested_class.pyx:318: in sage.misc.nested_class.NestedClassMetaclass.__init__ (build/cythonized/sage/misc/nested_class.c:2314)
      ???
  E   KeyError: 'vector'

[saraedum]

comment:49

Another workaround is to just pretend that pytest is not installed:

$ cat no-pytest/pytest.py
raise ModuleNotFoundError
$ PYTHONPATH=no-pytest:$PYTHONPATH sage -tp ...

But I actually think we should make this a bit more convenient than having to hack the module search path.

[tobiasdiez]

comment:50

I guess the issue with the modules is the same as #31924.

[tobiasdiez]

comment:51

And the exit code 5 issue should be fixed with #32892.