bpo-31602: Fix an assertion failure in zipimporter.get_source() in case of a bad zlib.decompress() #3784

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

Lib/test/test_zipimport.py

+        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

Lib/test/test_zipimport.py

+                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

Lib/test/test_zipimport.py

+            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

Lib/test/test_zipimport.py

+    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

Lib/test/test_zipimport.py

+                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

Lib/test/test_zipimport.py

+    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

Lib/test/test_zipimport.py

+    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

Lib/test/test_zipimport.py

+            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

Lib/test/test_zipimport.py

+    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31602: Fix an assertion failure in zipimporter.get_source() in case of a bad zlib.decompress() #3784

bpo-31602: Fix an assertion failure in zipimporter.get_source() in case of a bad zlib.decompress() #3784

orenmn commented Sep 27, 2017 •

edited by bedevere-bot

Loading

brettcannon left a comment

brettcannon Sep 27, 2017

brettcannon Sep 27, 2017

brettcannon Sep 27, 2017

brettcannon Sep 27, 2017

serhiy-storchaka Sep 27, 2017

orenmn Sep 28, 2017 •

edited

Loading

serhiy-storchaka Sep 28, 2017

brettcannon Sep 29, 2017

bedevere-bot commented Sep 27, 2017

serhiy-storchaka Sep 27, 2017

serhiy-storchaka Sep 27, 2017

orenmn commented Sep 28, 2017

bedevere-bot commented Sep 28, 2017

serhiy-storchaka Sep 28, 2017

serhiy-storchaka Sep 28, 2017

orenmn Sep 28, 2017

brettcannon left a comment

brettcannon Sep 29, 2017

bedevere-bot commented Sep 29, 2017

serhiy-storchaka left a comment

brettcannon commented Sep 29, 2017

orenmn commented Sep 29, 2017

brettcannon commented Sep 29, 2017

brettcannon commented Sep 29, 2017

bpo-31602: Fix an assertion failure in zipimporter.get_source() in case of a bad zlib.decompress() #3784

bpo-31602: Fix an assertion failure in zipimporter.get_source() in case of a bad zlib.decompress() #3784

Conversation

orenmn commented Sep 27, 2017 • edited by bedevere-bot Loading

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenmn Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenmn commented Sep 28, 2017

bedevere-bot commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 29, 2017

serhiy-storchaka left a comment

Choose a reason for hiding this comment

brettcannon commented Sep 29, 2017

orenmn commented Sep 29, 2017

brettcannon commented Sep 29, 2017

brettcannon commented Sep 29, 2017

orenmn commented Sep 27, 2017 •

edited by bedevere-bot

Loading

orenmn Sep 28, 2017 •

edited

Loading