Patch #1675424: Added tests for uncovered code in the zipfile module.

The KeyError raised by Zipfile.getinfo for nonexistent names now has
  a descriptive message.

Doc/lib/libzipfile.tex

@@ -24,8 +24,8 @@ it cannot currently create an encrypted file.
 
 The available attributes of this module are:
 
-\begin{excdesc}{error}
+\begin{excdesc}{BadZipfile}
-  The error raised for bad ZIP files.
+  The error raised for bad ZIP files (old name: \code{zipfile.error}).
 \end{excdesc}
 
 \begin{excdesc}{LargeZipFile}
@@ -90,7 +90,7 @@ The available attributes of this module are:
   (a string) or a file-like object.  The \var{mode} parameter
   should be \code{'r'} to read an existing file, \code{'w'} to
   truncate and write a new file, or \code{'a'} to append to an
-  existing file.  For \var{mode} is \code{'a'} and \var{file}
+  existing file.  If \var{mode} is \code{'a'} and \var{file}
   refers to an existing ZIP file, then additional files are added to
   it.  If \var{file} does not refer to a ZIP file, then a new ZIP
   archive is appended to the file.  This is meant for adding a ZIP
@@ -128,7 +128,8 @@ cat myzip.zip >> python.exe
 
 \begin{methoddesc}{getinfo}{name}
   Return a \class{ZipInfo} object with information about the archive
-  member \var{name}.
+  member \var{name}.  Calling \method{getinfo()} for a name not currently
+  contained in the archive will raise a \exception{KeyError}.
 \end{methoddesc}
 
 \begin{methoddesc}{infolist}{}
@@ -147,7 +148,9 @@ cat myzip.zip >> python.exe
     parameter, if included, must be one of the following: \code{'r'} (the 
     default), \code{'U'}, or \code{'rU'}. Choosing \code{'U'} or 
     \code{'rU'} will enable universal newline support in the read-only
-    object. \var{pwd} is the password used for encrypted files.
+    object. \var{pwd} is the password used for encrypted files.  Calling 
+    \method{open()} on a closed ZipFile will raise a 
+    \exception{RuntimeError}.
     \begin{notice}
         The file-like object is read-only and provides the following methods:
         \method{read()}, \method{readline()}, \method{readlines()},
@@ -182,7 +185,8 @@ cat myzip.zip >> python.exe
   Return the bytes of the file in the archive.  The archive must be
   open for read or append. \var{pwd} is the password used for encrypted 
   files and, if specified, it will override the default password set with
-  \method{setpassword()}.
+  \method{setpassword()}.  Calling \method{read()} on a closed ZipFile 
+  will raise a \exception{RuntimeError}.
 
   \versionchanged[\var{pwd} was added]{2.6}
 \end{methoddesc}
@@ -190,6 +194,8 @@ cat myzip.zip >> python.exe
 \begin{methoddesc}{testzip}{}
   Read all the files in the archive and check their CRC's and file
   headers.  Return the name of the first bad file, or else return \code{None}.
+  Calling \method{testzip()} on a closed ZipFile will raise a
+  \exception{RuntimeError}.
 \end{methoddesc}
 
 \begin{methoddesc}{write}{filename\optional{, arcname\optional{,
@@ -200,7 +206,10 @@ cat myzip.zip >> python.exe
   separators removed).  If given, \var{compress_type} overrides the
   value given for the \var{compression} parameter to the constructor
   for the new entry.  The archive must be open with mode \code{'w'}
-  or \code{'a'}.
+  or \code{'a'} -- calling \method{write()} on a ZipFile created with
+  mode \code{'r'} will raise a \exception{RuntimeError}.  Calling 
+  \method{write()} on a closed ZipFile will raise a 
+  \exception{RuntimeError}.
   
   \note{There is no official file name encoding for ZIP files.
   If you have unicode file names, please convert them to byte strings
@@ -210,6 +219,11 @@ cat myzip.zip >> python.exe
 
   \note{Archive names should be relative to the archive root, that is,
         they should not start with a path separator.}
+
+  \note{If \code{arcname} (or \code{filename}, if \code{arcname} is 
+  not given) contains a null byte, the name of the file in the archive will
+  be truncated at the null byte.}
+
 \end{methoddesc}
 
 \begin{methoddesc}{writestr}{zinfo_or_arcname, bytes}
@@ -218,7 +232,10 @@ cat myzip.zip >> python.exe
   \class{ZipInfo} instance.  If it's an instance, at least the
   filename, date, and time must be given.  If it's a name, the date
   and time is set to the current date and time. The archive must be
-  opened with mode \code{'w'} or \code{'a'}.
+  opened with mode \code{'w'} or \code{'a'} -- calling 
+  \method{writestr()} on a ZipFile created with mode \code{'r'} 
+  will raise a \exception{RuntimeError}.  Calling \method{writestr()}
+  on a closed ZipFile will raise a \exception{RuntimeError}.
 \end{methoddesc}
 
 
@@ -243,12 +260,13 @@ those of \class{ZipFile} objects.
   available, else a \file{*.pyc} file, compiling if necessary.  If the
   pathname is a file, the filename must end with \file{.py}, and just
   the (corresponding \file{*.py[co]}) file is added at the top level
-  (no path information).  If it is a directory, and the directory is
+  (no path information).  If the pathname is a file that does not end with
-  not a package directory, then all the files \file{*.py[co]} are
+  \file{.py}, a \exception{RuntimeError} will be raised.  If it is a
-  added at the top level.  If the directory is a package directory,
+  directory, and the directory is not a package directory, then all the
-  then all \file{*.py[oc]} are added under the package name as a file
+  files \file{*.py[co]} are added at the top level.  If the directory is
-  path, and if any subdirectories are package directories, all of
+  a package directory, then all \file{*.py[co]} are added under the package
-  these are added recursively.  \var{basename} is intended for
+  name as a file path, and if any subdirectories are package directories, all
+  of these are added recursively.  \var{basename} is intended for
   internal use only.  The \method{writepy()} method makes archives
   with file names like this:
 

Lib/test/test_zipfile.py

@@ -14,12 +14,12 @@ import test.test_support as support
 from test.test_support import TESTFN, run_unittest
 
 TESTFN2 = TESTFN + "2"
-FIXEDTEST_SIZE = 10
+FIXEDTEST_SIZE = 1000
 
 class TestsWithSourceFile(unittest.TestCase):
     def setUp(self):
-        self.line_gen = ("Zipfile test line %d. random float: %f" % (i, random())
+        self.line_gen = ["Zipfile test line %d. random float: %f" % (i, random())
-                          for i in xrange(FIXEDTEST_SIZE))
+                          for i in xrange(FIXEDTEST_SIZE)]
         self.data = '\n'.join(self.line_gen) + '\n'
 
         # Make a source file with some lines
@@ -239,6 +239,63 @@ class TestsWithSourceFile(unittest.TestCase):
         self.assertEqual(zipfp.namelist(), ["absolute"])
         zipfp.close()
 
+    def testAppendToZipFile(self):
+        # Test appending to an existing zipfile
+        zipfp = zipfile.ZipFile(TESTFN2, "w", zipfile.ZIP_STORED)
+        zipfp.write(TESTFN, TESTFN)
+        zipfp.close()
+        zipfp = zipfile.ZipFile(TESTFN2, "a", zipfile.ZIP_STORED)
+        zipfp.writestr("strfile", self.data)
+        self.assertEqual(zipfp.namelist(), [TESTFN, "strfile"])
+        zipfp.close()
+
+    def testAppendToNonZipFile(self):
+        # Test appending to an existing file that is not a zipfile
+        # NOTE: this test fails if len(d) < 22 because of the first
+        # line "fpin.seek(-22, 2)" in _EndRecData
+        d = 'I am not a ZipFile!'*10
+        f = file(TESTFN2, 'wb')
+        f.write(d)
+        f.close()
+        zipfp = zipfile.ZipFile(TESTFN2, "a", zipfile.ZIP_STORED)
+        zipfp.write(TESTFN, TESTFN)
+        zipfp.close()
+
+        f = file(TESTFN2, 'rb')
+        f.seek(len(d))
+        zipfp = zipfile.ZipFile(f, "r")
+        self.assertEqual(zipfp.namelist(), [TESTFN])
+        zipfp.close()
+        f.close()
+
+    def test_WriteDefaultName(self):
+        # Check that calling ZipFile.write without arcname specified produces the expected result
+        zipfp = zipfile.ZipFile(TESTFN2, "w")
+        zipfp.write(TESTFN)
+        self.assertEqual(zipfp.read(TESTFN), file(TESTFN).read())
+        zipfp.close()
+
+    def test_PerFileCompression(self):
+        # Check that files within a Zip archive can have different compression options
+        zipfp = zipfile.ZipFile(TESTFN2, "w")
+        zipfp.write(TESTFN, 'storeme', zipfile.ZIP_STORED)
+        zipfp.write(TESTFN, 'deflateme', zipfile.ZIP_DEFLATED)
+        sinfo = zipfp.getinfo('storeme')
+        dinfo = zipfp.getinfo('deflateme')
+        self.assertEqual(sinfo.compress_type, zipfile.ZIP_STORED)
+        self.assertEqual(dinfo.compress_type, zipfile.ZIP_DEFLATED)
+        zipfp.close()
+
+    def test_WriteToReadonly(self):
+        # Check that trying to call write() on a readonly ZipFile object
+        # raises a RuntimeError
+        zipf = zipfile.ZipFile(TESTFN2, mode="w")
+        zipf.writestr("somefile.txt", "bogus")
+        zipf.close()
+        zipf = zipfile.ZipFile(TESTFN2, mode="r")
+        self.assertRaises(RuntimeError, zipf.write, TESTFN)
+        zipf.close()
+
     def tearDown(self):
         os.remove(TESTFN)
         os.remove(TESTFN2)
@@ -361,7 +418,6 @@ class TestZip64InSmallFiles(unittest.TestCase):
         self.assertEqual(zipfp.namelist(), ["absolute"])
         zipfp.close()
 
-
     def tearDown(self):
         zipfile.ZIP64_LIMIT = self._limit
         os.remove(TESTFN)
@@ -432,6 +488,11 @@ class PyZipFileTests(unittest.TestCase):
         finally:
             shutil.rmtree(TESTFN2)
 
+    def testWriteNonPyfile(self):
+        zipfp  = zipfile.PyZipFile(TemporaryFile(), "w")
+        file(TESTFN, 'w').write('most definitely not a python file')
+        self.assertRaises(RuntimeError, zipfp.writepy, TESTFN)
+        os.remove(TESTFN)
 
 
 class OtherTests(unittest.TestCase):
@@ -513,7 +574,56 @@ class OtherTests(unittest.TestCase):
         # a RuntimeError, and so should calling .testzip.  An earlier
         # version of .testzip would swallow this exception (and any other)
         # and report that the first file in the archive was corrupt.
+        self.assertRaises(RuntimeError, zipf.read, "foo.txt")
+        self.assertRaises(RuntimeError, zipf.open, "foo.txt")
         self.assertRaises(RuntimeError, zipf.testzip)
+        self.assertRaises(RuntimeError, zipf.writestr, "bogus.txt", "bogus")
+        file(TESTFN, 'w').write('zipfile test data')
+        self.assertRaises(RuntimeError, zipf.write, TESTFN)
+
+    def test_BadConstructorMode(self):
+        # Check that bad modes passed to ZipFile constructor are caught
+        self.assertRaises(RuntimeError, zipfile.ZipFile, TESTFN, "q")
+
+    def test_BadOpenMode(self):
+        # Check that bad modes passed to ZipFile.open are caught
+        zipf = zipfile.ZipFile(TESTFN, mode="w")
+        zipf.writestr("foo.txt", "O, for a Muse of Fire!")
+        zipf.close()
+        zipf = zipfile.ZipFile(TESTFN, mode="r")
+        # read the data to make sure the file is there
+        zipf.read("foo.txt")
+        self.assertRaises(RuntimeError, zipf.open, "foo.txt", "q")
+        zipf.close()
+
+    def test_Read0(self):
+        # Check that calling read(0) on a ZipExtFile object returns an empty
+        # string and doesn't advance file pointer
+        zipf = zipfile.ZipFile(TESTFN, mode="w")
+        zipf.writestr("foo.txt", "O, for a Muse of Fire!")
+        # read the data to make sure the file is there
+        f = zipf.open("foo.txt")
+        for i in xrange(FIXEDTEST_SIZE):
+            self.assertEqual(f.read(0), '')
+
+        self.assertEqual(f.read(), "O, for a Muse of Fire!")
+        zipf.close()
+
+    def test_OpenNonexistentItem(self):
+        # Check that attempting to call open() for an item that doesn't
+        # exist in the archive raises a RuntimeError
+        zipf = zipfile.ZipFile(TESTFN, mode="w")
+        self.assertRaises(KeyError, zipf.open, "foo.txt", "r")
+
+    def test_BadCompressionMode(self):
+        # Check that bad compression methods passed to ZipFile.open are caught
+        self.assertRaises(RuntimeError, zipfile.ZipFile, TESTFN, "w", -1)
+
+    def test_NullByteInFilename(self):
+        # Check that a filename containing a null byte is properly terminated
+        zipf = zipfile.ZipFile(TESTFN, mode="w")
+        zipf.writestr("foo.txt\x00qqq", "O, for a Muse of Fire!")
+        self.assertEqual(zipf.namelist(), ['foo.txt'])
 
     def tearDown(self):
         support.unlink(TESTFN)

Lib/zipfile.py

@@ -568,8 +568,9 @@ class ZipFile:
 
     def __init__(self, file, mode="r", compression=ZIP_STORED, allowZip64=False):
         """Open the ZIP file with mode read "r", write "w" or append "a"."""
-        self._allowZip64 = allowZip64
+        if mode not in ("r", "w", "a"):
-        self._didModify = False
+            raise RuntimeError('ZipFile() requires mode "r", "w", or "a"')
+
         if compression == ZIP_STORED:
             pass
         elif compression == ZIP_DEFLATED:
@@ -578,6 +579,9 @@ class ZipFile:
                       "Compression requires the (missing) zlib module"
         else:
             raise RuntimeError, "That compression method is not supported"
+
+        self._allowZip64 = allowZip64
+        self._didModify = False
         self.debug = 0  # Level of printing: 0 through 3
         self.NameToInfo = {}    # Find file info given name
         self.filelist = []      # List of ZipInfo instances for archive
@@ -720,7 +724,12 @@ class ZipFile:
 
     def getinfo(self, name):
         """Return the instance of ZipInfo given 'name'."""
-        return self.NameToInfo[name]
+        info = self.NameToInfo.get(name)
+        if info is None:
+            raise KeyError(
+                'There is no item named %r in the archive' % name)
+
+        return info
 
     def setpassword(self, pwd):
         """Set default password for encrypted files."""
@@ -824,6 +833,10 @@ class ZipFile:
     def write(self, filename, arcname=None, compress_type=None):
         """Put the bytes from filename into the archive under the name
         arcname."""
+        if not self.fp:
+            raise RuntimeError(
+                  "Attempt to write to ZIP archive that was already closed")
+
         st = os.stat(filename)
         mtime = time.localtime(st.st_mtime)
         date_time = mtime[0:6]
@@ -896,6 +909,11 @@ class ZipFile:
             zinfo.compress_type = self.compression
         else:
             zinfo = zinfo_or_arcname
+
+        if not self.fp:
+            raise RuntimeError(
+                  "Attempt to write to ZIP archive that was already closed")
+
         zinfo.file_size = len(bytes)            # Uncompressed size
         zinfo.header_offset = self.fp.tell()    # Start of header bytes
         self._writecheck(zinfo)

Misc/NEWS

@@ -420,6 +420,10 @@ Library
 
 - Patch #1481079: add support for HTTP_REFERER to CGIHTTPServer.
 
+- Patch #1675424: Added tests for uncovered code in the zipfile module. 
+  The KeyError raised by Zipfile.getinfo for nonexistent names now has 
+  a descriptive message.
+
 - Bug #1115886: os.path.splitext('.cshrc') gives now ('.cshrc', '').
 
 - unittest now verifies more of its assumptions. In particular, TestCase