Use the 'with' statement in conjunction with 'open' throughout test modules

[midnightdf]

BPO	7944
Nosy	@rhettinger, @amauryfa, @giampaolo, @ezio-melotti, @merwok, @voidspace
Dependencies	bpo-7232: Support of 'with' statement fo TarFile class
Files	test_old_mailbox.py: Modified version of test_old_mailbox.py test_old_mailbox.patch issue7944_gzip.diff issue7944_marshal.diff issue7944_zipfile64.diff issue7944_tarfile.diff: This depends on Support of 'with' statement fo TarFile class #51481 issue7944_tempfile.diff: Updated with review comments from RDM test_os.patch issue7944_tarfile_2_7.diff

^{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 = 'https://github.com/ezio-melotti'
closed_at = <Date 2016-01-13.20:23:00.745>
created_at = <Date 2010-02-16.17:43:50.012>
labels = ['easy', 'type-feature', 'tests']
title = "Use the 'with' statement in conjunction with 'open' throughout test modules"
updated_at = <Date 2016-01-13.20:23:00.744>
user = 'https://bugs.python.org/midnightdf'

bugs.python.org fields:

activity = <Date 2016-01-13.20:23:00.744>
actor = 'ezio.melotti'
assignee = 'ezio.melotti'
closed = True
closed_date = <Date 2016-01-13.20:23:00.745>
closer = 'ezio.melotti'
components = ['Tests']
creation = <Date 2010-02-16.17:43:50.012>
creator = 'midnightdf'
dependencies = ['7232']
files = ['16235', '16237', '16286', '16287', '16290', '16311', '16312', '19218', '36212']
hgrepos = []
issue_num = 7944
keywords = ['patch', 'easy']
message_count = 22.0
messages = ['99425', '99434', '99617', '99618', '99619', '99699', '99700', '99703', '99780', '118498', '118500', '118553', '118554', '118556', '118557', '118610', '185771', '185817', '224458', '224564', '258166', '258167']
nosy_count = 10.0
nosy_names = ['rhettinger', 'amaury.forgeotdarc', 'giampaolo.rodola', 'ezio.melotti', 'eric.araujo', 'michael.foord', 'midnightdf', 'BreamoreBoy', 'python-dev', 'evans.mungai']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue7944'
versions = ['Python 2.7']

[midnightdf]

Sprinkled throughout CPython's test modules are snippets of code such as the following taken from 2.7A3's test_old_mailbox.py (line 141):
box = mailbox.UnixMailbox(open(self._path, 'r'))

The key thing to observe here is the file being opened yet never has it's 'close' method explicitly called. While this is fine for CPython's rather predictable garbage collections, it's quite possible that in alternate implementations of Python this file object won't be destroyed until well after line 141. This can result in seemingly random failures of CPython's tests under alternate implementations of Python.

The solution to this problem would be to consistently use the 'with' statement (or alternatively close all open file objects) throughout all CPython test modules.

I've gone ahead and submitted an updated (2.7A3) version of test_old_mailbox.py which addresses this. We can find other places where this is going on as well, but for the most part the tests already seem to be doing the right thing.

[amauryfa]

Please use patches to show where the code changed.
(I generated one; it looks good to me)

I would like this kind of patches applied to CPython.
Looking at the Pypy test suite, the following test files seem easy to update:
test_bz2
test_dumbdbm
test_mailbox
test_marshal
test_mmap
test_pkgimport
test_tarfile
test_tempfile
test_traceback

[briancurtin]

Does anyone prefer if this should be done on a patch-per-module basis, or just throw together one huge change?

Also, I've found a few places where context managers aren't supported where they probably should be, so I'll spin off separate issues for those.

[voidspace]

There are several standard library modules that deliberately maintain compatibility with earlier versions of Python. The 'with' statement shouldn't be used for tests of any modules that still need compatibility with Python 2.4.

[ezio-melotti]

Some are listed in the PEP-291 (http://www.python.org/dev/peps/pep-0291/) but I don't know if that list is really updated.

[briancurtin]

I'm going to go ahead with the patch-per-module approach, but anyone feel free to stop me from doing that.

Here's a patch for test_gzip on trunk.

[briancurtin]

Here's a patch for test_marshal on trunk.

[briancurtin]

Here's a patch for test_tempfile on trunk. The rest will just be silently added since this is already annoying.

[briancurtin]

Patch for test_tarfile. However, bpo-7232 needs to go in first.

[briancurtin]

Fixed test_gzip in r85400.

[briancurtin]

Fixed test_mailbox in r85401.
Fixed test_marshal in r85402.
Fixed test_bz2 in r85403.

[giampaolo]

Here is a patch for test_os.py.

[rhettinger]

I'm not sure that this is a worthwhile exercise unless it can be shown for certain that alternate implementations need this. Otherwise, it just changes tests in a way that isn't backwards compatible, makes it more difficult to apply patches cleanly, and risks damaging an otherwise good test.