The Lib/test/regrtest.py file became a monster. It's very long, it uses a lot of global variables. It's hard to review and hard to maintain. I propose to:

- split single file regrtest.py into multiple files in a new Lib/test/regrtest/ package
- refactor the test runner as a class to avoid global variables
- refactor the multiprocessing test runner as a class in a separated file

Attached patch implements this idea.


The following commands still work:

   ./python -m test [options] [args]
   ./python -m test.regrtest [options] [args]

But the following command doesn't work anymore:

   ./python Lib/test/regrtest.py [options] [args]

If regrtest.py is executed directly on buildbots, these buildbots should be modified first.


The "./python -m test.regrtest" command should work on all Python versions (Python 2 and Python 3) and so should be preferred. The "./python -m test" command only works on Python 3 (thanks to the new __main__.py feature).


The change adds a new feature: it now displays name of concurrent tests running since 30 seconds or longer when the multiprocessing test runner is used (-j command line option). Example:

    ...
    [240/399] test_uu
    [241/399] test_urllib_response
    [242/399] test_struct
    [243/399] test_descrtut
    [244/399] test_threadedtempfile
    [245/399] test_tracemalloc -- running: test_concurrent_futures (30 sec)
    [246/399] test_dbm_dumb -- running: test_concurrent_futures (30 sec)
    [247/399] test_codeop -- running: test_concurrent_futures (30 sec)
    ...
    [395/399/1] test_asyncio -- running: test_multiprocessing_fork (40 sec), test_multiprocessing_spawn (44 sec)
    [396/399/1] test_faulthandler -- running: test_multiprocessing_fork (50 sec), test_multiprocessing_spawn (54 sec)
    [397/399/1] test_multiprocessing_fork (52 sec) -- running: test_multiprocessing_spawn (56 sec)
    [398/399/1] test_multiprocessing_spawn (68 sec) -- running: test_multiprocessing_forkserver (39 sec)
    [399/399/1] test_multiprocessing_forkserver (50 sec)

I want this feature to analysis why more and more buildbots fail with a timeout without saying which test was running (well, I suspect multiprocessing tests...).

Note: faulthandler can show where regrtest is blocked, but not when the multiprocessing test runner is used. And sometimes the process is killed by the buildbot, not by faulthandler :-/


Another minor new feature: on CTRL+c, it shows which tests are running when the multiprocessing test runner is used. Example:

    [ 38/399] test_dummy_thread
    [ 39/399] test_codecmaps_jp
    [ 40/399] test_future5
    ^C
    Waiting for test_scope, test_decimal, test_memoryview, test_heapq, test_unicodedata, test_trace, test_threadsignals, test_cgitb, test_runpy, test_cmd_line_script


Other changes:

* Show test timing when a test runs longer than 30 seconds

* Don't make __file__ absolute, findtestdir() calls os.path.abspath() instead. Remove these lines:

    __file__ = os.path.abspath(__file__)
    assert __file__ == os.path.abspath(sys.argv[0])

* print() is now called wih flush=True (it didn't check all calls, only the major calls), remove sys.stdout.flush() and  sys.stdout.flush()

* A lot of refactoring. Sorry, I didn't take notes for each change.


I fear that test_regrtest has a small code coverage. I only tested major options, I didn't test -R for example.


Note: I don't understand how the --single option works when regrtest is not run from the Python source code directory. A temporary directory, so the pynexttest file is removed after its creation, no? If it doesn't make sense to use --single outside Python directory, maybe an error should be raised?

середа, 23-вер-2015 10:12:35 STINNER Victor написано:
> New submission from STINNER Victor:
> 
> The Lib/test/regrtest.py file became a monster. It's very long, it uses a
> lot of global variables. It's hard to review and hard to maintain. I
> propose to:
> 
> - split single file regrtest.py into multiple files in a new
> Lib/test/regrtest/ package

You can just mover parts of the code into utility files (or to the test.support 
package) without converting regrtest.py to a package. This preserves 
compatibility with running Lib/test/regrtest.py as a script.

> - refactor the test runner as a class to avoid global variables

Global variables are not evil in a script. Are there other reasons besides 
aesthetic?

I never used --single, but quickly looking at the code, it looks like the pynexttest file got stored in /tmp or similar, via tempfile.gettempdir(), so it should usually survive. However it looks like your patch now creates “pynexttest” in a temporary directory that gets removed at the end?

Left another comment on Rietveld about out of date stuff in __main__.

I like the new features, but I think we should take a look at issue 10967 before converting it to a package or refactoring it.

The buildbot part is also a bit complicated. For example, support.verbose doesn't work correctly on builtbots: issue 23235.

Serhiy Storchaka wrote:
"""
You can just mover parts of the code into utility files (or to the test.support 
package) without converting regrtest.py to a package. This preserves 
compatibility with running Lib/test/regrtest.py as a script.
"""

Does it really matter? Who runs directly Lib/test/regrtest.py? It's simple to replace Lib/test/regrtest.py with -m test (or -m test.regrtest), no?

Serhiy Storchaka wrote: "Global variables are not evil in a script. Are there other reasons besides 
aesthetic?"

It's much easier to split a long function using multiple small functions when no global variable is used. I also like OOP, so I created a class. Should I understand that you don't like classes and would prefer to keep flat functions?

Martin Panter wrote:
"I never used --single, but quickly looking at the code, it looks like the pynexttest file got stored in /tmp or similar, via tempfile.gettempdir(), so it should usually survive. However it looks like your patch now creates “pynexttest” in a temporary directory that gets removed at the end?"

Ah ok, it's /tmp. I misunderstood the code. I understand that it creates a temporary subdirectory and write into the temporary subdirectory which will be removed.

Berker Peksag: "I like the new features, but I think we should take a look at issue 10967 before converting it to a package or refactoring it."

Even if we move some features to unittest, I don't think that regrtest.py will be reduced to fewer than 100 lines of code. By the way, most reusable code is in test.support. test.regrtest is much more specific to the CPython test suite, no?

Berker Peksag: "The buildbot part is also a bit complicated. For example, support.verbose doesn't work correctly on builtbots: issue 23235."

Sorry, I don't see the link with this issue. How are these two issues related?

"I want this feature to analysis why more and more buildbots fail with a timeout without saying which test was running (well, I suspect multiprocessing tests...)."

Recent example:

http://buildbot.python.org/all/builders/AMD64%20FreeBSD%209.x%203.x/builds/3389/steps/test/logs/stdio

...

[395/399] test_pep352
[396/399] test_ioctl
[397/399] test_multiprocessing_fork
[398/399] test_multiprocessing_forkserver

command timed out: 3900 seconds without output running ['make', 'buildbottest', 'TESTOPTS= -j4', 'TESTPYTHONOPTS=', 'TESTTIMEOUT=3600'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=4949.196279

Another recent example:

http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/6712/steps/test/logs/stdio

...
[395/399] test_asyncio
[396/399] test_email
[397/399] test_threaded_import
[398/399] test_tools

command timed out: 3900 seconds without output, attempting to kill
program finished with exit code 1
elapsedTime=4958.515000

This may be opening a can of worms, but I wonder if what we should really do is re-engineer regrtest from the ground up, keeping the existing regrtest around until we are satisfied with its replacement...and maybe some of the options wouldn't make the transition.  (I've used --single, but it's been a long time, and I think it may have only been when I was testing regrtest after modifying it...)

We could then shift buildbots over to the new command gradually (or, do a few first, and then all the rest).  Then when we're satisfied we could change -m test to use the new interface, but keep regrtest around, unmaintained, for a couple of releases for those who are still using it.

I haven't looked at Victor's code to see if I like his re-engineering, but I'm really talking about starting the re-engineering from the API, and only then thinking about the code to implement it.

"This may be opening a can of worms, but I wonder if what we should really do is re-engineer regrtest from the ground up,"

It's not a full reengineering. My patch takes the current code and split it into smaller files. It's not a new implementation or anything like that.

"keeping the existing regrtest around until we are satisfied with its replacement..."

why are you saying "replacement"? Replaced by what?

"(I've used --single, but it's been a long time, and I think it may have only been when I was testing regrtest after modifying it...)"

You can propose to remove this option if you think that it's useless. I don't want to touch options, I don't know how regrtest is used, and regrtest works right? (If it works, don't touch it :-))

"I haven't looked at Victor's code to see if I like his re-engineering, but I'm really talking about starting the re-engineering from the API, and only then thinking about the code to implement it."

Sorry, but writing a new regrtest project is a full new project. Please open a new issue if you want to invest time on that.

New changeset eaf9a99b6bb8 by Victor Stinner in branch 'default':
Issue #25220: Create Lib/test/libregrtest/
https://hg.python.org/cpython/rev/eaf9a99b6bb8

New changeset c92d893fd3c8 by Victor Stinner in branch 'default':
Issue #25220: Backed out changeset eaf9a99b6bb8
https://hg.python.org/cpython/rev/c92d893fd3c8

I pushed the changeset eaf9a99b6bb8, but R. David Murray asked me to wait for a review before pushing changes. So I reverted my change.

I checked if Lib/test/regrtest.py is called directly. The answer is yes: it's called inside Python in various places, but also in scripts to build Python packages. Even if "python -m test" works (and "python -m test.regrtest" works on all Python versions), I prefer to keep Lib/test/regrest.py to not force users to have to modify their script.

So I propose to simply move regrtest.py code to smaller files in Lib/test/libregrtest/, as I did in attached regrtest_package.patch.

In the changeset eaf9a99b6bb8, I started with cmdline.py because it's the only part of regrtest.py which has real unit tests. I created Lib/test/libregrtest/cmdline.py with "hg cp" to keep Mercurial history. I checked which moved symbols are used: _parse_args() and RESOURCE_NAMES. I exported them in test.libregrtest. I had to modify Lib/test/test_regrtest.py.

For next changes, I will try to add a few new unit tests to Lib/test/test_regrtest.py.

--

Berker wrote: "I like the new features, but I think we should take a look at issue 10967 before converting it to a package or refactoring it."

IMHO it will be easier to enhance regrtest to reuse unit test features, make the code smaller, fix bugs, etc. if regrtest.py is splitted into smaller files. Moving code to a new test.libregrtest submodule should help to implement the issue #10967 & friends (ex: #16748)

Victor and I have been discussing this on IRC.  We agreed that importing from regrtest and running regrtest as a script needs to keep working.

Summary of my disagreements: I'm leery of wholesale changes to regrtest
because we have gotten bug reports when we've broken things before.  Obviously it's not in the same problem-class as breaking Python, but I'd rather see us start a new command that drops the cruft and just does the things we really use, and stop maintaining regrtest.  However, I'm not in a position to do that work, and Victor has no interest in it.  So, I'm -0.5 on a big refactoring of regrtest (but have no objection to the new features :).  I won't block the change, I'd just prefer a cleaner solution...but don't have the time to implement it myself.

New changeset 40667f456c6f by Victor Stinner in branch 'default':
Issue #25220: Create Lib/test/libregrtest/
https://hg.python.org/cpython/rev/40667f456c6f

New changeset 4a9418ed0d0c by Victor Stinner in branch 'default':
Issue #25220: Move most regrtest.py code to libregrtest
https://hg.python.org/cpython/rev/4a9418ed0d0c

Ok, I started simply by *moving* code, without additionnal changes. I will watch for buildbots to ensure that no regression was added. The Lib/test/regrtest.py is kept to not break any existing tool. It looks like Lib/test/regrtest.py is very important in Python, much more than what I expected. It's used for PGO compilation, it's used by various scripts for different platforms, it's used in scripts to build Linux packages,etc.

I used "hg cp" to create new files to ensure that the history is not lost.

I will write new patches for the real refactoring work described in the first message. It will be much easier to review changes written after the code was moved. So we can discuss controversal changes like the creation of a class for libregrtest/main.py ;-)

New changeset 892b4f029ac6 by Victor Stinner in branch 'default':
Issue #25220: Fix Lib/test/autotest.py
https://hg.python.org/cpython/rev/892b4f029ac6

test_regrtest.patch: add tests for the 8 (!) ways to run the Python test suite.

TODO:
* try test on Windows
* maybe test also PCbuild/rt.bat on Windows

test_regrtest-2.patch: add tests for -u, -r, --randseed and --fromfile command line options.

test_regrtest-3.patch: I added a test on PCbuild\rt.bat and fixed the test on Tools/buildbot/test.bat (Windows only tests).

regrtest_class.patch: Convert the long and complex main() function into a new Regrtest class using attributes and methods.

* Convert main() variables to Regrtest attributes: see the __init__() method. I documented some attributes

* Convert accumulate_result() function to a method

* Create setup_python() and setup_regrtest() methods. Maybe both methods should be merged, but for the first change I preferred to keep almost the same instruction order (not move code too much).

* Import gc at top level: the --threshold command line option is now ignored if the gc module is missing.

* Move resource.setrlimit() and the code to make the module paths absolute into the new setup_python() method. So this code is no more executed when the module is imported, only when main() is executed. We have a better on when the setup is done.

* Move textwrap import from printlist() to the top level.

* Some other minor cleanup.

New changeset 3393d86c3bb2 by Victor Stinner in branch 'default':
Issue #25220: Add functional tests to test_regrtest
https://hg.python.org/cpython/rev/3393d86c3bb2

New changeset a9fe8cd339b1 by Victor Stinner in branch 'default':
Fix test_regrtest.test_tools_buildbot_test()
https://hg.python.org/cpython/rev/a9fe8cd339b1

regrtest_class-2.patch: rebased path to try to get the [review] link.

New changeset e5165dcae942 by Victor Stinner in branch 'default':
Issue #25220: Add test for --wait in test_regrtest
https://hg.python.org/cpython/rev/e5165dcae942

New changeset 817e25bd34d0 by Victor Stinner in branch 'default':
Issue #25220: Split the huge main() function of libregrtest.main into a class
https://hg.python.org/cpython/rev/817e25bd34d0

New changeset 12c666eea556 by Victor Stinner in branch 'default':
Issue #25220: Create libregrtest/runtest_mp.py
https://hg.python.org/cpython/rev/12c666eea556

New changeset 281ab7954d7c by Victor Stinner in branch 'default':
Issue #25220: Enhance regrtest --coverage
https://hg.python.org/cpython/rev/281ab7954d7c

New changeset 45c43b9d50c5 by Victor Stinner in branch 'default':
Issue #25220: regrtest setups Python after parsing command line options
https://hg.python.org/cpython/rev/45c43b9d50c5

New changeset e6b48bfd6d8e by Victor Stinner in branch 'default':
Issue #25220: truncate some long lines in libregrtest/*.py
https://hg.python.org/cpython/rev/e6b48bfd6d8e

New changeset 2c53c8dcde3f by Victor Stinner in branch 'default':
Issue #25220, libregrtest: Remove unused import
https://hg.python.org/cpython/rev/2c53c8dcde3f

New changeset 8bd9422ef41e by Victor Stinner in branch 'default':
Issue #25220: Enhance regrtest -jN
https://hg.python.org/cpython/rev/8bd9422ef41e

New changeset 7e07c51d8fc6 by Victor Stinner in branch 'default':
Issue #25220: Use print(flush=True) in libregrtest
https://hg.python.org/cpython/rev/7e07c51d8fc6

New changeset e2ed6e9163d5 by Victor Stinner in branch 'default':
Issue #25220, libregrtest: Cleanup setup code
https://hg.python.org/cpython/rev/e2ed6e9163d5

New changeset b7d27c3c9e65 by Victor Stinner in branch 'default':
Issue #25220, libregrtest: Move setup_python() to a new submodule
https://hg.python.org/cpython/rev/b7d27c3c9e65

New changeset ff012c1b8068 by Victor Stinner in branch 'default':
Issue #25220, libregrtest: Add runtest_ns() function
https://hg.python.org/cpython/rev/ff012c1b8068

New changeset b48ae54ed5be by Victor Stinner in branch 'default':
Issue #25220, libregrtest: Call setup_python(ns) in the slaves
https://hg.python.org/cpython/rev/b48ae54ed5be

New changeset 6ec81abb8e6a by Victor Stinner in branch 'default':
Issue #25220, libregrtest: Set support.use_resources in setup_tests()
https://hg.python.org/cpython/rev/6ec81abb8e6a

New changeset e765b6c16e1c by Victor Stinner in branch 'default':
Issue #25220, libregrtest: Pass directly ns to runtest()
https://hg.python.org/cpython/rev/e765b6c16e1c

New changeset 8e985fb19724 by Victor Stinner in branch 'default':
Issue #25220, libregrtest: Cleanup
https://hg.python.org/cpython/rev/8e985fb19724

New changeset ed0734966dad by Victor Stinner in branch 'default':
Issue #25220, libregrtest: more verbose output for -jN
https://hg.python.org/cpython/rev/ed0734966dad

New changeset ec02ccffd1dc by Victor Stinner in branch 'default':
Issue #25220: Fix "-m test --forever"
https://hg.python.org/cpython/rev/ec02ccffd1dc

It's not perfect, but libregrtest looks better than Python 3.5 regrtest.py. I close the issue.