classification
Title: Use the 'with' statement in conjunction with 'open' throughout test modules
Type: enhancement Stage: resolved
Components: Tests Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: 7232 Superseder:
Assigned To: ezio.melotti Nosy List: BreamoreBoy, amaury.forgeotdarc, eric.araujo, evans.mungai, ezio.melotti, giampaolo.rodola, michael.foord, midnightdf, python-dev, rhettinger
Priority: normal Keywords: easy, patch

Created on 2010-02-16 17:43 by midnightdf, last changed 2016-01-13 20:23 by ezio.melotti. This issue is now closed.

Files
File name Uploaded Description Edit
test_old_mailbox.py midnightdf, 2010-02-16 17:43 Modified version of test_old_mailbox.py
test_old_mailbox.patch amaury.forgeotdarc, 2010-02-16 18:23
issue7944_gzip.diff brian.curtin, 2010-02-22 01:44
issue7944_marshal.diff brian.curtin, 2010-02-22 01:48
issue7944_zipfile64.diff brian.curtin, 2010-02-22 02:21
issue7944_tarfile.diff brian.curtin, 2010-02-22 16:41 This depends on #7232
issue7944_tempfile.diff brian.curtin, 2010-02-22 17:17 Updated with review comments from RDM
test_os.patch giampaolo.rodola, 2010-10-13 17:57 review
issue7944_tarfile_2_7.diff evans.mungai, 2014-08-02 13:36 review
Messages (22)
msg99425 - (view) Author: Dave Fugate (midnightdf) Date: 2010-02-16 17:43
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.
msg99434 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-02-16 18:23
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
msg99617 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-02-20 15:34
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.
msg99618 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-02-20 15:37
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.
msg99619 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2010-02-20 15:39
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.
msg99699 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-02-22 01:44
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.
msg99700 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-02-22 01:48
Here's a patch for test_marshal on trunk.
msg99703 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-02-22 02:21
Here's a patch for test_tempfile on trunk. The rest will just be silently added since this is already annoying.
msg99780 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-02-22 16:41
Patch for test_tarfile. However, #7232 needs to go in first.
msg118498 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-10-13 02:23
Fixed test_gzip in r85400.
msg118500 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-10-13 02:58
Fixed test_mailbox in r85401.
Fixed test_marshal in r85402.
Fixed test_bz2 in r85403.
msg118553 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-10-13 17:57
Here is a patch for test_os.py.
msg118554 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-10-13 18:10
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.
msg118556 - (view) Author: Dave Fugate (midnightdf) Date: 2010-10-13 18:23
I guarantee you that IronPython needs this.  I was a tester on IronPython for four years, and saw hundreds of random failures due to this specific issue.

My best,

Dave
msg118557 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-10-13 18:25
If you're sure that is still the case, this can proceed.
msg118610 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-10-14 02:08
Backported test_gzip to to release27-maint in r85446.
Backported test_mailbox to to release27-maint in r85447.
Backported test_bz2 to to release27-maint in r85448.

Fixed test_old_mailbox, the original problem file, in release27-maint in r85449.
msg185771 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2013-04-01 21:20
#7232 has been closed so test_tarfile can presumably now be committed, also test_os.
msg185817 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-04-02 05:56
FWIW test_tarfile seems to use the "with" statement on 3.3+ already.
On 2.7 is not always used, but I'm not sure it's worth backporting/fixing the remaining occurrences there.
Most of the fixes in the test_os patch have already being applied too.
Either someone updates the current patch and/or proposes new ones, or this can probably be closed.
msg224458 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-31 23:53
Following on from msg185817 I'd suggest this is closed.
msg224564 - (view) Author: Evans Mungai (evans.mungai) Date: 2014-08-02 13:36
Backport for test_tarfile.py
msg258166 - (view) Author: Roundup Robot (python-dev) Date: 2016-01-13 20:21
New changeset e3763d98c46e by Ezio Melotti in branch '2.7':
#7944: close files explicitly in test_tarfile (backport d560eece0857).
https://hg.python.org/cpython/rev/e3763d98c46e
msg258167 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2016-01-13 20:23
Thanks for the patch, however I got several errors while trying to apply it, so I ended up backporting d560eece0857 instead.
This can be closed now.
History
Date User Action Args
2016-01-13 20:23:00ezio.melottisetstatus: open -> closed
messages: + msg258167

assignee: ezio.melotti
resolution: fixed
stage: patch review -> resolved
2016-01-13 20:21:30python-devsetnosy: + python-dev
messages: + msg258166
2014-08-02 13:36:43evans.mungaisetfiles: + issue7944_tarfile_2_7.diff
versions: + Python 2.7, - Python 3.3, Python 3.4
nosy: + evans.mungai

messages: + msg224564
2014-08-01 00:23:58brian.curtinsetnosy: - brian.curtin
2014-07-31 23:53:20BreamoreBoysetnosy: + BreamoreBoy
messages: + msg224458
2014-02-03 18:36:26BreamoreBoysetnosy: - BreamoreBoy
2013-04-02 05:56:13ezio.melottisetkeywords: + easy

messages: + msg185817
versions: + Python 3.3, Python 3.4, - Python 3.2
2013-04-01 21:20:07BreamoreBoysetnosy: + BreamoreBoy
messages: + msg185771
2010-10-14 02:08:04brian.curtinsetmessages: + msg118610
2010-10-13 18:25:03rhettingersetmessages: + msg118557
2010-10-13 18:23:05midnightdfsetmessages: + msg118556
2010-10-13 18:14:37rhettingersetresolution: accepted -> (no value)
2010-10-13 18:10:42rhettingersetnosy: + rhettinger
messages: + msg118554
2010-10-13 17:57:16giampaolo.rodolasetfiles: + test_os.patch
nosy: + giampaolo.rodola
messages: + msg118553

2010-10-13 02:58:27brian.curtinsetmessages: + msg118500
2010-10-13 02:23:55brian.curtinsetresolution: accepted
messages: + msg118498
2010-08-01 01:03:42eric.araujosetnosy: + eric.araujo
2010-07-11 00:01:53terry.reedysetversions: + Python 3.2, - Python 2.6, Python 2.7
2010-02-22 17:17:10brian.curtinsetfiles: + issue7944_tempfile.diff
2010-02-22 17:16:38brian.curtinsetfiles: - issue7944_tempfile.diff
2010-02-22 16:42:01brian.curtinsetfiles: + issue7944_tarfile.diff

messages: + msg99780
2010-02-22 02:21:57brian.curtinsetfiles: + issue7944_zipfile64.diff
2010-02-22 02:21:43brian.curtinsetfiles: + issue7944_tempfile.diff

messages: + msg99703
2010-02-22 01:48:18brian.curtinsetfiles: + issue7944_marshal.diff

messages: + msg99700
2010-02-22 01:44:41brian.curtinsetfiles: + issue7944_gzip.diff

messages: + msg99699
2010-02-22 01:40:21brian.curtinsetdependencies: + Support of 'with' statement fo TarFile class
2010-02-20 15:39:10ezio.melottisetmessages: + msg99619
2010-02-20 15:37:24michael.foordsetnosy: + michael.foord
messages: + msg99618
2010-02-20 15:34:46brian.curtinsetmessages: + msg99617
2010-02-16 23:41:46ezio.melottisetnosy: + ezio.melotti

stage: needs patch -> patch review
2010-02-16 18:23:47amaury.forgeotdarcsetfiles: + test_old_mailbox.patch

nosy: + amaury.forgeotdarc
messages: + msg99434

keywords: + patch
2010-02-16 17:46:55brian.curtinsetpriority: normal
nosy: + brian.curtin

stage: needs patch
2010-02-16 17:43:50midnightdfcreate