Review for issue #10233

Lib/test/test_tarfile.py

Index: Lib/test/test_tarfile.py
===================================================================
--- Lib/test/test_tarfile.py	(révision 85919)
+++ Lib/test/test_tarfile.py	(copie de travail)
@@ -52,25 +52,32 @@
     def test_fileobj_regular_file(self):
         tarinfo = self.tar.getmember("ustar/regtype")
         fobj = self.tar.extractfile(tarinfo)
-        data = fobj.read()
-        self.assertTrue((len(data), md5sum(data)) == (tarinfo.size, md5_regtype),
-                "regular file extraction failed")
+        try:

[brett.cannon, 2010/10/29 23:38:39]

Any reason why you are using a try/finally instead of a context manager? Same
goes for all the other places in this file where you don't use a context
manager.

[Antoine Pitrou, 2010/10/29 23:46:02]

Because that file-like object doesn't support the context manager (which could
be the subject of a separate issue, I suppose).

+            data = fobj.read()
+            self.assertTrue((len(data), md5sum(data)) == (tarinfo.size, md5_regtype),
+                    "regular file extraction failed")
+        finally:
+            fobj.close()
 
     def test_fileobj_readlines(self):
         self.tar.extract("ustar/regtype", TEMPDIR)
         tarinfo = self.tar.getmember("ustar/regtype")
         with open(os.path.join(TEMPDIR, "ustar/regtype"), "r") as fobj1:
             lines1 = fobj1.readlines()
-        fobj2 = io.TextIOWrapper(self.tar.extractfile(tarinfo))
 
-        lines2 = fobj2.readlines()
-        self.assertTrue(lines1 == lines2,
-                "fileobj.readlines() failed")
-        self.assertTrue(len(lines2) == 114,
-                "fileobj.readlines() failed")
-        self.assertTrue(lines2[83] ==
-                "I will gladly admit that Python is not the fastest running scripting language.\n",
-                "fileobj.readlines() failed")
+        fobj = self.tar.extractfile(tarinfo)
+        try:
+            fobj2 = io.TextIOWrapper(fobj)
+            lines2 = fobj2.readlines()
+            self.assertTrue(lines1 == lines2,
+                    "fileobj.readlines() failed")
+            self.assertTrue(len(lines2) == 114,
+                    "fileobj.readlines() failed")
+            self.assertTrue(lines2[83] ==
+                    "I will gladly admit that Python is not the fastest running scripting language.\n",
+                    "fileobj.readlines() failed")
+        finally:
+            fobj.close()
 
     def test_fileobj_iter(self):
         self.tar.extract("ustar/regtype", TEMPDIR)
@@ -78,9 +85,12 @@
         with open(os.path.join(TEMPDIR, "ustar/regtype"), "rU") as fobj1:
             lines1 = fobj1.readlines()
         fobj2 = self.tar.extractfile(tarinfo)
-        lines2 = list(io.TextIOWrapper(fobj2))
-        self.assertTrue(lines1 == lines2,
-                     "fileobj.__iter__() failed")
+        try:
+            lines2 = list(io.TextIOWrapper(fobj2))
+            self.assertTrue(lines1 == lines2,
+                         "fileobj.__iter__() failed")
+        finally:
+            fobj2.close()
 
     def test_fileobj_seek(self):
         self.tar.extract("ustar/regtype", TEMPDIR)
@@ -139,6 +149,8 @@
         a = self.tar.extractfile(lnktype)
         b = self.tar.extractfile(regtype)
         self.assertEqual(a.name, b.name)
+        a.close()

[brett.cannon, 2010/10/29 23:38:39]

If the assertEqual fails these files won't be closed. Better to put them in a
nexted context manager.

[Antoine Pitrou, 2010/10/29 23:46:02]

Ok, will do.

+        b.close()
 
     def test_fileobj_link1(self):
         self._test_fileobj_link("ustar/lnktype", "ustar/regtype")
@@ -225,8 +237,8 @@
             data = fobj.read()
         fobj = io.BytesIO(data)
         fobj.name = ""
-        tar = tarfile.open(fileobj=fobj, mode=self.mode)
-        self.assertEqual(tar.name, None)
+        with tarfile.open(fileobj=fobj, mode=self.mode) as tar:
+            self.assertEqual(tar.name, None)
 
     def test_fileobj_with_offset(self):
         # Skip the first member and store values from the second member
@@ -237,7 +249,9 @@
             t = tar.next()
             name = t.name
             offset = t.offset
-            data = tar.extractfile(t).read()
+            f = tar.extractfile(t)
+            data = f.read()
+            f.close()
         finally:
             tar.close()
 
@@ -319,7 +333,8 @@
                 if e.errno == errno.ENOENT:
                     self.fail("hardlink not extracted properly")
 
-            data = open(os.path.join(TEMPDIR, "ustar/lnktype"), "rb").read()
+            with open(os.path.join(TEMPDIR, "ustar/lnktype"), "rb") as f:
+                data = f.read()
             self.assertEqual(md5sum(data), md5_regtype)
 
             try:
@@ -328,7 +343,8 @@
                 if e.errno == errno.ENOENT:
                     self.fail("symlink not extracted properly")
 
-            data = open(os.path.join(TEMPDIR, "ustar/symtype"), "rb").read()
+            with open(os.path.join(TEMPDIR, "ustar/symtype"), "rb") as f:
+                data = f.read()
             self.assertEqual(md5sum(data), md5_regtype)
         finally:
             tar.close()
@@ -594,10 +610,10 @@
         # the preceding extended header.
         longname = self.subdir + "/" + "123/" * 125 + "longname"
         offset = self.tar.getmember(longname).offset
-        fobj = open(tarname, "rb")
-        fobj.seek(offset)
-        tarinfo = tarfile.TarInfo.frombuf(fobj.read(512), "iso8859-1", "strict")
-        self.assertEqual(tarinfo.type, self.longnametype)
+        with open(tarname, "rb") as fobj:
+            fobj.seek(offset)
+            tarinfo = tarfile.TarInfo.frombuf(fobj.read(512), "iso8859-1", "strict")
+            self.assertEqual(tarinfo.type, self.longnametype)
 
 
 class GNUReadTest(LongnameTest):
@@ -1343,8 +1359,11 @@
             t = src.getmember("ustar/regtype")
             t.name = "foo"
             f = src.extractfile(t)
-            with tarfile.open(self.tarname, mode) as tar:
-                tar.addfile(t, f)
+            try:
+                with tarfile.open(self.tarname, mode) as tar:
+                    tar.addfile(t, f)
+            finally:
+                f.close()
 
     def _test(self, names=["bar"], fileobj=None):
         with tarfile.open(self.tarname, fileobj=fileobj) as tar: