Skip to content
This repository has been archived by the owner on Mar 15, 2018. It is now read-only.

Commit

Permalink
do not check SSL certs for Reviewer Tools' manifest viewer (bug 784000)
Browse files Browse the repository at this point in the history
  • Loading branch information
@cvan
cvan committed Aug 20, 2012
1 parent 98239cb commit 50904f2
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion mkt/reviewers/views.py
View file
Expand Up @@ -315,7 +315,7 @@ def app_view_manifest(request, addon):
content, headers = '', {}
if addon.manifest_url:
try:
req = requests.get(addon.manifest_url)

This comment has been minimized.

Sign in to view

Copy link
@kumar303

kumar303 Aug 21, 2012

Contributor

it would be great to add a comment here so that someone doesn't "fix" the code when reading it.
req = requests.get(addon.manifest_url, verify=False)
content, headers = req.content, req.headers
except Exception, e:
content = e
Expand Down

7 comments on commit 50904f2

@robhudson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no tests? 😏

@cvan
Copy link
Contributor Author

@cvan cvan commented on 50904f2 Aug 20, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and it fixed the bug ;) ideas how to unit test this?

@robhudson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno exactly. I'd look at mocking whatever requests calls and add a side effect that throws SSLError (or whatever it is)? I was half joking, but for regressions it might be a good test?

@cvan
Copy link
Contributor Author

@cvan cvan commented on 50904f2 Aug 20, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no, I definitely thought of it but I don't know how to cause a "side effect" that would throw an SSLError without making a network connection?

@robhudson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like...

_get = mock.Mock()
_get.side_effect = requests.exceptions.SSLError
_requests = mock.Mock()  # This would be via the decorator probably.
_requests.get = _get

Then call your method?

@kumar303
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just add a test that makes sure verify=False is passed in to a mock. That way when code gets refactored around you won't forget that it needs to be False. Fudge is great for this :)

@cvan
Copy link
Contributor Author

@cvan cvan commented on 50904f2 Aug 21, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great advice, you two! I added a test in edfee97 but, also, good call, @kumar303

Please sign in to comment.