Issue 33873: False positives when running leak tests with -R 1:1

I am not sure is a problem we can do something about but right know if you run the refleak tests with low repetitions it reports leaks:

./python -m test test_list -R 1:1
Run tests sequentially
0:00:00 load avg: 0.66 [1/1] test_list
beginning 2 repetitions
12
..

test_list leaked [3] memory blocks, sum=3
test_list failed

== Tests result: FAILURE ==

1 test failed:
    test_list

Total duration: 1 sec 759 ms
Tests result: FAILURE

This also happens with other low numbers: 

./python -m test test_list -R 1:2

Obviously using this numbers is "wrong" (there is not enough repetitions to get meaningful results). The only problem I see is that if you are not aware of this limitation (in the case this is a real limitation on how `dash_R` works) the output is a bit misleading. 

Should we leave this as it is or try to improve the output?

> test_list leaked [3] memory blocks, sum=3

Memory block leaks are very different from reference leaks. Memory block are low level allocations. Python has *many* internal caches: tuple uses an internal "free list" for example. The first runs of the tests (2 runs when using -R 2:3) is used to warmup these caches.

Maybe regrtest -R should raise an error, or at least emit a big warning when using -R with less than 3 warmup runs.

By the way, regrtest has a very old bug: -R 3:3 runs the test 7 times, not 6 times. See runtest_inner() in Lib/test/libregrtest/runtest.py:

            test_runner()
            if ns.huntrleaks:
                refleak = dash_R(the_module, test, test_runner, ns.huntrleaks)

The code should be:

            if ns.huntrleaks:
                refleak = dash_R(the_module, test, test_runner, ns.huntrleaks)
            else:
                test_runner()

Do you want to write a PR for that? I should make our Refleaks buildbots 1/7 faster ;-)

Let's make the buildbots happier!

I tested PR 7735:

vstinner@apu$ ./python -m test -R 0:0 test_os -m test_access
Run tests sequentially
0:00:00 load avg: 0.19 [1/1] test_os
beginning 0 repetitions


test_os leaked [] references, sum=0
test_os leaked [] memory blocks, sum=0
test_os failed

== Tests result: FAILURE ==

1 test failed:
    test_os

Total duration: 63 ms
Tests result: FAILURE

vstinner@apu$ ./python -m test -R 0:1 test_os -m test_access
Run tests sequentially
0:00:00 load avg: 0.35 [1/1] test_os
beginning 1 repetitions
1
.
test_os leaked [280435] references, sum=280435
test_os leaked [91518] memory blocks, sum=91518
test_os leaked [4] file descriptors, sum=4
test_os failed

== Tests result: FAILURE ==

1 test failed:
    test_os

Total duration: 95 ms
Tests result: FAILURE

vstinner@apu$ ./python -m test -R 1:0 test_os -m test_access
Run tests sequentially
0:00:00 load avg: 0.16 [1/1] test_os
beginning 1 repetitions
1
.
test_os leaked [] references, sum=0
test_os leaked [] memory blocks, sum=0
test_os failed

== Tests result: FAILURE ==

1 test failed:
    test_os

Total duration: 95 ms
Tests result: FAILURE



Hum, we should require at least one run and at least one warmup: -R 1:1 should be the bare minimum.

By the way, it seems like negative numbers are currently accepted, whereas it doesn't make sense:

vstinner@apu$ ./python -m test -R 0:-2 test_list
Run tests sequentially
0:00:00 load avg: 0.31 [1/1] test_list
beginning -2 repetitions
(...)

It would fix this bug as well.

Updated PR7735 with the checks for invalid parameters.

New changeset cac4fef8860e66a9da67d09762f5b614b9471a12 by Victor Stinner (Pablo Galindo) in branch 'master':
bpo-33873: regrtest: Add warning on -R 1:3 (GH-7736)
https://github.com/python/cpython/commit/cac4fef8860e66a9da67d09762f5b614b9471a12

Once PR 7735 will be merged, it may be worth it to backport the enhancements to other branches. See maybe bpo-33718 to check if there are other changes that should be backportd. It can help to backport the change to 2.7 (which is very different).

I think commit4ffe9c2b251f6e027b26250b7a2618e78d4edd22 from bpo-33718 should be backported IMO:

bpo-33718: regrtest: use format_duration() to display failed tests (GH-7686)
https://github.com/python/cpython/commit/4ffe9c2b251f6e027b26250b7a2618e78d4edd22

> I think commit 4ffe9c2b251f6e027b26250b7a2618e78d4edd22 from bpo-33718 should be backported IMO: ...

Once PR 7735 will be merged, we can backport recent regrtest commits at once into 3.7, then backport the 3.7 change to 3.6 and 2.7.

New changeset 58ed7307ea0b5c5aa052291ebc3030f314f938d8 by Pablo Galindo in branch 'master':
bpo-33873: Fix bug in `runtest.py` and add checks for invalid `-R` parameters (GH-7735)
https://github.com/python/cpython/commit/58ed7307ea0b5c5aa052291ebc3030f314f938d8

Since I didn't backport yet the commit 4ffe9c2b251f6e027b26250b7a2618e78d4edd22 (format duration) from master to other branches, I will backport the commit 58ed7307ea0b5c5aa052291ebc3030f314f938d8 (-R fix) manually to other branches with other recent regrtest changes.

New changeset d1f9481b7a2d31c40fca1347ef99d819eb656ce7 by Victor Stinner in branch '3.7':
bpo-33873: Backport regrtest from master to 3.7 (GH-7935)
https://github.com/python/cpython/commit/d1f9481b7a2d31c40fca1347ef99d819eb656ce7

New changeset 5430c14aba319f83b18879575244ba429e8c1d81 by Victor Stinner in branch '2.7':
[2.7] bpo-33873: Backport regrtest from master (GH-7936)
https://github.com/python/cpython/commit/5430c14aba319f83b18879575244ba429e8c1d81

New changeset 1d55888a78bcefed3723fb7e48df2a75b4f0adb0 by Victor Stinner in branch '3.6':
bpo-33873: Backport regrtest from master to 3.7 (GH-7935) (GH-7937)
https://github.com/python/cpython/commit/1d55888a78bcefed3723fb7e48df2a75b4f0adb0

Pablo Galindo Salgado fixed the bug in master, I backported his fix to 2.7, 3.6 and 3.7 branches. Thanks Pablo!

resolution: fixed
stage: patch review -> resolved