bpo-31602: Fix an assertion failure in zipimporter.get_source() in case of a bad zlib.decompress() by orenmn · Pull Request #3784 · python/cpython

orenmn · 2017-09-27T07:21:46Z

in zipimport.c: add a check whether zlib.decompress() returned a bytes object.
in test_zipimport.py: add a test to verify that the assertion failure is no more.

https://bugs.python.org/issue31602

brettcannon

The fix LGTM, but some tweaks to make to the test to use some specific idioms.

brettcannon · 2017-09-27T17:07:59Z

+        import zlib
+        def bad_decompress(*args):
+            return None
+        z = ZipFile(TEMP_ZIP, 'w')


We tend to prefer people use more descriptive variable names and avoid single-letter ones unless it's a loop counter.

brettcannon · 2017-09-27T17:10:21Z

+                self.assertRaises(TypeError, zi.get_source, 'bar')
+        finally:
+            z.close()
+            os.remove(TEMP_ZIP)


You can use self.addCleanup() so you don't have to specify this in the try block.

brettcannon · 2017-09-27T17:16:37Z

+            with support.swap_attr(zlib, 'decompress', bad_decompress):
+                self.assertRaises(TypeError, zi.get_source, 'bar')
+        finally:
+            z.close()


You also called z.close() above, so I would use my self.addCleanup() suggestion below and rework this to drop the try block and use the context manager for zipfiles:

name = 'bar.py' data = b'spam' with ZipFile(...) as zip_file: self.addCleanup(os.remove, TEMP_ZIP) zip_file.writestr(...) zi = ...

brettcannon · 2017-09-27T17:19:16Z

+    def test_issue31602(self):
+        # There shouldn't be an assertion failure in zipimporter.get_source()
+        # in case of a bad zlib.decompress().
+        import zlib


There is no guarantee the zlib module will be available, so at the top of the module you should do:

try: import zlib except ImportError: zlib = None

Then decorate this method with unittest.skipIf(zlib is None, "zlib not available").

Or use the test.support.requires_zlib decorator.

doesn't this decorator already guarantee that zlib will be available for the test?

@support.requires_zlib class CompressedZipImportTestCase(UncompressedZipImportTestCase):

Yes, it was just out of the visible context.

Ah, then keep the decorator but still switch to the try block at the top for the import (importing in methods is icky).

bedevere-bot · 2017-09-27T17:20:45Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka · 2017-09-27T07:36:40Z

+                self.assertRaises(TypeError, zi.get_source, 'bar')
+        finally:
+            z.close()
+            os.remove(TEMP_ZIP)


It is better to use test.support.unlink().

serhiy-storchaka · 2017-09-27T18:20:02Z

+    def test_issue31602(self):
+        # There shouldn't be an assertion failure in zipimporter.get_source()
+        # in case of a bad zlib.decompress().
+        import zlib


Or use the test.support.requires_zlib decorator.

orenmn · 2017-09-28T06:37:08Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-28T06:37:10Z

Nobody expects the Spanish Inquisition!

@brettcannon: please review the changes made to this pull request.

serhiy-storchaka · 2017-09-28T08:52:41Z

+    def test_issue31602(self):
+        # There shouldn't be an assertion failure in zipimporter.get_source()
+        # in case of a bad zlib.decompress().
+        import zlib


Yes, it was just out of the visible context.

serhiy-storchaka · 2017-09-28T08:52:52Z

+            return None
+        with ZipFile(TEMP_ZIP, 'w') as zip_file:
+            self.addCleanup(support.unlink, TEMP_ZIP)
+            name = 'bar.py'


Why not inline these values? For clarity use a valid Python statement instead of 'spam'.

No idea why not. I just copied the style of all other tests in this file.

brettcannon

One very minor cleanup left and then this PR is ready for merging!

brettcannon · 2017-09-29T17:01:48Z

+    def test_issue31602(self):
+        # There shouldn't be an assertion failure in zipimporter.get_source()
+        # in case of a bad zlib.decompress().
+        import zlib


Ah, then keep the decorator but still switch to the try block at the top for the import (importing in methods is icky).

bedevere-bot · 2017-09-29T17:10:58Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka

The test now looks much nicer!

brettcannon · 2017-09-29T18:18:48Z

It's so weird to be looking at the latest update to the PR and have the latest commit from @orenmn literally land right in front of me. 😄

orenmn · 2017-09-29T18:24:42Z

This happened only thanks to Murphy's law.
On my machine test_zipimport passed, but travis failed immediately.
I am not sure why. Maybe because testAFakeZlib() messes with zlib?

brettcannon · 2017-09-29T18:31:35Z

The failure was from an automatic cancellation on the part of Travis; it decided to start a new build and thus cancelled the already-running one. But everything is good now!

brettcannon · 2017-09-29T18:34:47Z

Thanks for the pull request!

orenmn added 4 commits September 27, 2017 09:46

init commit

c620131

fix NEWS.d item

1d8763a

add @cpython_only

65a70a2

use ZIP_DEFLATED explicitly in the test

5c44836

orenmn requested a review from a team September 27, 2017 07:21

the-knights-who-say-ni added the CLA signed label Sep 27, 2017

bedevere-bot added the awaiting review label Sep 27, 2017

brettcannon requested changes Sep 27, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 27, 2017

serhiy-storchaka reviewed Sep 27, 2017

View reviewed changes

improve the test

641b962

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 28, 2017

serhiy-storchaka reviewed Sep 28, 2017

View reviewed changes

improve the test

2100a21

brettcannon requested changes Sep 29, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Sep 29, 2017

serhiy-storchaka approved these changes Sep 29, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 29, 2017

move the import out of the test

ee9461b

brettcannon approved these changes Sep 29, 2017

View reviewed changes

brettcannon merged commit 01c6a88 into python:master Sep 29, 2017

Mariatta removed the awaiting merge label Oct 8, 2017

Uh oh!

Conversation

orenmn commented Sep 27, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenmn Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenmn commented Sep 28, 2017

Uh oh!

bedevere-bot commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 29, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Sep 29, 2017

Uh oh!

orenmn commented Sep 29, 2017

Uh oh!

brettcannon commented Sep 29, 2017

Uh oh!

brettcannon commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

orenmn commented Sep 27, 2017 •

edited by bedevere-bot

Loading

orenmn Sep 28, 2017 •

edited

Loading