From e0528029b345531cbcc51bb2fcf5289744905fec Mon Sep 17 00:00:00 2001 From: Matt Chung Date: Wed, 3 Feb 2016 11:12:42 +0000 Subject: [PATCH 1/3] Removed tests/test_repos.py Removed duplicated TestAsset in tests_repos_repos; TestAsset exists in test_repos_release. Added test_str, and test_download to TestAsset. Updated repos_asset_example's JSON. --- github3/repos/release.py | 2 +- tests/test_repos.py | 82 ----------------------------- tests/unit/json/asset_example | 32 ----------- tests/unit/json/repos_asset_example | 38 +++++++++---- tests/unit/test_repos_release.py | 24 +++++++++ tests/unit/test_repos_repo.py | 21 -------- 6 files changed, 54 insertions(+), 145 deletions(-) delete mode 100644 tests/test_repos.py delete mode 100644 tests/unit/json/asset_example diff --git a/github3/repos/release.py b/github3/repos/release.py index 2d6c8c6e4..7f55adc6a 100644 --- a/github3/repos/release.py +++ b/github3/repos/release.py @@ -227,7 +227,7 @@ def download(self, path=''): } resp = self._get(self._api, allow_redirects=False, stream=True, headers=headers) - if resp.status_code == 302: + if resp and resp.status_code == 302: # Amazon S3 will reject the redirected request unless we omit # certain request headers headers.update({ diff --git a/tests/test_repos.py b/tests/test_repos.py deleted file mode 100644 index 3a48dc042..000000000 --- a/tests/test_repos.py +++ /dev/null @@ -1,82 +0,0 @@ -import os -import github3 -import pytest -from github3 import repos -from tests.utils import (BaseCase, load, mock) - - -class TestAsset(BaseCase): - def __init__(self, methodName='runTest'): - super(TestAsset, self).__init__(methodName) - self.asset = repos.release.Asset(load('asset')) - self.api = ("https://api.github.com/repos/sigmavirus24/github3.py/" - "releases/assets/37945") - - @pytest.mark.xfail - def test_download(self): - headers = {'content-disposition': 'filename=foo'} - self.response('archive', 200, **headers) - self.get(self.api) - self.conf.update({ - 'stream': True, - 'allow_redirects': False, - 'headers': {'Accept': 'application/octet-stream'} - }) - - # 200, to default location - assert os.path.isfile('foo') is False - assert self.asset.download() - assert os.path.isfile('foo') - os.unlink('foo') - self.mock_assertions() - - self.request.return_value.raw.seek(0) - self.request.return_value._content_consumed = False - - # 200, to path - assert os.path.isfile('path_to_file') is False - assert self.asset.download('path_to_file') - assert os.path.isfile('path_to_file') - os.unlink('path_to_file') - self.mock_assertions() - - self.request.return_value.raw.seek(0) - self.request.return_value._content_consumed = False - - # 200, to file-like object - o = mock.mock_open() - with mock.patch('{0}.open'.format(__name__), o, create=True): - with open('download', 'wb+') as fd: - self.asset.download(fd) - o.assert_called_once_with('download', 'wb+') - fd = o() - fd.write.assert_called_once_with(b'archive_data') - self.mock_assertions() - - self.request.return_value.raw.seek(0) - self.request.return_value._content_consumed = False - - # 302, to file-like object - r = self.request.return_value - target = 'http://github.s3.example.com/foo' - self.response('', 302, location=target) - self.get(target) - self.request.side_effect = [self.request.return_value, r] - self.conf['headers'].update({ - 'Authorization': None, - 'Content-Type': None, - }) - del self.conf['allow_redirects'] - o = mock.mock_open() - with mock.patch('{0}.open'.format(__name__), o, create=True): - with open('download', 'wb+') as fd: - self.asset.download(fd) - o.assert_called_once_with('download', 'wb+') - fd = o() - fd.write.assert_called_once_with(b'archive_data') - self.mock_assertions() - - # 404 - self.response('', 404) - self.request.side_effect = None - assert self.asset.download() is False diff --git a/tests/unit/json/asset_example b/tests/unit/json/asset_example deleted file mode 100644 index b08d11ceb..000000000 --- a/tests/unit/json/asset_example +++ /dev/null @@ -1,32 +0,0 @@ -{ - "url": "https://api.github.com/repos/octocat/Hello-World/releases/assets/1", - "browser_download_url": "https://github.com/octocat/Hello-World/releases/download/v1.0.0/example.zip", - "id": 1, - "name": "example.zip", - "label": "short description", - "state": "uploaded", - "content_type": "application/zip", - "size": 1024, - "download_count": 42, - "created_at": "2013-02-27T19:35:32Z", - "updated_at": "2013-02-27T19:35:32Z", - "uploader": { - "login": "octocat", - "id": 1, - "avatar_url": "https://github.com/images/error/octocat_happy.gif", - "gravatar_id": "", - "url": "https://api.github.com/users/octocat", - "html_url": "https://github.com/octocat", - "followers_url": "https://api.github.com/users/octocat/followers", - "following_url": "https://api.github.com/users/octocat/following{/other_user}", - "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", - "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", - "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", - "organizations_url": "https://api.github.com/users/octocat/orgs", - "repos_url": "https://api.github.com/users/octocat/repos", - "events_url": "https://api.github.com/users/octocat/events{/privacy}", - "received_events_url": "https://api.github.com/users/octocat/received_events", - "type": "User", - "site_admin": false - } -} diff --git a/tests/unit/json/repos_asset_example b/tests/unit/json/repos_asset_example index 063e80022..b08d11ceb 100644 --- a/tests/unit/json/repos_asset_example +++ b/tests/unit/json/repos_asset_example @@ -1,12 +1,32 @@ { - "url": "https://api.github.com/repos/octocat/Hello-World/releases/assets/1", + "url": "https://api.github.com/repos/octocat/Hello-World/releases/assets/1", + "browser_download_url": "https://github.com/octocat/Hello-World/releases/download/v1.0.0/example.zip", + "id": 1, + "name": "example.zip", + "label": "short description", + "state": "uploaded", + "content_type": "application/zip", + "size": 1024, + "download_count": 42, + "created_at": "2013-02-27T19:35:32Z", + "updated_at": "2013-02-27T19:35:32Z", + "uploader": { + "login": "octocat", "id": 1, - "name": "example.zip", - "label": "short description", - "state": "uploaded", - "content_type": "application/zip", - "size": 1024, - "download_count": 42, - "created_at": "2013-02-27T19:35:32Z", - "updated_at": "2013-02-27T19:35:32Z" + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "url": "https://api.github.com/users/octocat", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "repos_url": "https://api.github.com/users/octocat/repos", + "events_url": "https://api.github.com/users/octocat/events{/privacy}", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": false + } } diff --git a/tests/unit/test_repos_release.py b/tests/unit/test_repos_release.py index 4677c1cf4..386e39539 100644 --- a/tests/unit/test_repos_release.py +++ b/tests/unit/test_repos_release.py @@ -3,6 +3,7 @@ from .helper import (UnitHelper, UnitIteratorHelper, create_url_helper, mock, create_example_data_helper) +import github3 import json import pytest @@ -140,6 +141,25 @@ def test_download(self): ) assert stream.called is False + def test_download_with_302(self): + """Verify the request to download an Asset file.""" + with mock.patch.object(github3.models.GitHubCore, '_get') as get: + get.return_value.status_code = 302 + get.return_value.headers = {'location': 'https://fakeurl'} + self.instance.download() + data = { + 'headers': { + 'Content-Type': None, + 'Accept': 'application/octet-stream' + }, + 'stream': True + } + assert get.call_count == 2 + get.assert_any_call( + 'https://fakeurl', + **data + ) + def test_edit_without_label(self): self.instance.edit('new name') self.session.patch.assert_called_once_with( @@ -157,3 +177,7 @@ def test_edit_with_label(self): assert json.loads(kwargs['data']) == { 'name': 'new name', 'label': 'label' } + + def test_str(self): + """Verify that instance string is formatted correctly.""" + assert str(self.instance).startswith(' Date: Sun, 7 Feb 2016 09:55:45 +0000 Subject: [PATCH 2/3] Removed unnecessary defensive check Per @sigmavirus24, proxy to requests always return a response unless an exception is raised. --- github3/repos/release.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github3/repos/release.py b/github3/repos/release.py index 7f55adc6a..2d6c8c6e4 100644 --- a/github3/repos/release.py +++ b/github3/repos/release.py @@ -227,7 +227,7 @@ def download(self, path=''): } resp = self._get(self._api, allow_redirects=False, stream=True, headers=headers) - if resp and resp.status_code == 302: + if resp.status_code == 302: # Amazon S3 will reject the redirected request unless we omit # certain request headers headers.update({ From 3af126906daddf309b124342f2612a109b2b1ca5 Mon Sep 17 00:00:00 2001 From: Matt Chung Date: Mon, 8 Feb 2016 07:29:18 +0000 Subject: [PATCH 3/3] Removed test_str from Asset test case Per @sigmavirus24 directive --- tests/unit/test_repos_release.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/unit/test_repos_release.py b/tests/unit/test_repos_release.py index 386e39539..7f22ac019 100644 --- a/tests/unit/test_repos_release.py +++ b/tests/unit/test_repos_release.py @@ -177,7 +177,3 @@ def test_edit_with_label(self): assert json.loads(kwargs['data']) == { 'name': 'new name', 'label': 'label' } - - def test_str(self): - """Verify that instance string is formatted correctly.""" - assert str(self.instance).startswith('