Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add handling of struct.error during Image.verify #78

Closed
melinath opened this issue Mar 2, 2016 · 6 comments
Closed

Add handling of struct.error during Image.verify #78

melinath opened this issue Mar 2, 2016 · 6 comments
Labels
Bug High Priority
Milestone
2.2.2

Comments

@melinath
Copy link
Owner

melinath commented Mar 2, 2016

See python-pillow/Pillow#1755. Pillow now throws struct.error in certain situations where we expect something else (specifically IndexError). Though that may or may not be the proper behavior, there are at least 3 versions of Pillow out with this behavior; we should handle it.
@melinath melinath modified the milestone: 2.2.2 May 6, 2016
@melinath
Copy link
Owner Author

melinath commented May 6, 2016

It looks like the latest version now raises either SyntaxError or IOError :-/ See python-pillow/Pillow#1805. Not sure if that's been released yet.

I don't really want to test every version of Pillow with every different version of Python for just this one test. Maybe we could use mock side effects to test that each option is handled?

@chigby
Copy link
Contributor

chigby commented May 6, 2016

I tried writing some tests for this and they all passed without me doing anything. Looks like the only place we call Image.verify is here: https://github.com/littleweaver/django-daguerre/blob/master/daguerre/helpers.py#L354 -- and we except everything.

@melinath is your proposal to replace except Exception with the three exceptions we know could potentially be raised?

@melinath
Copy link
Owner Author

melinath commented May 6, 2016

@chigby This ticket was based on a reported (or experienced) bug. I don't remember the details any more. We except every subclass of Exception, you're right. Maybe struct.error is not a subclass of Exception? But you said you tested it and it was caught?

@melinath
Copy link
Owner Author

melinath commented May 6, 2016

Oh, the place it was failing was itself a test case which was looking for a specific error.

@melinath
Copy link
Owner Author

melinath commented May 6, 2016

I guess just updating that test case to make sure the exception is an Exception subclass would be fine.

@chigby
Copy link
Contributor

chigby commented May 6, 2016

Ahhh, okay, the problem is in the test. I can fix that.

chigby added a commit that referenced this issue May 9, 2016
@chigby
Created tests for ways that Image.verify can fail across different …
6c5031f 
…versions of PIL

1. Use mock objects to simulate the different errors PIL raises when it has problems verifying an image.
2. There is one test that does the verification without mocking, this is sort of future-proofing in case they change the errors again.
3. Removed the assertion on Image.verify -- this is not really our code to test.
4. Removed `except Exception` which I think casts too broad a net. If we think it is necessary I can put it back.

Fixed #78
@chigby chigby mentioned this issue May 9, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug High Priority
Projects
None yet
Milestone
2.2.2
Development

No branches or pull requests
2 participants
@melinath @chigby