Issue 25220: Enhance and refactor test.regrtest (convert regrtest.py to a package) (original) (raw)

Created on 2015-09-23 10:12 by vstinner, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Messages (35)

msg251419 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-23 10:12

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:

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:

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?

msg251420 - (view)

Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer)

Date: 2015-09-23 11:06

середа, 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:

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.

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

msg251421 - (view)

Author: Martin Panter (martin.panter) * (Python committer)

Date: 2015-09-23 11:32

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.

msg251422 - (view)

Author: Berker Peksag (berker.peksag) * (Python committer)

Date: 2015-09-23 11:36

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.

msg251423 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-23 11:57

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?

msg251428 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-23 13:27

"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

msg251429 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-23 13:52

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

msg251444 - (view)

Author: R. David Murray (r.david.murray) * (Python committer)

Date: 2015-09-23 20:11

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.

msg251445 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-23 20:18

"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.

msg251455 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-23 21:11

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

msg251458 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-23 21:17

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

msg251459 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-23 21:26

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)

msg251462 - (view)

Author: R. David Murray (r.david.murray) * (Python committer)

Date: 2015-09-23 21:51

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.

msg251639 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-26 07:44

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

msg251642 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-26 09:25

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

msg251643 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-26 09:29

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 ;-)

msg251695 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-27 09:40

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

msg251696 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-27 09:41

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

TODO:

msg251752 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-28 07:30

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

msg251763 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-28 12:48

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

msg251780 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-28 17:15

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

msg251803 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-28 22:19

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

msg251806 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-28 23:10

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

msg251842 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-09-29 10:37

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

msg251850 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-29 12:19

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

msg251887 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-29 20:57

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

msg251890 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-29 21:25

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

msg251893 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-29 21:37

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

msg251894 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-29 21:56

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

msg251895 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-29 22:41

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

msg251900 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-30 00:05

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

msg251903 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-30 00:40

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

msg251907 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-30 01:06

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

msg251923 - (view)

Author: Roundup Robot (python-dev) (Python triager)

Date: 2015-09-30 12:30

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

msg252159 - (view)

Author: STINNER Victor (vstinner) * (Python committer)

Date: 2015-10-02 21:04

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

History

Date

User

Action

Args

2022-04-11 14:58:21

admin

set

github: 69407

2015-10-02 21:13:52

vstinner

set

status: open -> closed
resolution: fixed

2015-10-02 21:04:08

vstinner

set

messages: +

2015-09-30 12:30:08

python-dev

set

messages: +

2015-09-30 01:06:00

python-dev

set

messages: +

2015-09-30 00:40:36

python-dev

set

messages: +

2015-09-30 00:05:13

python-dev

set

messages: +

2015-09-29 22:41:24

python-dev

set

messages: +

2015-09-29 21:56:33

python-dev

set

messages: +

2015-09-29 21:37:54

python-dev

set

messages: +

2015-09-29 21:25:05

python-dev

set

messages: +

2015-09-29 20:57:30

python-dev

set

messages: +

2015-09-29 12:19:10

python-dev

set

messages: +

2015-09-29 10:37:07

vstinner

set

files: + regrtest_class-2.patch

messages: +

2015-09-28 23:10:36

python-dev

set

messages: +

2015-09-28 22:19:12

python-dev

set

messages: +

2015-09-28 17:15:37

vstinner

set

files: + regrtest_class.patch

messages: +

2015-09-28 15:36:17

Arfrever

set

nosy: + Arfrever

2015-09-28 12:48:35

vstinner

set

files: + test_regrtest-3.patch

messages: +

2015-09-28 07:30:20

vstinner

set

files: + test_regrtest-2.patch

messages: +

2015-09-27 09:41:16

vstinner

set

files: + test_regrtest.patch

messages: +

2015-09-27 09:40:16

python-dev

set

messages: +

2015-09-26 09:29:17

vstinner

set

messages: +

2015-09-26 09:25:51

python-dev

set

messages: +

2015-09-26 07:44:54

python-dev

set

messages: +

2015-09-23 21:51:30

r.david.murray

set

messages: +

2015-09-23 21:26:36

vstinner

set

messages: +

2015-09-23 21:17:04

python-dev

set

messages: +

2015-09-23 21:11:52

python-dev

set

nosy: + python-dev
messages: +

2015-09-23 20🔞58

vstinner

set

messages: +

2015-09-23 20:11:55

r.david.murray

set

nosy: + r.david.murray
messages: +

2015-09-23 13:52:29

vstinner

set

messages: +

2015-09-23 13:27:43

vstinner

set

messages: +

2015-09-23 11:57:35

vstinner

set

messages: +

2015-09-23 11:36:22

berker.peksag

set

nosy: + berker.peksag
messages: +

2015-09-23 11:32:06

martin.panter

set

nosy: + martin.panter
messages: +

2015-09-23 11:06:39

serhiy.storchaka

set

messages: +

2015-09-23 10:23:20

jkloth

set

nosy: + jkloth

2015-09-23 10:12:34

vstinner

create