From c06cd39cab35e68ae0bfa5a865acfc08cc996baf Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 20 Sep 2017 09:55:21 -0700 Subject: [PATCH] Fix up all the tests --- openlp/core/common/httputils.py | 10 +- .../openlp_core_common/test_httputils.py | 208 ++++++++---------- .../openlp_core_ui/test_first_time.py | 26 +-- .../openlp_core_ui/test_firsttimeform.py | 25 +-- 4 files changed, 118 insertions(+), 151 deletions(-) diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index 0a5c962f8..d02626389 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -86,14 +86,13 @@ def get_web_page(url, headers=None, update_openlp=False, proxies=None): """ if not url: return None - if headers and 'user-agent' not in [key.lower() for key in headers.keys()]: + if not headers: + headers = {} + if 'user-agent' not in [key.lower() for key in headers.keys()]: headers['User-Agent'] = get_user_agent() log.debug('Downloading URL = %s' % url) retries = 0 while retries < CONNECTION_RETRIES: - # Put this at the bottom - # retries += 1 - # time.sleep(0.1) try: response = requests.get(url, headers=headers, proxies=proxies, timeout=float(CONNECTION_TIMEOUT)) log.debug('Downloaded page {url}'.format(url=response.url)) @@ -102,8 +101,9 @@ def get_web_page(url, headers=None, update_openlp=False, proxies=None): # For now, catch IOError. All requests errors inherit from IOError log.exception('Unable to connect to {url}'.format(url=url)) response = None - if retries > CONNECTION_RETRIES: + if retries >= CONNECTION_RETRIES: raise ConnectionError('Unable to connect to {url}, see log for details'.format(url=url)) + retries += 1 except: # Don't know what's happening, so reraise the original log.exception('Unknown error when trying to connect to {url}'.format(url=url)) diff --git a/tests/functional/openlp_core_common/test_httputils.py b/tests/functional/openlp_core_common/test_httputils.py index 9cb32db25..a32f19172 100644 --- a/tests/functional/openlp_core_common/test_httputils.py +++ b/tests/functional/openlp_core_common/test_httputils.py @@ -24,7 +24,6 @@ Functional tests to test the AppLocation class and related methods. """ import os import tempfile -import socket from unittest import TestCase from unittest.mock import MagicMock, patch @@ -67,7 +66,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is Windows mocked_sys.platform = 'win32' # WHEN: We call get_user_agent() @@ -82,7 +81,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is macOS mocked_sys.platform = 'darwin' # WHEN: We call get_user_agent() @@ -97,7 +96,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is something else mocked_sys.platform = 'freebsd' # WHEN: We call get_user_agent() @@ -119,156 +118,127 @@ class TestHttpUtils(TestCase, TestMixin): # THEN: None should be returned self.assertIsNone(result, 'The return value of get_web_page should be None') - def test_get_web_page(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + @patch('openlp.core.common.httputils.Registry') + def test_get_web_page(self, MockRegistry, mocked_get_user_agent, mocked_requests): """ Test that the get_web_page method works correctly """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent, \ - patch('openlp.core.common.Registry') as MockRegistry: - # GIVEN: Mocked out objects and a fake URL - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects and a fake URL + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + fake_url = 'this://is.a.fake/url' - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with('User-Agent', 'user_agent') - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - mock_get_user_agent.assert_called_with() - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(0, MockRegistry.call_count, 'The Registry() object should have never been called') - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers={'User-Agent': 'user_agent'}, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_once_with() + assert MockRegistry.call_count == 0, 'The Registry() object should have never been called' + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_web_page_with_header(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + def test_get_web_page_with_header(self, mocked_get_user_agent, mocked_requests): """ Test that adding a header to the call to get_web_page() adds the header to the request """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL and a fake header - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' - fake_header = ('Fake-Header', 'fake value') + # GIVEN: Mocked out objects, a fake URL and a fake header + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + fake_url = 'this://is.a.fake/url' + fake_headers = {'Fake-Header': 'fake value'} - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, header=fake_header) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, headers=fake_headers) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with(fake_header[0], fake_header[1]) - self.assertEqual(2, mocked_request_object.add_header.call_count, - 'There should only be 2 calls to add_header') - mock_get_user_agent.assert_called_with() - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + expected_headers = dict(fake_headers) + expected_headers.update({'User-Agent': 'user_agent'}) + mocked_requests.get.assert_called_once_with(fake_url, headers=expected_headers, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_with() + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_web_page_with_user_agent_in_headers(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + def test_get_web_page_with_user_agent_in_headers(self, mocked_get_user_agent, mocked_requests): """ Test that adding a user agent in the header when calling get_web_page() adds that user agent to the request """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL and a fake header - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - fake_url = 'this://is.a.fake/url' - user_agent_header = ('User-Agent', 'OpenLP/2.2.0') + # GIVEN: Mocked out objects, a fake URL and a fake header + mocked_requests.get.return_value = MagicMock(text='text') + fake_url = 'this://is.a.fake/url' + user_agent_headers = {'User-Agent': 'OpenLP/2.2.0'} - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, header=user_agent_header) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, headers=user_agent_headers) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with(user_agent_header[0], user_agent_header[1]) - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - self.assertEqual(0, mock_get_user_agent.call_count, 'get_user_agent should not have been called') - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers=user_agent_headers, + proxies=None, timeout=30.0) + assert mocked_get_user_agent.call_count == 0, 'get_user_agent() should not have been called' + assert returned_page == 'text', 'The returned page should be "test"' - def test_get_web_page_update_openlp(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + @patch('openlp.core.common.httputils.Registry') + def test_get_web_page_update_openlp(self, MockRegistry, mocked_get_user_agent, mocked_requests): """ Test that passing "update_openlp" as true to get_web_page calls Registry().get('app').process_events() """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent, \ - patch('openlp.core.common.httputils.Registry') as MockRegistry: - # GIVEN: Mocked out objects, a fake URL - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - mocked_registry_object = MagicMock() - mocked_application_object = MagicMock() - mocked_registry_object.get.return_value = mocked_application_object - MockRegistry.return_value = mocked_registry_object - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects, a fake URL + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + mocked_registry_object = MagicMock() + mocked_application_object = MagicMock() + mocked_registry_object.get.return_value = mocked_application_object + MockRegistry.return_value = mocked_registry_object + fake_url = 'this://is.a.fake/url' - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, update_openlp=True) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, update_openlp=True) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with('User-Agent', 'user_agent') - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - mocked_registry_object.get.assert_called_with('application') - mocked_application_object.process_events.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers={'User-Agent': 'user_agent'}, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_once_with() + mocked_registry_object.get.assert_called_with('application') + mocked_application_object.process_events.assert_called_with() + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_url_file_size(self): + @patch('openlp.core.common.httputils.requests') + def test_get_url_file_size(self, mocked_requests): """ - Test that passing "update_openlp" as true to get_web_page calls Registry().get('app').process_events() + Test that calling "get_url_file_size" works correctly """ - with patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects, a fake URL + mocked_requests.head.return_value = MagicMock(headers={'Content-Length': 100}) + fake_url = 'this://is.a.fake/url' - # WHEN: The get_url_file_size() method is called - get_url_file_size(fake_url) + # WHEN: The get_url_file_size() method is called + file_size = get_url_file_size(fake_url) - # THEN: The correct methods are called with the correct arguments and a web page is returned - mock_urlopen.assert_called_with(fake_url, timeout=30) + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.head.assert_called_once_with(fake_url, allow_redirects=True, timeout=30.0) + assert file_size == 100 - @patch('openlp.core.ui.firsttimeform.urllib.request.urlopen') - def test_socket_timeout(self, mocked_urlopen): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.os.remove') + def test_socket_timeout(self, mocked_remove, mocked_requests): """ Test socket timeout gets caught """ # GIVEN: Mocked urlopen to fake a network disconnect in the middle of a download - mocked_urlopen.side_effect = socket.timeout() + mocked_requests.get.side_effect = IOError # WHEN: Attempt to retrieve a file - url_get_file(MagicMock(), url='http://localhost/test', f_path=self.tempfile) + url_get_file(MagicMock(), url='http://localhost/test', file_path=self.tempfile) # THEN: socket.timeout should have been caught # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files - self.assertFalse(os.path.exists(self.tempfile), 'FTW url_get_file should have caught socket.timeout') + mocked_remove.assert_called_with(self.tempfile) + assert mocked_remove.call_count == 3, 'os.remove() should have been called 3 times' diff --git a/tests/functional/openlp_core_ui/test_first_time.py b/tests/functional/openlp_core_ui/test_first_time.py index 35507e15e..eb9464375 100644 --- a/tests/functional/openlp_core_ui/test_first_time.py +++ b/tests/functional/openlp_core_ui/test_first_time.py @@ -22,9 +22,6 @@ """ Package to test the openlp.core.utils.__init__ package. """ -import urllib.request -import urllib.error -import urllib.parse from unittest import TestCase from unittest.mock import patch @@ -37,20 +34,21 @@ class TestFirstTimeWizard(TestMixin, TestCase): """ Test First Time Wizard import functions """ - def test_webpage_connection_retry(self): + @patch('openlp.core.common.httputils.requests') + def test_webpage_connection_retry(self, mocked_requests): """ Test get_web_page will attempt CONNECTION_RETRIES+1 connections - bug 1409031 """ # GIVEN: Initial settings and mocks - with patch.object(urllib.request, 'urlopen') as mocked_urlopen: - mocked_urlopen.side_effect = ConnectionError + mocked_requests.get.side_effect = IOError('Unable to connect') - # WHEN: A webpage is requested - try: - get_web_page(url='http://localhost') - except: - pass + # WHEN: A webpage is requested + try: + get_web_page('http://localhost') + except Exception as e: + assert isinstance(e, ConnectionError) - # THEN: urlopen should have been called CONNECTION_RETRIES + 1 count - self.assertEquals(mocked_urlopen.call_count, CONNECTION_RETRIES + 1, - 'get_web_page() should have tried {} times'.format(CONNECTION_RETRIES)) + # THEN: urlopen should have been called CONNECTION_RETRIES + 1 count + assert mocked_requests.get.call_count == CONNECTION_RETRIES, \ + 'get should have been called {} times, but was only called {} times'.format( + CONNECTION_RETRIES, mocked_requests.get.call_count) diff --git a/tests/functional/openlp_core_ui/test_firsttimeform.py b/tests/functional/openlp_core_ui/test_firsttimeform.py index 0f7c6be6a..c90cdd80b 100644 --- a/tests/functional/openlp_core_ui/test_firsttimeform.py +++ b/tests/functional/openlp_core_ui/test_firsttimeform.py @@ -34,7 +34,7 @@ from openlp.core.ui.firsttimeform import FirstTimeForm from tests.helpers.testmixin import TestMixin -FAKE_CONFIG = b""" +FAKE_CONFIG = """ [general] base url = http://example.com/frw/ [songs] @@ -45,7 +45,7 @@ directory = bibles directory = themes """ -FAKE_BROKEN_CONFIG = b""" +FAKE_BROKEN_CONFIG = """ [general] base url = http://example.com/frw/ [songs] @@ -54,7 +54,7 @@ directory = songs directory = bibles """ -FAKE_INVALID_CONFIG = b""" +FAKE_INVALID_CONFIG = """ This is not a config file Some text @@ -112,7 +112,7 @@ class TestFirstTimeForm(TestCase, TestMixin): patch('openlp.core.ui.firsttimeform.Settings') as MockedSettings, \ patch('openlp.core.ui.firsttimeform.gettempdir') as mocked_gettempdir, \ patch('openlp.core.ui.firsttimeform.check_directory_exists') as mocked_check_directory_exists, \ - patch.object(frw.application, 'set_normal_cursor') as mocked_set_normal_cursor: + patch.object(frw.application, 'set_normal_cursor'): mocked_settings = MagicMock() mocked_settings.value.return_value = True MockedSettings.return_value = mocked_settings @@ -192,7 +192,7 @@ class TestFirstTimeForm(TestCase, TestMixin): with patch('openlp.core.ui.firsttimeform.get_web_page') as mocked_get_web_page: first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.return_value.read.return_value = FAKE_BROKEN_CONFIG + mocked_get_web_page.return_value = FAKE_BROKEN_CONFIG # WHEN: The First Time Wizard is downloads the config file first_time_form._download_index() @@ -208,7 +208,7 @@ class TestFirstTimeForm(TestCase, TestMixin): with patch('openlp.core.ui.firsttimeform.get_web_page') as mocked_get_web_page: first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.return_value.read.return_value = FAKE_INVALID_CONFIG + mocked_get_web_page.return_value = FAKE_INVALID_CONFIG # WHEN: The First Time Wizard is downloads the config file first_time_form._download_index() @@ -225,14 +225,13 @@ class TestFirstTimeForm(TestCase, TestMixin): # GIVEN: Initial setup and mocks first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.side_effect = urllib.error.HTTPError(url='http//localhost', - code=407, - msg='Network proxy error', - hdrs=None, - fp=None) + mocked_get_web_page.side_effect = ConnectionError('') + mocked_message_box.Ok = 'OK' + # WHEN: the First Time Wizard calls to get the initial configuration first_time_form._download_index() # THEN: the critical_error_message_box should have been called - self.assertEquals(mocked_message_box.mock_calls[1][1][0], 'Network Error 407', - 'first_time_form should have caught Network Error') + mocked_message_box.critical.assert_called_once_with( + first_time_form, 'Network Error', 'There was a network error attempting to connect to retrieve ' + 'initial configuration information', 'OK')