Rename test_... functions to avoid the naming scheme reserved by pytest

[mkoeppe]

sage -t throws errors on some files because pytest is configured to look for methods prefixed with test_ and treats them as pytest test functions/methods, which are then executed. However, sage's code base contains some functions that match this pattern without being pytests.

This leads to problems like

$ ./sage -tp src/sage/tests/cmdline.py
too many failed tests, not using stored timings
Running doctests with ID 2022-03-20-11-46-21-1dc8fb11.
Using --optional=4ti2,buckygen,ccache,cryptominisat,debugpy,e_antic,gap_packages,homebrew,igraph,jupymake,latte_int,libsemigroups,lidia,lrslib,meataxe,normaliz,pip,polytopes_db_4d,pynormaliz,sage,sage_spkg,texttable
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,nauty,palp,pandoc,pdf2svg,pdftocairo,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 1 file using 8 threads.
sage -t --random-seed=23402257756506608859254049173870803147 src/sage/tests/cmdline.py
...
=================================================================================== test session starts ===================================================================================
platform darwin -- Python 3.10.2, pytest-7.0.1, pluggy-1.0.0
rootdir: /Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src, configfile: tox.ini
collected 1 item                                                                                                                                                                          

src/sage/tests/cmdline.py E                                                                                                                                                         [100%]

========================================================================================= ERRORS ==========================================================================================
____________________________________________________________________________ ERROR at setup of test_executable ____________________________________________________________________________
file /Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/tests/cmdline.py, line 61
  def test_executable(args, input="", timeout=100.0, pydebug_ignore_warnings=False, **kwds):
E       fixture 'args' not found
>       available fixtures: add_imports, cache, capfd, capfdbinary, caplog, capsys, capsysbinary, doctest_namespace, monkeypatch, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory
>       use 'pytest --fixtures [testpath]' for help on them.

/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/tests/cmdline.py:61
================================================================================= short test summary info =================================================================================
ERROR src/sage/tests/cmdline.py::test_executable
==================================================================================== 1 error in 0.01s =====================================================================================

We fix it in this ticket as follows:

• If the offending function is a proper (sage) test function, then change its name from test_xyz to _test_xyz (which also marks them as private)
• If the offending function is a "real" user-faced function, then explicitly exclude it from pytests test discovery.

See #31924 for further discussion on different approaches to fix the same issue including their pros and cons.

This is preparation for #33546.

Depends on #32975
Depends on #33612
Depends on #33617

CC: @tobiasdiez

Component: refactoring

Author: Tobias Diez

Branch/Commit: public/tests/pytest_wrong_test_methods @ 7644854

Reviewer: Sebastian Oehms, Matthias Koeppe

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

[slel]

comment:2

Good thing pycheck and pyright let us use
check_... and right_...! : )

[tobiasdiez]

Branch: public/tests/pytest_wrong_test_methods

[tobiasdiez]

Commit: 7060ea1

[tobiasdiez]

Author: Tobias Diez

[tobiasdiez]

Changed dependencies from #31924 to none

[tobiasdiez]

New commits:

59cabaa	Make test methods private
ba26086	Hide remaining methods starting with test from pytest
3dc0f63	Cleanup cmdline
321bd9d	Revert changes in test_class_pickling
db1d505	Fix tests in sageinspect
a09d5ec	Fix test_long
7060ea1	Fix imports in modular decomposition

[soehms]

comment:4

Yo should have a look at:

sage -t --warn-long 126.1 --random-seed=123007730607096856939441742800394041101 src/sage/tests/cmdline.py
**********************************************************************
File "src/sage/tests/cmdline.py", line 313, in sage.tests.cmdline._test_executable
Failed example:
    out.find("All tests passed!") >= 0
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/tests/cmdline.py", line 316, in sage.tests.cmdline._test_executable
Failed example:
    ret
Expected:
    0
Got:
    2
**********************************************************************
1 item had failures:
   2 of 217 in sage.tests.cmdline._test_executable
    [216 tests, 2 failures, 190.78 s]

and

sage -t --warn-long 125.1 --random-seed=216213492016970842904386788649176794439 src/sage/rings/tests.py
**********************************************************************
File "src/sage/rings/tests.py", line 416, in sage.rings.tests.?
Failed example:
    sage.rings.tests._test_karatsuba_multiplication(ZZ, 6, 5, verbose=True, seed=42)
Expected:
    _test_karatsuba_multiplication: ring=Univariate Polynomial Ring in x over Integer Ring, threshold=2
    (2*x^6 - x^5 - x^4 - 3*x^3 + 4*x^2 + 4*x + 1)*(4*x^4 + x^3 - 2*x^2 - 20*x + 3)
      (16*x^2)*(-41*x + 1)
      (x^6 + 2*x^5 + 8*x^4 - x^3 + x^2 + x)*(-x^2 - 4*x + 3)
      (-x^3 - x - 8)*(-1)
      (x - 1)*(-x^5 + 3*x^4 - x^3 + 2*x + 1)
      (x^3 + x^2 + x + 1)*(4*x^3 + 76*x^2 - x - 1)
      (x^6 - 5*x^4 - x^3 + 6*x^2 + 1)*(5*x^2 - x + 4)
      (3*x - 2)*(x - 1)
      (21)*(14*x^5 - x^2 + 4*x + 1)
      (12*x^5 - 12*x^2 + 2*x + 1)*(26*x^4 + x^3 + 1)
Got:
    test_karatsuba_multiplication: ring=Univariate Polynomial Ring in x over Integer Ring, threshold=2
      (2*x^6 - x^5 - x^4 - 3*x^3 + 4*x^2 + 4*x + 1)*(4*x^4 + x^3 - 2*x^2 - 20*x + 3)
      (16*x^2)*(-41*x + 1)
      (x^6 + 2*x^5 + 8*x^4 - x^3 + x^2 + x)*(-x^2 - 4*x + 3)
      (-x^3 - x - 8)*(-1)
      (x - 1)*(-x^5 + 3*x^4 - x^3 + 2*x + 1)
      (x^3 + x^2 + x + 1)*(4*x^3 + 76*x^2 - x - 1)
      (x^6 - 5*x^4 - x^3 + 6*x^2 + 1)*(5*x^2 - x + 4)
      (3*x - 2)*(x - 1)
      (21)*(14*x^5 - x^2 + 4*x + 1)
      (12*x^5 - 12*x^2 + 2*x + 1)*(26*x^4 + x^3 + 1)
**********************************************************************
1 item had failures:
   1 of   7 in sage.rings.tests.?
    [54 tests, 1 failure, 20.02 s]

[sagetrac-git]

Changed commit from 7060ea1 to dec7b7b

[sagetrac-git]

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

dec7b7b

Fix doctest in rings

[tobiasdiez]

comment:6

@soehms, thanks for having a look. The first issue you mention also exists in develop and I made some progress in fixing it at #33550. The second issue is now fixed.

[mkoeppe]

comment:7

Testing with the command-line from the ticket description of #31924:

$ ./sage -tp src/sage/numerical/backends src/sage/symbolic/expression.pyx src/sage/manifolds/differentiable/symplectic_form_test.py 
too many failed tests, not using stored timings
Running doctests with ID 2022-03-24-12-10-39-2142352f.
Using --optional=4ti2,buckygen,ccache,cryptominisat,debugpy,e_antic,gap_packages,homebrew,igraph,jupymake,latte_int,libsemigroups,lidia,lrslib,meataxe,normaliz,pip,polytopes_db_4d,pynormaliz,sage,sage_spkg,texttable
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,nauty,palp,pandoc,pdf2svg,pdftocairo,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Sorting sources by runtime so that slower doctests are run first....
Doctesting 27 files using 8 threads.
sage -t --random-seed=241138219171579094251943417982594901397 src/sage/numerical/backends/interactivelp_backend.pxd
    [0 tests, 0.00 s]
[...]
sage -t --random-seed=241138219171579094251943417982594901397 src/sage/numerical/backends/interactivelp_backend.pyx
    [266 tests, 5.13 s]
sage -t --random-seed=241138219171579094251943417982594901397 src/sage/symbolic/expression.pyx
    [3070 tests, 35.91 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 36.7 seconds
    cpu time: 41.2 seconds
    cumulative wall time: 48.9 seconds
Features detected for doctesting: sage.rings.number_field
============================================================================================================ test session starts ============================================================================================================
platform darwin -- Python 3.10.2, pytest-7.0.1, pluggy-1.0.0
rootdir: /Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src, configfile: tox.ini
collected 0 items                                                                                                                                                                                                                           

=========================================================================================================== no tests ran in 0.01s ===========================================================================================================
ERROR: not found: /Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/symbolic/expression.pyx
(no name '/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/symbolic/expression.pyx' in any of [])

[mkoeppe]

comment:8

There's a (trivial) merge conflict with #32975, best to merge in to make it easier to test.

[mkoeppe]

comment:9

Typo: skip_as_false_positve -> skip_as_false_positive (2 times)

[mkoeppe]

comment:10

Other than comment:7, comment:8, comment:9, it looks good.

I've tested it by itself (it makes a clear improvement - the remaining failures discussed in comment:6 and comment:7 are also present in the current beta).

It also works well with #31924 merged (it does not create new problems).

[mkoeppe]

Reviewer: Sebastian Oehms, Matthias Koeppe

[soehms]

comment:13

Replying to @tobiasdiez:

@soehms, thanks for having a look. The first issue you mention also exists in develop and I made some progress in fixing it at #33550. The second issue is now fixed.

Merging with #33550 I had to resolve some conflics. So I think it would make sence to have it here as dependency.

Im not really familiar with pytest. Thus, I can check that this branch does what it is supposed to. But I'm not the right person to evaluate the code.
I think it would be helpful, if the hook function pytest_collection_modifyitems (and maybe other function and classes in conftest.py, as well) would have a docstring
explaining what this hook is good for (with pointers to the documentation) and why we need to use it here (in more than just one line).

Together with #33550 the behavior with respect to src/sage/tests/cmdline.py remains unchanged. Further, I get the following warnings:

Doctesting 3 files using 4 threads.
sage -t --long --warn-long 124.6 --random-seed=329268236894550985110801561272292236637 src/sage/misc/explain_pickle.py
    [329 tests, 0.52 s]
sage -t --long --warn-long 124.6 --random-seed=329268236894550985110801561272292236637 src/sage/schemes/elliptic_curves/kraus.py
    [142 tests, 24.97 s]
sage -t --long --warn-long 124.6 --random-seed=329268236894550985110801561272292236637 src/sage/symbolic/relation.py
    [393 tests, 28.02 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 28.6 seconds
    cpu time: 51.6 seconds
    cumulative wall time: 53.5 seconds
Features detected for doctesting:
==================================================================================================================== test session starts ====================================================================================================================
platform linux -- Python 3.10.2, pytest-7.1.1, pluggy-1.0.0
rootdir: /home/sebastian/devel/sage/src, configfile: tox.ini
collected 7 items

src/sage/misc/explain_pickle.py s                                                                                                                                                                                                                     [ 14%]
src/sage/schemes/elliptic_curves/kraus.py sssss                                                                                                                                                                                                       [ 85%]
src/sage/symbolic/relation.py s                                                                                                                                                                                                                       [100%]

===================================================================================================================== warnings summary ======================================================================================================================
src/sage/misc/explain_pickle.py:2723
  /home/sebastian/devel/sage/src/sage/misc/explain_pickle.py:2723: PytestCollectionWarning: cannot collect test class 'TestReduceGetinitargs' because it has a __init__ constructor (from: sage/misc/explain_pickle.py)
    class TestReduceGetinitargs:

src/sage/misc/explain_pickle.py:2775
  /home/sebastian/devel/sage/src/sage/misc/explain_pickle.py:2775: PytestCollectionWarning: cannot collect test class 'TestReduceNoGetinitargs' because it has a __init__ constructor (from: sage/misc/explain_pickle.py)
    class TestReduceNoGetinitargs:

src/sage/misc/explain_pickle.py:2813
  /home/sebastian/devel/sage/src/sage/misc/explain_pickle.py:2813: PytestCollectionWarning: cannot collect test class 'TestAppendList' because it has a __init__ constructor (from: sage/misc/explain_pickle.py)
    class TestAppendList(list):

src/sage/misc/explain_pickle.py:2861
  /home/sebastian/devel/sage/src/sage/misc/explain_pickle.py:2861: PytestCollectionWarning: cannot collect test class 'TestAppendNonlist' because it has a __init__ constructor (from: sage/misc/explain_pickle.py)
    class TestAppendNonlist(object):

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================================================================================================== 7 skipped, 4 warnings in 0.18s ===============================================================================================================

Are these expected?

[tobiasdiez]

comment:34

In #31924 comment:43, Matthias argues that

I think it's fine to rename them if they are only used by doctests in the same module. Then we might argue that it was not really intended to be in the public interface and should have been named _test....

I agree with him that these test-related methods should have been private from the get-go. Especially since most of them contain an explicit disclaimer ala "this is intended only for usage in doctests".

But if you really have issues with the rename and think it's the right procedure, I can start a discussion on the mailing list.

[vbraun]

Changed branch from public/tests/pytest_wrong_test_methods to 7644854

[vbraun]

Changed branch from 7644854 to public/tests/pytest_wrong_test_methods

[vbraun]

comment:37

Well tests work... let me know when it is ready to be merged.

[tscrim]

comment:39

Replying to @tobiasdiez:

In #31924 comment:43, Matthias argues that

I think it's fine to rename them if they are only used by doctests in the same module. Then we might argue that it was not really intended to be in the public interface and should have been named _test....

I agree with him that these test-related methods should have been private from the get-go. Especially since most of them contain an explicit disclaimer ala "this is intended only for usage in doctests".

It does not matter what they should have been, just what they are.

But if you really have issues with the rename and think it's the right procedure, I can start a discussion on the mailing list.

Yes: comment:33

[mkoeppe]

comment:40

Instead of a policy discussion, perhaps can we move the ticket forward by setting up deprecated aliases for the renamed functions.

[tscrim]

comment:41

I strongly think we should not give any package, much less an optional package, any priority over names of public functions without a very good reason. I am also not convinced that we want to move forward with pytest becoming standard either. Both of these need to be discussed on sage-devel, with the first issue for this ticket. You cannot sidestep this. Deprecating the functions is efffectively the same as renaming them as it marks a major policy change.

[tscrim]

comment:42

Well, function names that are generic. If they are sufficiently unique to the package, then that is fine with me.

[mkoeppe]

comment:43

All of them are unique to the module that they are defined in, and their purpose is to support doctests in the same module. Renaming them from test_... to _test_... is justified by existing policies, I'd say.

[tscrim]

comment:44

Yes, but that is a non sequitur. I agree that renaming them (with deprecation) is within the current policy, but it is the reason for these changes that is the issue. It is about the conflict with an optional package's conventions that are fully reserve a certain class of (natural) function names. Enforcing this (which is done here across the entire Sage library) would give pytest very special privileges and status within Sage.

[mkoeppe]

comment:45

I have split out #33612 for the strategy proposed in comment:40, comment:43. As you will see in the ticket description, it is an improvement to the Sage library independent from its motivation for pytest.

[mkoeppe]

Changed dependencies from #32975 to #32975, #33612

[tobiasdiez]

comment:47

Thanks Matthias!

[mkoeppe]

Changed dependencies from #32975, #33612 to #32975, #33612, #33617