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

[pablogsal]

BPO	33873
Nosy	@vstinner, @serhiy-storchaka, @pablogsal
PRs	bpo-33873: Fix bug in runtest.py and add checks for invalid -R parameters #7735 bpo-33873: Add warning when using less than 3 warmup runs in runtest.py. #7736 [3.7] bpo-33873: Fix bug in runtest.py and add checks for invalid -R parameters (GH-7735) #7933 [3.6] bpo-33873: Fix bug in runtest.py and add checks for invalid -R parameters (GH-7735) #7934 bpo-33873: Backport regrtest from master to 3.7 #7935 [2.7] bpo-33873: Backport regrtest from master #7936 [3.6] bpo-33873: Backport regrtest from master to 3.6 (GH-7935) #7937

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-06-26.22:45:32.457>
created_at = <Date 2018-06-15.22:42:06.428>
labels = ['3.7', '3.8', 'type-bug', 'tests']
title = 'False positives when running leak tests with -R 1:1'
updated_at = <Date 2018-06-26.22:45:32.454>
user = 'https://github.com/pablogsal'

bugs.python.org fields:

activity = <Date 2018-06-26.22:45:32.454>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-06-26.22:45:32.457>
closer = 'vstinner'
components = ['Tests']
creation = <Date 2018-06-15.22:42:06.428>
creator = 'pablogsal'
dependencies = []
files = []
hgrepos = []
issue_num = 33873
keywords = ['patch']
message_count = 15.0
messages = ['319688', '319689', '319690', '319691', '319693', '319694', '320077', '320118', '320120', '320492', '320510', '320514', '320517', '320522', '320524']
nosy_count = 3.0
nosy_names = ['vstinner', 'serhiy.storchaka', 'pablogsal']
pr_nums = ['7735', '7736', '7933', '7934', '7935', '7936', '7937']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33873'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

[pablogsal]

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?

[vstinner]

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

[pablogsal]

Let's make the buildbots happier!

[vstinner]

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.

[pablogsal]

Updated PR7735 with the checks for invalid parameters.

[vstinner]

New changeset cac4fef by Victor Stinner (Pablo Galindo) in branch 'master':
bpo-33873: regrtest: Add warning on -R 1:3 (GH-7736)
cac4fef

[vstinner]

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

[pablogsal]

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

bpo-33718: regrtest: use format_duration() to display failed tests (GH-7686)
4ffe9c2

[vstinner]

I think commit 4ffe9c2 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.

[pablogsal]

New changeset 58ed730 by Pablo Galindo in branch 'master':
bpo-33873: Fix bug in runtest.py and add checks for invalid -R parameters (GH-7735)
58ed730

[vstinner]

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